[Qemu-devel] [PATCH] build: try improve handling of clang warnings

[Qemu-devel] [PATCH] build: try improve handling of clang warnings

[Gerd Hoffmann]

This patch disables the pragma diagnostic -Wunused-but-set-variable for
clang in util/coroutine-ucontext.c.

This in turn allows us to remove it from the configure check, so the
CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE will succeed for clang.

With that in place clang builds (linux) will use -Werror by default,
which breaks the build due to warning about unaligned struct members.

Given that our structs are often dictated by the hardware qemu emulates
and the file formats we use this can't be fixed easily, so I think it is
reasonaable to turn off this warning.  Do so by adding
-Wno-address-of-packed-member to our list of warning flags.

Now clang builds qemu without warnings.  Well, almost.  There are some
left in the rdma code.  Leaving that to the rdma people.  All others can
use --disable-rdma to workarounds this.

Cc: Thomas Huth <thuth@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 configure                 | 2 +-
 util/coroutine-ucontext.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 6f3921c02a..5e4310b910 100755
--- a/configure
+++ b/configure
@@ -1692,6 +1692,7 @@ gcc_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
 gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags"
 gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags"
 gcc_flags="-Wno-string-plus-int $gcc_flags"
+gcc_flags="-Wno-address-of-packed-member $gcc_flags"
 # Note that we do not add -Werror to gcc_flags here, because that would
 # enable it for all configure tests. If a configure test failed due
 # to -Werror this would just silently disable some features,
@@ -4860,7 +4861,6 @@ fi
 pragma_disable_unused_but_set=no
 cat > $TMPC << EOF
 #pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wunused-but-set-variable"
 #pragma GCC diagnostic ignored "-Wstrict-prototypes"
 #pragma GCC diagnostic pop
 
diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index 926d3402e3..090ba21a13 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -170,7 +170,7 @@ Coroutine *qemu_coroutine_new(void)
 }
 
 #ifdef CONFIG_VALGRIND_H
-#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
+#if defined(CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE) && !defined(__clang__)
 /* Work around an unused variable in the valgrind.h macro... */
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wunused-but-set-variable"
@@ -179,7 +179,7 @@ static inline void valgrind_stack_deregister(CoroutineUContext *co)
 {
     VALGRIND_STACK_DEREGISTER(co->valgrind_stack_id);
 }
-#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
+#if defined(CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE) && !defined(__clang__)
 #pragma GCC diagnostic pop
 #endif
 #endif
-- 
2.9.3

Re: [Qemu-devel] [PATCH] build: try improve handling of clang warnings

[Philippe Mathieu-Daudé]

On 03/09/2018 12:01 PM, Gerd Hoffmann wrote:
> This patch disables the pragma diagnostic -Wunused-but-set-variable for
> clang in util/coroutine-ucontext.c.
> 
> This in turn allows us to remove it from the configure check, so the
> CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE will succeed for clang.
> 
> With that in place clang builds (linux) will use -Werror by default,
> which breaks the build due to warning about unaligned struct members.
> 
> Given that our structs are often dictated by the hardware qemu emulates
> and the file formats we use this can't be fixed easily, so I think it is
> reasonaable to turn off this warning.  Do so by adding

reasonable

> -Wno-address-of-packed-member to our list of warning flags.
> 
> Now clang builds qemu without warnings.  Well, almost.  There are some
> left in the rdma code.  Leaving that to the rdma people.  All others can
> use --disable-rdma to workarounds this.
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

This looks the clever way to fix this.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  configure                 | 2 +-
>  util/coroutine-ucontext.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/configure b/configure
> index 6f3921c02a..5e4310b910 100755
> --- a/configure
> +++ b/configure
> @@ -1692,6 +1692,7 @@ gcc_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
>  gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags"
>  gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags"
>  gcc_flags="-Wno-string-plus-int $gcc_flags"
> +gcc_flags="-Wno-address-of-packed-member $gcc_flags"
>  # Note that we do not add -Werror to gcc_flags here, because that would
>  # enable it for all configure tests. If a configure test failed due
>  # to -Werror this would just silently disable some features,
> @@ -4860,7 +4861,6 @@ fi
>  pragma_disable_unused_but_set=no
>  cat > $TMPC << EOF
>  #pragma GCC diagnostic push
> -#pragma GCC diagnostic ignored "-Wunused-but-set-variable"
>  #pragma GCC diagnostic ignored "-Wstrict-prototypes"
>  #pragma GCC diagnostic pop
>  
> diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
> index 926d3402e3..090ba21a13 100644
> --- a/util/coroutine-ucontext.c
> +++ b/util/coroutine-ucontext.c
> @@ -170,7 +170,7 @@ Coroutine *qemu_coroutine_new(void)
>  }
>  
>  #ifdef CONFIG_VALGRIND_H
> -#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
> +#if defined(CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE) && !defined(__clang__)
>  /* Work around an unused variable in the valgrind.h macro... */
>  #pragma GCC diagnostic push
>  #pragma GCC diagnostic ignored "-Wunused-but-set-variable"
> @@ -179,7 +179,7 @@ static inline void valgrind_stack_deregister(CoroutineUContext *co)
>  {
>      VALGRIND_STACK_DEREGISTER(co->valgrind_stack_id);
>  }
> -#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
> +#if defined(CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE) && !defined(__clang__)
>  #pragma GCC diagnostic pop
>  #endif
>  #endif
> 

Re: [Qemu-devel] [PATCH] build: try improve handling of clang warnings

[Thomas Huth]

