[libvirt] [PATCH] build: silence stupid gcc warning on STREQ_NULLABLE
Our existing STRNEQ_NULLABLE() triggered a warning in gcc 4.7 when used with a literal NULL argument: qemumonitorjsontest.c: In function 'testQemuMonitorJSONGetMachines': qemumonitorjsontest.c:289:5: error: null argument where non-null required (argument 1) [-Werror=nonnull] even though the strcmp is provably dead when a null argument is present. Squelch the warning by refactoring things so that gcc never sees strcmp() called with NULL arguments (we still compare NULL as not equal to "", this rewrite merely aids gcc). Next, gcc has a valid warning about a literal NULLSTR(NULL): qemumonitorjsontest.c:289:5: error: invalid application of 'sizeof' to a void type [-Werror=pointer-arith] Of course, you'd never write NULLSTR(NULL) directly, but it is handy to use through macros. And since we only really need the code to be warning-free when developing with modern gcc, and merely compiled correctly elsewhere, we can rely on gcc extensions to avoid dereferencing NULL even inside a sizeof operation. * src/internal.h (STREQ_NULLABLE, STRNEQ_NULLABLE): Avoid gcc 4.7 stupidity. (NULLSTR): Allow passing compile-time constants via macros. --- src/internal.h | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/internal.h b/src/internal.h index 300de3a..c27ffc5 100644 --- a/src/internal.h +++ b/src/internal.h @@ -79,10 +79,9 @@ # define STRSKIP(a,b) (STRPREFIX(a,b) ? (a) + strlen(b) : NULL) # define STREQ_NULLABLE(a, b) \ - ((!(a) && !(b)) || ((a) && (b) && STREQ((a), (b)))) + ((a) ? (b) && STREQ((a) ? (a) : "", (b) ? (b) : "") : !(b)) # define STRNEQ_NULLABLE(a, b) \ - ((!(a) ^ !(b)) || ((a) && (b) && STRNEQ((a), (b)))) - + ((a) ? !(b) || STRNEQ((a) ? (a) : "", (b) ? (b) : "") : !!(b)) # define NUL_TERMINATE(buf) do { (buf)[sizeof(buf)-1] = '\0'; } while (0) # define ARRAY_CARDINALITY(Array) (sizeof(Array) / sizeof(*(Array))) @@ -206,9 +205,22 @@ /* * Use this when passing possibly-NULL strings to printf-a-likes. */ -# define NULLSTR(s) \ - ((void)verify_true(sizeof(*(s)) == sizeof(char)), \ - (s) ? (s) : "(null)") +# if __GNUC_PREREQ(4, 6) +char *link_error_due_to_bad_NULLSTR_type(void); +# define NULLSTR(s) \ + ({ \ + const char *_tmp; \ + if (__builtin_constant_p(s) || \ + __builtin_types_compatible_p(typeof(s), char *) || \ + __builtin_types_compatible_p(typeof(s), const char *)) \ + _tmp = (s) ? (s) : "(null)"; \ + else \ + _tmp = link_error_due_to_bad_NULLSTR_type(); \ + _tmp; \ + }) +# else +# define NULLSTR(s) ((s) ? (s) : "(null)") +# endif /** * TODO: -- 1.7.11.4
On Mon, Aug 20, 2012 at 01:51:49PM -0600, Eric Blake wrote:
Our existing STRNEQ_NULLABLE() triggered a warning in gcc 4.7 when used with a literal NULL argument:
qemumonitorjsontest.c: In function 'testQemuMonitorJSONGetMachines': qemumonitorjsontest.c:289:5: error: null argument where non-null required (argument 1) [-Werror=nonnull]
even though the strcmp is provably dead when a null argument is present. Squelch the warning by refactoring things so that gcc never sees strcmp() called with NULL arguments (we still compare NULL as not equal to "", this rewrite merely aids gcc).
Next, gcc has a valid warning about a literal NULLSTR(NULL):
qemumonitorjsontest.c:289:5: error: invalid application of 'sizeof' to a void type [-Werror=pointer-arith]
Of course, you'd never write NULLSTR(NULL) directly, but it is handy to use through macros. And since we only really need the code to be warning-free when developing with modern gcc, and merely compiled correctly elsewhere, we can rely on gcc extensions to avoid dereferencing NULL even inside a sizeof operation.
* src/internal.h (STREQ_NULLABLE, STRNEQ_NULLABLE): Avoid gcc 4.7 stupidity. (NULLSTR): Allow passing compile-time constants via macros. --- src/internal.h | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)
Not sure I entirely understand the GCC black magic, but if it works for you I'll ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 08/21/2012 03:25 AM, Daniel P. Berrange wrote:
On Mon, Aug 20, 2012 at 01:51:49PM -0600, Eric Blake wrote:
Our existing STRNEQ_NULLABLE() triggered a warning in gcc 4.7 when used with a literal NULL argument:
qemumonitorjsontest.c: In function 'testQemuMonitorJSONGetMachines': qemumonitorjsontest.c:289:5: error: null argument where non-null required (argument 1) [-Werror=nonnull]
* src/internal.h (STREQ_NULLABLE, STRNEQ_NULLABLE): Avoid gcc 4.7 stupidity. (NULLSTR): Allow passing compile-time constants via macros. --- src/internal.h | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)
Not sure I entirely understand the GCC black magic, but if it works
I think the STREQ_NULLABLE changes were pretty easy (convert NULL to "" in the dead code branches so that the unreached strcmp() no longer complains about a NULL arg), but agree that the NULLSTR changes were more black magic. There, the premise is that a literal (such as NULL or "pc") or a variable with [const] char * are accepted, while all other types fall through to the else branch and cause a link error, which makes it so the compiler will complain if you try to do NULLSTR(char) or NULLSTR(char**). Basically, a form of compiler-enforced type checking if your compiler is decent enough. But you made me think about it more - the cases we want to warn about are _already_ warned thanks to other gcc errors - I quickly tested that with F17 gcc: NULLSTR(virBufferPtr) => error: pointer type mismatch in conditional expression [-Werror] NULLSTR(char) => error: pointer/integer type mismatch in conditional expression [-Werror] while NULLSTR(NULL) and NULLSTR("") got through without warnings all without my gyrations.
for you I'll ACK
Thanks; I squashed this in before pushing, since it's simpler and still gave the type safety we originally wanted with the verify_true(): diff --git i/src/internal.h w/src/internal.h index c27ffc5..832a931 100644 --- i/src/internal.h +++ w/src/internal.h @@ -205,22 +205,7 @@ /* * Use this when passing possibly-NULL strings to printf-a-likes. */ -# if __GNUC_PREREQ(4, 6) -char *link_error_due_to_bad_NULLSTR_type(void); -# define NULLSTR(s) \ - ({ \ - const char *_tmp; \ - if (__builtin_constant_p(s) || \ - __builtin_types_compatible_p(typeof(s), char *) || \ - __builtin_types_compatible_p(typeof(s), const char *)) \ - _tmp = (s) ? (s) : "(null)"; \ - else \ - _tmp = link_error_due_to_bad_NULLSTR_type(); \ - _tmp; \ - }) -# else -# define NULLSTR(s) ((s) ? (s) : "(null)") -# endif +# define NULLSTR(s) ((s) ? (s) : "(null)") /** * TODO: -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake