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

[Daniel P. Berrange]

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 :-(

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

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
-- 
|: 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 :|