On 09.03.2018 12:01, Gerd Hoffmann wrote:
> This patch disables the pragma diagnostic -Wunused-but-set-variable for
> clang in util/coroutine-ucontext.c.
> 
> This in turn allows us to remove it from the configure check, so the
> CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE will succeed for clang.
> 
> With that in place clang builds (linux) will use -Werror by default,
> which breaks the build due to warning about unaligned struct members.
> 
> Given that our structs are often dictated by the hardware qemu emulates
> and the file formats we use this can't be fixed easily, so I think it is
> reasonaable to turn off this warning.  Do so by adding
> -Wno-address-of-packed-member to our list of warning flags.
> 
> Now clang builds qemu without warnings.  Well, almost.  There are some
> left in the rdma code.  Leaving that to the rdma people.  All others can
> use --disable-rdma to workarounds this.
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  configure                 | 2 +-
>  util/coroutine-ucontext.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/configure b/configure
> index 6f3921c02a..5e4310b910 100755
> --- a/configure
> +++ b/configure
> @@ -1692,6 +1692,7 @@ gcc_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
>  gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags"
>  gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags"
>  gcc_flags="-Wno-string-plus-int $gcc_flags"
> +gcc_flags="-Wno-address-of-packed-member $gcc_flags"

Since the warning is sometimes also a good idea ... maybe it would
rather be better to use -Wno-error=address-of-packed-member instead?

Anyway, with or without that change:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH] build: try improve handling of clang warnings

[Peter Maydell]

On 9 March 2018 at 11:01, Gerd Hoffmann <kraxel@redhat.com> wrote:
> This patch disables the pragma diagnostic -Wunused-but-set-variable for
> clang in util/coroutine-ucontext.c.
>
> This in turn allows us to remove it from the configure check, so the
> CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE will succeed for clang.
>
> With that in place clang builds (linux) will use -Werror by default,
> which breaks the build due to warning about unaligned struct members.
>
> Given that our structs are often dictated by the hardware qemu emulates
> and the file formats we use this can't be fixed easily, so I think it is
> reasonaable to turn off this warning.  Do so by adding
> -Wno-address-of-packed-member to our list of warning flags.

I disagree with this part. Everywhere we do this is at least
plausibly a bug, and we should fix the bugs. Taking addresses of
members in packed structs and then passing them to functions will
break on some architectures, because it drops the information that
the field is not at an aligned address.

(In general this is one reason why I prefer to avoid the coding
pattern of trying to reflect hardware or in-memory guest data
structures with host C struct layout. It may happen to work on
x86 but it's not reliably portable.)

> @@ -1692,6 +1692,7 @@ gcc_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
>  gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags"
>  gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags"
>  gcc_flags="-Wno-string-plus-int $gcc_flags"
> +gcc_flags="-Wno-address-of-packed-member $gcc_flags"
>  # Note that we do not add -Werror to gcc_flags here, because that would
>  # enable it for all configure tests. If a configure test failed due
>  # to -Werror this would just silently disable some features,
> @@ -4860,7 +4861,6 @@ fi
>  pragma_disable_unused_but_set=no
>  cat > $TMPC << EOF
>  #pragma GCC diagnostic push
> -#pragma GCC diagnostic ignored "-Wunused-but-set-variable"
>  #pragma GCC diagnostic ignored "-Wstrict-prototypes"
>  #pragma GCC diagnostic pop
>
> diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
> index 926d3402e3..090ba21a13 100644
> --- a/util/coroutine-ucontext.c
> +++ b/util/coroutine-ucontext.c
> @@ -170,7 +170,7 @@ Coroutine *qemu_coroutine_new(void)
>  }
>
>  #ifdef CONFIG_VALGRIND_H
> -#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
> +#if defined(CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE) && !defined(__clang__)
>  /* Work around an unused variable in the valgrind.h macro... */
>  #pragma GCC diagnostic push
>  #pragma GCC diagnostic ignored "-Wunused-but-set-variable"
> @@ -179,7 +179,7 @@ static inline void valgrind_stack_deregister(CoroutineUContext *co)
>  {
>      VALGRIND_STACK_DEREGISTER(co->valgrind_stack_id);
>  }
> -#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
> +#if defined(CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE) && !defined(__clang__)

Should we have configure test for the availability of this
diagnostic rather than assuming that clang never has it and never will?

>  #pragma GCC diagnostic pop
>  #endif
>  #endif
> --
> 2.9.3

thanks
-- PMM

Re: [Qemu-devel] [PATCH] build: try improve handling of clang warnings

[Gerd Hoffmann]

On Fri, Mar 09, 2018 at 11:32:30AM +0000, Peter Maydell wrote:
> On 9 March 2018 at 11:01, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > This patch disables the pragma diagnostic -Wunused-but-set-variable for
> > clang in util/coroutine-ucontext.c.
> >
> > This in turn allows us to remove it from the configure check, so the
> > CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE will succeed for clang.
> >
> > With that in place clang builds (linux) will use -Werror by default,
> > which breaks the build due to warning about unaligned struct members.
> >
> > Given that our structs are often dictated by the hardware qemu emulates
> > and the file formats we use this can't be fixed easily, so I think it is
> > reasonaable to turn off this warning.  Do so by adding
> > -Wno-address-of-packed-member to our list of warning flags.
> 
> I disagree with this part. Everywhere we do this is at least
> plausibly a bug, and we should fix the bugs. Taking addresses of
> members in packed structs and then passing them to functions will
> break on some architectures, because it drops the information that
> the field is not at an aligned address.

Ok, I guess I pick up Thomas' idea then and turn it into a warning using
-Wno-error=address-of-packed-member.  That way it will not fail the
build with -Werror, but we still see it in the logs.

cheers,
  Gerd