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

[Daniel P. Berrangé]

On Wed, Jan 02, 2019 at 02:37:00PM -0600, Eric Blake wrote:
> Adding Rich, since he recently raised the topic on the libguestfs list
> and pointed to libvirt as precedence:
> 
> On 12/18/18 9:47 AM, Daniel P. Berrangé wrote:
> > On Tue, Dec 18, 2018 at 10:39:20AM -0500, John Ferlan wrote:
> >> Commit 912c6b22 modified the virCommandAddArg and virCommandAddArgPair
> >> to perform NULL argument checking; however, no corresponding change
> >> to the prototypes was made to remove the ATTRIBUTE_NONNULL, so the
> >> Coverity build failed. Adjust the prototypes accordingly.
> > 
> > This sounds wrong or at least different from the past.
> > 
> > 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. 

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.

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.

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.

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. 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|