[libvirt] [PATCH 0/2] workaround bogus GCC
This combines Eric's and Cole's ideas together with configure check for GCC that is affected by https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69602. Pavel Hrdina (2): build: cleanup GCC < 4.6 -Wlogical-op workaround build: add GCC 6.0 -Wlogical-op workaround m4/virt-compile-warnings.m4 | 22 +++++++++++++++++++++- src/fdstream.c | 4 ++++ src/internal.h | 22 ++++++++++++++++++++++ src/rpc/virnetsshsession.c | 6 ++++++ src/security/security_selinux.c | 2 ++ src/util/virbuffer.c | 11 +++-------- src/util/virstring.c | 9 +-------- src/util/virsysinfo.c | 13 ++----------- 8 files changed, 61 insertions(+), 28 deletions(-) -- 2.8.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- m4/virt-compile-warnings.m4 | 2 +- src/internal.h | 10 ++++++++++ src/util/virbuffer.c | 11 +++-------- src/util/virstring.c | 9 +-------- src/util/virsysinfo.c | 13 ++----------- 5 files changed, 17 insertions(+), 28 deletions(-) diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index 3dd0665..1b0a2cf 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -236,7 +236,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ if test "$gl_cv_warn_c__Wlogical_op" = yes && test "$lv_cv_gcc_wlogical_op_broken" = yes; then - AC_DEFINE_UNQUOTED([BROKEN_GCC_WLOGICALOP], 1, + AC_DEFINE_UNQUOTED([BROKEN_GCC_WLOGICALOP_STRCHR], 1, [Define to 1 if gcc -Wlogical-op reports false positives on strchr]) fi ]) diff --git a/src/internal.h b/src/internal.h index 9ebaf3c..bc3126f 100644 --- a/src/internal.h +++ b/src/internal.h @@ -253,6 +253,16 @@ # define VIR_WARNINGS_RESET # endif +/* Workaround bogus GCC < 4.6 that produces false -Wlogical-op warnings for + * strchr(). Those old GCCs doesn't support push/pop. */ +# if BROKEN_GCC_WLOGICALOP_STRCHR +# define VIR_WARNINGS_NO_WLOGICALOP_STRCHR \ + _Pragma ("GCC diagnostic ignored \"-Wlogical-op\"") +# else +# define VIR_WARNINGS_NO_WLOGICALOP_STRCHR +# endif + + /* * Use this when passing possibly-NULL strings to printf-a-likes. */ diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 43cd1a7..d582e7d 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -417,14 +417,9 @@ virBufferVasprintf(virBufferPtr buf, const char *format, va_list argptr) buf->use += count; } -/* Work around spurious strchr() diagnostics given by -Wlogical-op - * for gcc < 4.6. Doing it via a local pragma keeps the damage - * smaller than disabling it on the package level. Unfortunately, the - * affected GCCs don't allow diagnostic push/pop which would have - * further reduced the impact. */ -#if BROKEN_GCC_WLOGICALOP -# pragma GCC diagnostic ignored "-Wlogical-op" -#endif + +VIR_WARNINGS_NO_WLOGICALOP_STRCHR + /** * virBufferEscapeString: diff --git a/src/util/virstring.c b/src/util/virstring.c index 7ec42aa..2d7fbf3 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -989,14 +989,7 @@ virStringHasControlChars(const char *str) } -/* Work around spurious strchr() diagnostics given by -Wlogical-op - * for gcc < 4.6. Doing it via a local pragma keeps the damage - * smaller than disabling it on the package level. Unfortunately, the - * affected GCCs don't allow diagnostic push/pop which would have - * further reduced the impact. */ -#if BROKEN_GCC_WLOGICALOP -# pragma GCC diagnostic ignored "-Wlogical-op" -#endif +VIR_WARNINGS_NO_WLOGICALOP_STRCHR /** diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index 05d33a8..e8dbd4d 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -428,17 +428,8 @@ virSysinfoRead(void) #elif defined(__s390__) || defined(__s390x__) -/* - we need to ignore warnings about strchr caused by -Wlogical-op - for some GCC versions. - Doing it via a local pragma keeps the damage smaller than - disabling it on the package level. - Unfortunately, the affected GCCs don't allow diagnostic push/pop - which would have further reduced the impact. - */ -# if BROKEN_GCC_WLOGICALOP -# pragma GCC diagnostic ignored "-Wlogical-op" -# endif + +VIR_WARNINGS_NO_WLOGICALOP_STRCHR static char * virSysinfoParseDelimited(const char *base, const char *name, char **value, -- 2.8.1
On Sun, Apr 10, 2016 at 06:37:58PM +0200, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- m4/virt-compile-warnings.m4 | 2 +- src/internal.h | 10 ++++++++++ src/util/virbuffer.c | 11 +++-------- src/util/virstring.c | 9 +-------- src/util/virsysinfo.c | 13 ++----------- 5 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index 3dd0665..1b0a2cf 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -236,7 +236,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
if test "$gl_cv_warn_c__Wlogical_op" = yes && test "$lv_cv_gcc_wlogical_op_broken" = yes; then - AC_DEFINE_UNQUOTED([BROKEN_GCC_WLOGICALOP], 1, + AC_DEFINE_UNQUOTED([BROKEN_GCC_WLOGICALOP_STRCHR], 1, [Define to 1 if gcc -Wlogical-op reports false positives on strchr]) fi ]) diff --git a/src/internal.h b/src/internal.h index 9ebaf3c..bc3126f 100644 --- a/src/internal.h +++ b/src/internal.h @@ -253,6 +253,16 @@ # define VIR_WARNINGS_RESET # endif
+/* Workaround bogus GCC < 4.6 that produces false -Wlogical-op warnings for + * strchr(). Those old GCCs doesn't support push/pop. */
s/doesn't/don't/
+# if BROKEN_GCC_WLOGICALOP_STRCHR +# define VIR_WARNINGS_NO_WLOGICALOP_STRCHR \ + _Pragma ("GCC diagnostic ignored \"-Wlogical-op\"") +# else +# define VIR_WARNINGS_NO_WLOGICALOP_STRCHR +# endif + + /* * Use this when passing possibly-NULL strings to printf-a-likes. */ diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 43cd1a7..d582e7d 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -417,14 +417,9 @@ virBufferVasprintf(virBufferPtr buf, const char *format, va_list argptr) buf->use += count; }
-/* Work around spurious strchr() diagnostics given by -Wlogical-op - * for gcc < 4.6. Doing it via a local pragma keeps the damage - * smaller than disabling it on the package level. Unfortunately, the - * affected GCCs don't allow diagnostic push/pop which would have - * further reduced the impact. */
To make the damage as small as possible, we could create a src/util/virstrchr.c file in which this would be disabled, create something like virStrChr() in it and then disable the use of strchr() everywhere else in the code with syntax-check as we did with some other string functions. Was that discussed as well or should I send it as a follow-up?
-#if BROKEN_GCC_WLOGICALOP -# pragma GCC diagnostic ignored "-Wlogical-op" -#endif + +VIR_WARNINGS_NO_WLOGICALOP_STRCHR +
/** * virBufferEscapeString:
On Mon, Apr 11, 2016 at 11:30:25AM +0200, Martin Kletzander wrote:
On Sun, Apr 10, 2016 at 06:37:58PM +0200, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- m4/virt-compile-warnings.m4 | 2 +- src/internal.h | 10 ++++++++++ src/util/virbuffer.c | 11 +++-------- src/util/virstring.c | 9 +-------- src/util/virsysinfo.c | 13 ++----------- 5 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index 3dd0665..1b0a2cf 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -236,7 +236,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
if test "$gl_cv_warn_c__Wlogical_op" = yes && test "$lv_cv_gcc_wlogical_op_broken" = yes; then - AC_DEFINE_UNQUOTED([BROKEN_GCC_WLOGICALOP], 1, + AC_DEFINE_UNQUOTED([BROKEN_GCC_WLOGICALOP_STRCHR], 1, [Define to 1 if gcc -Wlogical-op reports false positives on strchr]) fi ]) diff --git a/src/internal.h b/src/internal.h index 9ebaf3c..bc3126f 100644 --- a/src/internal.h +++ b/src/internal.h @@ -253,6 +253,16 @@ # define VIR_WARNINGS_RESET # endif
+/* Workaround bogus GCC < 4.6 that produces false -Wlogical-op warnings for + * strchr(). Those old GCCs doesn't support push/pop. */
s/doesn't/don't/
Sigh, shame on me :)
+# if BROKEN_GCC_WLOGICALOP_STRCHR +# define VIR_WARNINGS_NO_WLOGICALOP_STRCHR \ + _Pragma ("GCC diagnostic ignored \"-Wlogical-op\"") +# else +# define VIR_WARNINGS_NO_WLOGICALOP_STRCHR +# endif + + /* * Use this when passing possibly-NULL strings to printf-a-likes. */ diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 43cd1a7..d582e7d 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -417,14 +417,9 @@ virBufferVasprintf(virBufferPtr buf, const char *format, va_list argptr) buf->use += count; }
-/* Work around spurious strchr() diagnostics given by -Wlogical-op - * for gcc < 4.6. Doing it via a local pragma keeps the damage - * smaller than disabling it on the package level. Unfortunately, the - * affected GCCs don't allow diagnostic push/pop which would have - * further reduced the impact. */
To make the damage as small as possible, we could create a src/util/virstrchr.c file in which this would be disabled, create something like virStrChr() in it and then disable the use of strchr() everywhere else in the code with syntax-check as we did with some other string functions. Was that discussed as well or should I send it as a follow-up?
That's actually a good idea and no, it wasn't discussed. I'll push this series and yes send it as follow-up, its a good solution. Thanks, Pavel
On Mon, Apr 11, 2016 at 12:07:26PM +0200, Pavel Hrdina wrote:
On Mon, Apr 11, 2016 at 11:30:25AM +0200, Martin Kletzander wrote:
On Sun, Apr 10, 2016 at 06:37:58PM +0200, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- m4/virt-compile-warnings.m4 | 2 +- src/internal.h | 10 ++++++++++ src/util/virbuffer.c | 11 +++-------- src/util/virstring.c | 9 +-------- src/util/virsysinfo.c | 13 ++----------- 5 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index 3dd0665..1b0a2cf 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -236,7 +236,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
if test "$gl_cv_warn_c__Wlogical_op" = yes && test "$lv_cv_gcc_wlogical_op_broken" = yes; then - AC_DEFINE_UNQUOTED([BROKEN_GCC_WLOGICALOP], 1, + AC_DEFINE_UNQUOTED([BROKEN_GCC_WLOGICALOP_STRCHR], 1, [Define to 1 if gcc -Wlogical-op reports false positives on strchr]) fi ]) diff --git a/src/internal.h b/src/internal.h index 9ebaf3c..bc3126f 100644 --- a/src/internal.h +++ b/src/internal.h @@ -253,6 +253,16 @@ # define VIR_WARNINGS_RESET # endif
+/* Workaround bogus GCC < 4.6 that produces false -Wlogical-op warnings for + * strchr(). Those old GCCs doesn't support push/pop. */
s/doesn't/don't/
Sigh, shame on me :)
+# if BROKEN_GCC_WLOGICALOP_STRCHR +# define VIR_WARNINGS_NO_WLOGICALOP_STRCHR \ + _Pragma ("GCC diagnostic ignored \"-Wlogical-op\"") +# else +# define VIR_WARNINGS_NO_WLOGICALOP_STRCHR +# endif + + /* * Use this when passing possibly-NULL strings to printf-a-likes. */ diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 43cd1a7..d582e7d 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -417,14 +417,9 @@ virBufferVasprintf(virBufferPtr buf, const char *format, va_list argptr) buf->use += count; }
-/* Work around spurious strchr() diagnostics given by -Wlogical-op - * for gcc < 4.6. Doing it via a local pragma keeps the damage - * smaller than disabling it on the package level. Unfortunately, the - * affected GCCs don't allow diagnostic push/pop which would have - * further reduced the impact. */
To make the damage as small as possible, we could create a src/util/virstrchr.c file in which this would be disabled, create something like virStrChr() in it and then disable the use of strchr() everywhere else in the code with syntax-check as we did with some other string functions. Was that discussed as well or should I send it as a follow-up?
That's actually a good idea and no, it wasn't discussed. I'll push this series and yes send it as follow-up, its a good solution.
Just as an update, I worked on it and it's actually not. It does spoil so much of the codebase with super-ugly #include <virstrchr.h> (ugly because it is in every freaking file) and it is only needed for places where the output of strchr(haystack, needle) is used as a boolean (those places that do (strchr(h, n) == h) are just stupid because they should do h[0] == n or *h == n). It got me so sick and tired that I stopped caring so if anyone else wants to clean it up somehow, feel free to try.
Thanks,
Pavel
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
fdstream.c: In function 'virFDStreamWrite': fdstream.c:390:29: error: logical 'or' of equal expressions [-Werror=logical-op] if (errno == EAGAIN || errno == EWOULDBLOCK) { ^~ Fedora rawhide now uses gcc 6.0 and there is a bug with -Wlogical-op producing false warnings. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69602 Use GCC pragma push/pop and ignore -Wlogical-op for GCC that supports push/pop pragma and also has this bug. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- m4/virt-compile-warnings.m4 | 20 ++++++++++++++++++++ src/fdstream.c | 4 ++++ src/internal.h | 12 ++++++++++++ src/rpc/virnetsshsession.c | 6 ++++++ src/security/security_selinux.c | 2 ++ 5 files changed, 44 insertions(+) diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index 1b0a2cf..eb689e2 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -117,6 +117,20 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ [lv_cv_gcc_wlogical_op_broken=yes]) CFLAGS="$save_CFLAGS"]) + AC_CACHE_CHECK([whether gcc gives bogus warnings for -Wlogical-op], + [lv_cv_gcc_wlogical_op_equal_expr_broken], [ + save_CFLAGS="$CFLAGS" + CFLAGS="-O2 -Wlogical-op -Werror" + AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ + #define TEST1 1 + #define TEST2 TEST1 + ]], [[ + int test = 0; + return test == TEST1 || test == TEST2;]])], + [lv_cv_gcc_wlogical_op_equal_expr_broken=no], + [lv_cv_gcc_wlogical_op_equal_expr_broken=yes]) + CFLAGS="$save_CFLAGS"]) + # We might fundamentally need some of these disabled forever, but # ideally we'd turn many of them on dontwarn="$dontwarn -Wfloat-equal" @@ -239,4 +253,10 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ AC_DEFINE_UNQUOTED([BROKEN_GCC_WLOGICALOP_STRCHR], 1, [Define to 1 if gcc -Wlogical-op reports false positives on strchr]) fi + + if test "$gl_cv_warn_c__Wlogical_op" = yes && + test "$lv_cv_gcc_wlogical_op_equal_expr_broken" = yes; then + AC_DEFINE_UNQUOTED([BROKEN_GCC_WLOGICALOP_EQUAL_EXPR], 1, + [Define to 1 if gcc -Wlogical-op reports false positive 'or' equal expr]) + fi ]) diff --git a/src/fdstream.c b/src/fdstream.c index a85cf9d..ef118b5 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -387,7 +387,9 @@ static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) retry: ret = write(fdst->fd, bytes, nbytes); if (ret < 0) { + VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR if (errno == EAGAIN || errno == EWOULDBLOCK) { + VIR_WARNINGS_RESET ret = -2; } else if (errno == EINTR) { goto retry; @@ -437,7 +439,9 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) retry: ret = read(fdst->fd, bytes, nbytes); if (ret < 0) { + VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR if (errno == EAGAIN || errno == EWOULDBLOCK) { + VIR_WARNINGS_RESET ret = -2; } else if (errno == EINTR) { goto retry; diff --git a/src/internal.h b/src/internal.h index bc3126f..3843d92 100644 --- a/src/internal.h +++ b/src/internal.h @@ -245,11 +245,23 @@ _Pragma ("GCC diagnostic push") # endif +/* Workaround bogus GCC 6.0 for logical 'or' equal expression warnings. + * (GCC bz 69602) */ +# if BROKEN_GCC_WLOGICALOP_EQUAL_EXPR +# define VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR \ + _Pragma ("GCC diagnostic push") \ + _Pragma ("GCC diagnostic ignored \"-Wlogical-op\"") +# else +# define VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR \ + _Pragma ("GCC diagnostic push") +# endif + # define VIR_WARNINGS_RESET \ _Pragma ("GCC diagnostic pop") # else # define VIR_WARNINGS_NO_CAST_ALIGN # define VIR_WARNINGS_NO_PRINTF +# define VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR # define VIR_WARNINGS_RESET # endif diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c index 406a831..e742175 100644 --- a/src/rpc/virnetsshsession.c +++ b/src/rpc/virnetsshsession.c @@ -545,9 +545,11 @@ virNetSSHAuthenticateAgent(virNetSSHSessionPtr sess, agent_identity))) return 0; /* key accepted */ + VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR if (ret != LIBSSH2_ERROR_AUTHENTICATION_FAILED && ret != LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED && ret != LIBSSH2_ERROR_PUBLICKEY_UNVERIFIED) { + VIR_WARNINGS_RESET libssh2_session_last_error(sess->session, &errmsg, NULL, 0); virReportError(VIR_ERR_AUTH_FAILED, _("failed to authenticate using SSH agent: %s"), @@ -605,9 +607,11 @@ virNetSSHAuthenticatePrivkey(virNetSSHSessionPtr sess, priv->password)) == 0) return 0; /* success */ + VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR if (priv->password || ret == LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED || ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED) { + VIR_WARNINGS_RESET libssh2_session_last_error(sess->session, &errmsg, NULL, 0); virReportError(VIR_ERR_AUTH_FAILED, _("authentication with private key '%s' " @@ -673,11 +677,13 @@ virNetSSHAuthenticatePrivkey(virNetSSHSessionPtr sess, "has failed: %s"), priv->filename, errmsg); + VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR if (ret == LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED || ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED) return 1; else return -1; + VIR_WARNINGS_RESET } return 0; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 26d95d1..04760a1 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -911,8 +911,10 @@ virSecuritySELinuxSetFileconHelper(const char *path, char *tcon, * hopefully sets one of the necessary SELinux virt_use_{nfs,usb,pci} * boolean tunables to allow it ... */ + VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR if (setfilecon_errno != EOPNOTSUPP && setfilecon_errno != ENOTSUP && setfilecon_errno != EROFS) { + VIR_WARNINGS_RESET virReportSystemError(setfilecon_errno, _("unable to set security context '%s' on '%s'"), tcon, path); -- 2.8.1
On Sun, Apr 10, 2016 at 06:37:57PM +0200, Pavel Hrdina wrote:
This combines Eric's and Cole's ideas together with configure check for GCC that is affected by https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69602.
I think it's a good compromise that doesn't pollute the code very much and keeps the warning-disablement damage level very low, so ACK series so that we build successfully. We can work on it later if we want even better results instead of bikesheding on this solution.
Pavel Hrdina (2): build: cleanup GCC < 4.6 -Wlogical-op workaround build: add GCC 6.0 -Wlogical-op workaround
m4/virt-compile-warnings.m4 | 22 +++++++++++++++++++++- src/fdstream.c | 4 ++++ src/internal.h | 22 ++++++++++++++++++++++ src/rpc/virnetsshsession.c | 6 ++++++ src/security/security_selinux.c | 2 ++ src/util/virbuffer.c | 11 +++-------- src/util/virstring.c | 9 +-------- src/util/virsysinfo.c | 13 ++----------- 8 files changed, 61 insertions(+), 28 deletions(-)
-- 2.8.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Martin Kletzander -
Pavel Hrdina