[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH 1/3] compiler: add QEMU_WARN_NONNULL_ARGS()
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [RFC PATCH 1/3] compiler: add QEMU_WARN_NONNULL_ARGS() |
Date: |
Wed, 17 Jan 2018 14:39:11 +0000 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
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é <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:
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 :|
[Qemu-devel] [RFC PATCH 2/3] virtio: let virtio_add/clear_feature() use QEMU_WARN_NONNULL_ARGS(), Philippe Mathieu-Daudé, 2018/01/17
[Qemu-devel] [RFC PATCH 3/3] utils: let qemu_find_file() use QEMU_WARN_NONNULL_ARGS(), Philippe Mathieu-Daudé, 2018/01/17
Re: [Qemu-devel] [RFC PATCH 0/3] add QEMU_WARN_NONNULL_ARGS() macro, Philippe Mathieu-Daudé, 2018/01/17
Re: [Qemu-devel] [RFC PATCH 0/3] add QEMU_WARN_NONNULL_ARGS() macro, Richard Henderson, 2018/01/17