[libvirt] [PATCH] maint: turn on gcc logical-op checking
This would have detected the bug in commit 38ad33931 (Aug 09), which we missed until commit f828ca35 (Jul 10); over 11 months later. However, on Fedora 13, it also triggers LOTS of warnings from the libcurl-devel header for one file: esx/esx_vi.c: In function 'esxVI_CURL_Perform': esx/esx_vi.c:232: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] esx/esx_vi.c:232: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] esx/esx_vi.c:232: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] ... I figure the gross hack to disable the warnings in the third-party code, along with the reduction in type-safety in just esx_vi.c, is worth the improved compiler checking throughout the rest of libvirt. * acinclude.m4 (--enable-compile-warnings=error): Add -Wlogical-op. * src/esx/esx_vi.c (includes): Hack around broken libcurl-devel header, to avoid compilation warning. Suggested by Daniel P. Berrange. --- In response to https://www.redhat.com/archives/libvir-list/2010-July/msg00497.html and fixing some long lines while I was at it. acinclude.m4 | 16 +++++++++++++--- src/esx/esx_vi.c | 10 ++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/acinclude.m4 b/acinclude.m4 index 8c97184..838ec46 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -36,9 +36,19 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ try_compiler_flags="-Wall -Wformat -Wformat-security -Wmissing-prototypes $common_flags" ;; maximum|error) - try_compiler_flags="-Wall -Wformat -Wformat-security -Wmissing-prototypes -Wnested-externs -Wpointer-arith" - try_compiler_flags="$try_compiler_flags -Wextra -Wshadow -Wcast-align -Wwrite-strings -Waggregate-return" - try_compiler_flags="$try_compiler_flags -Wstrict-prototypes -Winline -Wredundant-decls -Wno-sign-compare" + try_compiler_flags="-Wall -Wformat -Wformat-security" + try_compiler_flags="$try_compiler_flags -Wmissing-prototypes" + try_compiler_flags="$try_compiler_flags -Wnested-externs " + try_compiler_flags="$try_compiler_flags -Wpointer-arith" + try_compiler_flags="$try_compiler_flags -Wextra -Wshadow" + try_compiler_flags="$try_compiler_flags -Wcast-align" + try_compiler_flags="$try_compiler_flags -Wwrite-strings" + try_compiler_flags="$try_compiler_flags -Waggregate-return" + try_compiler_flags="$try_compiler_flags -Wstrict-prototypes" + try_compiler_flags="$try_compiler_flags -Winline" + try_compiler_flags="$try_compiler_flags -Wredundant-decls" + try_compiler_flags="$try_compiler_flags -Wno-sign-compare" + try_compiler_flags="$try_compiler_flags -Wlogical-op" try_compiler_flags="$try_compiler_flags $common_flags" if test "$enable_compile_warnings" = "error" ; then try_compiler_flags="$try_compiler_flags -Werror" diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index 8c43d67..9ea8d24 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -36,6 +36,16 @@ #include "esx_vi_methods.h" #include "esx_util.h" + +/* XXX "esx_vi.h" includes <curl/curl.h>; as of + * libcurl-devel-7.20.1-3.fc13.x86_64, curl ships a version of this + * header that #defines several wrapper macros around underlying + * functions to add type safety for gcc only. However, these macros + * spuriously trip gcc's -Wlogical-op warning. Avoid the warning by + * nuking the wrappers; even if it removes some type-check safety. */ +# undef curl_easy_getinfo +# undef curl_easy_setopt + #define VIR_FROM_THIS VIR_FROM_ESX -- 1.7.1.1
On 07/23/2010 02:28 PM, Eric Blake wrote:
However, on Fedora 13, it also triggers LOTS of warnings from the libcurl-devel header for one file:
esx/esx_vi.c: In function 'esxVI_CURL_Perform': esx/esx_vi.c:232: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op]
For the record, I filed https://bugzilla.redhat.com/show_bug.cgi?id=617757 against curl (and/or gcc) -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2010/7/23 Eric Blake <eblake@redhat.com>:
This would have detected the bug in commit 38ad33931 (Aug 09), which we missed until commit f828ca35 (Jul 10); over 11 months later.
However, on Fedora 13, it also triggers LOTS of warnings from the libcurl-devel header for one file:
esx/esx_vi.c: In function 'esxVI_CURL_Perform': esx/esx_vi.c:232: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] esx/esx_vi.c:232: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] esx/esx_vi.c:232: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] ...
I figure the gross hack to disable the warnings in the third-party code, along with the reduction in type-safety in just esx_vi.c, is worth the improved compiler checking throughout the rest of libvirt.
* acinclude.m4 (--enable-compile-warnings=error): Add -Wlogical-op. * src/esx/esx_vi.c (includes): Hack around broken libcurl-devel header, to avoid compilation warning. Suggested by Daniel P. Berrange. ---
In response to https://www.redhat.com/archives/libvir-list/2010-July/msg00497.html and fixing some long lines while I was at it.
acinclude.m4 | 16 +++++++++++++--- src/esx/esx_vi.c | 10 ++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-)
@@ -36,6 +36,16 @@ #include "esx_vi_methods.h" #include "esx_util.h"
+ +/* XXX "esx_vi.h" includes <curl/curl.h>; as of + * libcurl-devel-7.20.1-3.fc13.x86_64, curl ships a version of this + * header that #defines several wrapper macros around underlying + * functions to add type safety for gcc only. However, these macros + * spuriously trip gcc's -Wlogical-op warning. Avoid the warning by + * nuking the wrappers; even if it removes some type-check safety. */ +# undef curl_easy_getinfo +# undef curl_easy_setopt + #define VIR_FROM_THIS VIR_FROM_ESX
No need to hack here. We can define CURL_DISABLE_TYPECHECK to disable those type checks. Also the warnings are not that spuriously. If you look at typecheck-gcc.h you'll see that the condition of the if clauses only depend onto type information. This type information is static at compile- and runtime, so gcc is correct to warn about them being fixed true or false. This also affects the XenAPI driver because it includes curl.h too. Therefore, I suggest the attached variation of your patch. Matthias
On 07/24/2010 02:36 PM, Matthias Bolte wrote:
+ * nuking the wrappers; even if it removes some type-check safety. */ +# undef curl_easy_getinfo +# undef curl_easy_setopt + #define VIR_FROM_THIS VIR_FROM_ESX
No need to hack here. We can define CURL_DISABLE_TYPECHECK to disable those type checks.
Yes, that sounds nicer.
Also the warnings are not that spuriously. If you look at typecheck-gcc.h you'll see that the condition of the if clauses only depend onto type information. This type information is static at compile- and runtime, so gcc is correct to warn about them being fixed true or false.
In the sense that the conditions are always true or false, yes, gcc could warn. But because they are hidden by macro expansion (especially since the macros come from a third-party header), it would be nice if gcc would avoid the warning, because it is much different then explicitly writing the always-true && directly in the .c file. Meanwhile, I can't help but wonder if the curl.h headers could be written in a manner less prone to trip the gcc warning in the first place. That is, instead of doing: if (arg == type1 && cond) warn(); if (arg == type2 && cond) warn(); ... it could instead do: switch (arg) { type1: if (cond) warn(); break; type2: if (cond) warn(); break; } ... But that's not for libvirt to decide.
This also affects the XenAPI driver because it includes curl.h too. Therefore, I suggest the attached variation of your patch.
ACK to your version as being better than mine. But maybe we should wait for a third-party ACK before pushing either version? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/26/2010 08:27 AM, Eric Blake wrote:
On 07/24/2010 02:36 PM, Matthias Bolte wrote:
+ * nuking the wrappers; even if it removes some type-check safety. */ +# undef curl_easy_getinfo +# undef curl_easy_setopt + #define VIR_FROM_THIS VIR_FROM_ESX
No need to hack here. We can define CURL_DISABLE_TYPECHECK to disable those type checks.
Yes, that sounds nicer.
Older gcc versions had a bug that meant the warnings in the libcurl header were spurious, since corrected in newer gcc, available in Rawhide. In the meantime, a newer libcurl release now available on Fedora 13 works around the older gcc bug. So, either way, should we remove CURL_DISABLE_TYPECHECK (effectively re-enabling the type safety), now that the spurious warnings are no longer triggered? https://bugzilla.redhat.com/show_bug.cgi?id=617757 -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2010/8/27 Eric Blake <eblake@redhat.com>:
On 07/26/2010 08:27 AM, Eric Blake wrote:
On 07/24/2010 02:36 PM, Matthias Bolte wrote:
+ * nuking the wrappers; even if it removes some type-check safety. */ +# undef curl_easy_getinfo +# undef curl_easy_setopt + #define VIR_FROM_THIS VIR_FROM_ESX
No need to hack here. We can define CURL_DISABLE_TYPECHECK to disable those type checks.
Yes, that sounds nicer.
Older gcc versions had a bug that meant the warnings in the libcurl header were spurious, since corrected in newer gcc, available in Rawhide. In the meantime, a newer libcurl release now available on Fedora 13 works around the older gcc bug. So, either way, should we remove CURL_DISABLE_TYPECHECK (effectively re-enabling the type safety), now that the spurious warnings are no longer triggered?
Well, even if this is fixed in GCC >= 4.5.0 and a workaround was added to libcurl and Fedora isn't affected anymore doesn't mean that this problem is fixed for all systems/distros out there. I think we could remove CURL_DISABLE_TYPECHECK for GCC >= 4.5.0, but keep it for older GCC versions. I tried to figure out how to detect GCC version in configure.ac, but my autotools-fu is weak today. Matthias
On 08/27/2010 02:32 AM, Matthias Bolte wrote:
Well, even if this is fixed in GCC>= 4.5.0 and a workaround was added to libcurl and Fedora isn't affected anymore doesn't mean that this problem is fixed for all systems/distros out there.
I think we could remove CURL_DISABLE_TYPECHECK for GCC>= 4.5.0, but keep it for older GCC versions.
Yes, that sounds better. I guess I'll whip up the patch, then.
I tried to figure out how to detect GCC version in configure.ac, but my autotools-fu is weak today.
No need to worry about configure.ac. It is doable all with preprocessor statements: /* Some versions of libcurl trigger a gcc bug with -Wlogical-op * that was fixed for gcc 4.5.0; disable the problematic libcurl * code if we don't detect a good enough compiler. */ #if __GNUC__ < 4 || (__GNUC__ == 4 && __GNUC_MINOR < 5) # define CURL_DISABLE_TYPECHECK #endif -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Sat, Jul 24, 2010 at 10:36:50PM +0200, Matthias Bolte wrote:
2010/7/23 Eric Blake <eblake@redhat.com>: [...] From: Eric Blake <eblake@redhat.com> Date: Fri, 23 Jul 2010 14:28:31 -0600 Subject: [PATCH] maint: turn on gcc logical-op checking
This would have detected the bug in commit 38ad33931 (Aug 09), which we missed until commit f828ca35 (Jul 10); over 11 months later.
However, on Fedora 13, it also triggers LOTS of warnings from the libcurl-devel header for two files:
esx/esx_vi.c: In function 'esxVI_CURL_Perform': esx/esx_vi.c:232: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] esx/esx_vi.c:232: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] esx/esx_vi.c:232: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] ... xenapi/xenapi_driver.c: In function 'call_func': xenapi/xenapi_driver.c:1872: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] xenapi/xenapi_driver.c:1872: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] xenapi/xenapi_driver.c:1872: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] ...
ACK to Matthias' patch, is there is a predefined way to avoid those defines, let's use it ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
2010/7/26 Daniel Veillard <veillard@redhat.com>:
On Sat, Jul 24, 2010 at 10:36:50PM +0200, Matthias Bolte wrote:
2010/7/23 Eric Blake <eblake@redhat.com>: [...] From: Eric Blake <eblake@redhat.com> Date: Fri, 23 Jul 2010 14:28:31 -0600 Subject: [PATCH] maint: turn on gcc logical-op checking
This would have detected the bug in commit 38ad33931 (Aug 09), which we missed until commit f828ca35 (Jul 10); over 11 months later.
However, on Fedora 13, it also triggers LOTS of warnings from the libcurl-devel header for two files:
esx/esx_vi.c: In function 'esxVI_CURL_Perform': esx/esx_vi.c:232: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] esx/esx_vi.c:232: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] esx/esx_vi.c:232: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] ... xenapi/xenapi_driver.c: In function 'call_func': xenapi/xenapi_driver.c:1872: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] xenapi/xenapi_driver.c:1872: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] xenapi/xenapi_driver.c:1872: error: logical '&&' with non-zero constant will always evaluate as true [-Wlogical-op] ...
ACK to Matthias' patch, is there is a predefined way to avoid those defines, let's use it !
Daniel
Thanks, pushed. Matthias
participants (3)
-
Daniel Veillard -
Eric Blake -
Matthias Bolte