[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 1/2/19 2:37 PM, Eric Blake 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.

Also worth considering:

https://lists.fedoraproject.org/archives/list/devel lists fedoraproject org/thread/VA2QCXJGXVLH43327TRR5UM2BP52DWIC/

mentions that we can use -fsanitize=nonnull -fsanitize=returns-nonnull
(both parts of the larger -fsanitize=undefined) for runtime checking of
cases that sneak into the code in spite of the compile-time checking.
gnulib's manywarnings module does not currently enable -fsanitize
automatically, so we'd have to do a bit more work if we want to take
advantage of that (there's also a question of how much runtime slowdown
happens any time we run with -fsanitize= turned on, to the point where
it may be something useful during CI testing but not in general)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]