[libvirt] [PATCH] util: Remove unnecessary ATTRIBUTE_NONNULL for virCommandAddArg[Pair]

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190110/1fa8263e/attachment-0001.sig>