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

["Daniel P. Berrange" <berrange@redhat.com>]

On Wed, Jan 17, 2018 at 11:33:34AM -0300, Philippe Mathieu-Daudé wrote:
> 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é <f4bug@amsat.org>
> >> ---
> >>  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:

No idea offhand - Eric wrote this so perhaps he had a reason for that
else branch style.

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