Re: [Qemu-devel] [RFC PATCH 1/3] compiler: add QEMU_WARN_NONNULL_ARGS()

[Philippe Mathieu-Daudé]

On 01/17/2018 10:32 AM, Daniel P. Berrange wrote:
> On Wed, Jan 17, 2018 at 10:18:19AM -0300, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>> ---
>>  include/qemu/compiler.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
>> index 340e5fdc09..d9b2489391 100644
>> --- a/include/qemu/compiler.h
>> +++ b/include/qemu/compiler.h
>> @@ -26,6 +26,8 @@
>>  
>>  #define QEMU_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
>>  
>> +#define QEMU_WARN_NONNULL_ARGS(args...) __attribute__((nonnull(args)))
> 
> If we take this, it should come with a warning attached to it, because
> it has really nasty behaviour with GCC. Consider code like:
> 
>   void foo(void *bar) __attribute__((nonnull(1)));
> 
>   ...
> 
>   void foo(void *bar) { if (!bar) return; }
> 
> GCC may or may not warn you about passing NULL for the 'bar'
> parameter, but it will none the less assume nothing passes
> NULL, and thus remove the 'if (!bar)' conditional during
> optimization. IOW, adding nonnull annotations can actually
> make your code less robust :-(

TIL!

> After having a number of crashes in libvirt caused by gcc
> optimizing out checks for NULL, we now only define nonnull
> when running under static analysis (coverity) and not when
> compiling normally.
> 
> https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/internal.h;h=5895030415968d72200599e8a59bbf01ffc2d5a3;hb=HEAD#l162

Why do you use __attribute__(()) ? Isn't this enough:

#if defined __clang_analyzer__ || defined __COVERITY__
#define QEMU_STATIC_ANALYSIS 1
+#define QEMU_WARN_NONNULL_ARGS(args...) __attribute__((nonnull(args)))
+#else
+#define QEMU_WARN_NONNULL_ARGS(args...)
#endif

> The 2 functions you've added nonnull attrs to look safe enough,
> but people might unwittingly use this elsewhere in QEMU in future
> not realizing the side-effect it has.
> 
> Regards,
> Daniel
>