[libvirt] [PATCH] build: make ATTRIBUTE_NONNULL() a NOP unless STATIC_ANALYSIS is on

[Laine Stump]

On 04/26/2012 08:57 AM, Eric Blake wrote:
> On 04/26/2012 12:56 AM, Laine Stump wrote:
>> The ATTRIBUTE_NONNULL(m) macro normally resolves to the gcc builtin
>> __attribute__((__nonnull__(m))). The effect of this in gcc is
>> unfortunately only to make gcc believe that "m" can never possibly be
>> NULL, *not* to add in any checks to guarantee that it isn't ever NULL
>> (i.e. it is an optimization aid, *not* something to verify code
>> correctness.) - see the following gcc bug report for more details:
>>
>>   http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308
>>
>> Static source analyzers such as clang and coverity apparently can use
>> ATTRIBUTE_NONNULL(), though, to detect dead code (in the case that the
>> arg really is guaranteed non-NULL), as well as situations where an
>> obviously NULL arg is given to the function.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=815270 is a good example
>> of a bug caused by erroneous application of ATTRIBUTE_NONNULL().
>> Several people spent a long time staring at this code and not finding
>> the problem, because the problem wasn't in the function itself, but in
>> the prototype that specified ATTRIBUTE_NONNULL() for an arg that
>> actually *wasn't* always non-NULL, and caused a segv when dereferenced
>> (even though the code that dereferenced the pointer was inside an if()
>> that checked for a NULL pointer, that code was optimized out by gcc).
>>
>> There may be some very small gain to be had from the optimizations
>> that can be inferred from ATTRIBUTE_NONNULL(), but it seems safer to
>> err on the side of generating code that behaves as expected, while
>> turning on the attribute for static analyzers.
>>
>> (dissenting opinions welcome :-)
> None from me :)

Since nobody said they thought it was a bad idea, I've pushed it.