On 1/10/19 5:23 AM, Daniel P. Berrangé wrote: >>> >>> We have historically still added nonnull annotations even when >>> having checks for NULL in the impl. We had to explicitly disabble >>> the nonnull annotation when building under GCC to prevent it from >>> optimizing out the NULL checks. We left it enabled for STATIC_ANALYSIS >>> so that coverity would still report if any callers passed a NULL into >>> the methods. >> >> Looking at the gcc bug and following some of the links mentioned therein, >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 >> https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01103.html >> >> It looks like modern gcc has added -Wnonnull-compare that noisily >> informs us any time we have an ATTRIBUTE_NONNULL mismatch with the body >> of a function comparing that parameter against NULL after all. And >> gnulib's manywarnings module turns that on automatically (at least for >> gcc 8.2.1 on Fedora 29). >> >> That is sufficient to fix the bug that led us to historically disable >> ATTRIBUTE_NONNULL under gcc (commit eefb881), so maybe it's time to just >> blindly enable ATTRIBUTE_NONNULL everywhere on the grounds that we have >> enough developers and CI coverage to now immediately catch inconsistent >> attributes rather than risking silently mis-compiled code that omits the >> branch after a NULL comparison as dead code. > > Yes & no. > > Originally the desire was that we would have functions which were > marked NONNULL and *also* have "if (foo == NULL) ... return..' > as a second safety net. No, I think that any case where we had 'if (foo == NULL)' but didn't change the attribute was not a safety net, but a mismatch in declared intentions because of the gcc bug not informing us about the mismatch. > > IOW, we were aiming to get *both* compile time and runtime checking > for NULL, since compile time checks were unsufficiently safe on their > own. > > The compiler was "clever" and optimized out the runtime checks, leaving > only the incomplete compile time checks. The compiler's bug was not that it optimized out the safety checks because we had a buggy attribute, but that it did not warn us that it was optimizing out the safety checks so that we could fix our buggy attribute. But that bug has been fixed. > > We compromised and only set __attribute__(nonnull) under coverity. The > idea was that coverity would get us the static checks as a substiute > for the compile time checks, while we still have the runtime checks. Coverity was able to warn about attribute mismatch even where older gcc wasn't able to, so having the attribute set under coverity let us flag our bugs where we had attribute mismatch to fix them, while avoiding the mis-compiled code where gcc optimized without warning. But now that gcc no longer optimizes without warning, we can rely on gcc instead of coverity to flag the mismatch, which is a much lower barrier of entry. > > Using -Wnonnull-compare would detect when gcc did this dangerous > optimization, but it would still force us to either remove either > the compile time check, or the runtime check. Which is correct. Either the compile-time check is correct (and we don't need the runtime check - if the programmer passes in NULL in spite of the attribute, they deserve the resulting crash), or the attribute is wrong (if we are doing a runtime check, then the function should NOT have the attribute). > > I do wonder if we can enable __attribute__(nonnull) under GCC and > then set '-fdelete-null-pointer-checks' to stop it deleting our > runtime NULL checks, so we get both compile time & runtime protection. I think that leads to the more confusing situation where we are documenting something that is not true. If we're going to handle NULL, then there's no point in telling users not to rely on that. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
