| Message ID | 20180117131821.18700-2-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | add QEMU_WARN_NONNULL_ARGS() macro | expand |
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 :-( 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
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: #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 >
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
On 01/17/2018 08:39 AM, Daniel P. Berrange wrote: >>> >>> 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 :-( The gcc bug mentioned in the libvirt code says that newer gcc may be smarter than when we added the libvirt workaround: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 But I haven't played with it in a long time (the sour experience with misoptimized code with no warning has made me wary of re-trying). >> >> 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 My reasoning for using the empty attribute was to at least ensure that the gcc compilation agrees that the annotation is syntactically valid (nothing worse than sticking an __attribute__ code in a place that gcc doesn't like, but only for static checker builds, so you don't learn about it right away). Defining the macro to nothing, rather than an empty attribute, makes it harder for the common-case compilation to detect placement problems.
On 01/17/2018 11:56 AM, Eric Blake wrote: > On 01/17/2018 08:39 AM, Daniel P. Berrange wrote: > >>>> >>>> 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 :-( > > The gcc bug mentioned in the libvirt code says that newer gcc may be > smarter than when we added the libvirt workaround: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 > > But I haven't played with it in a long time (the sour experience with > misoptimized code with no warning has made me wary of re-trying). I have been there... >>> >>> 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 > > My reasoning for using the empty attribute was to at least ensure that > the gcc compilation agrees that the annotation is syntactically valid > (nothing worse than sticking an __attribute__ code in a place that gcc > doesn't like, but only for static checker builds, so you don't learn > about it right away). Defining the macro to nothing, rather than an > empty attribute, makes it harder for the common-case compilation to > detect placement problems. I wouldn't have thought of that, thanks! Phil.
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))) + #define QEMU_SENTINEL __attribute__((sentinel)) #if QEMU_GNUC_PREREQ(4, 3)
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- include/qemu/compiler.h | 2 ++ 1 file changed, 2 insertions(+)