[PATCH v2 0/4] build qemu with gcc and tsan

[PATCH v2 0/4] build qemu with gcc and tsan

[Pierrick Bouvier]

While working on a concurrency bug, I gave a try to tsan builds for QEMU. I
noticed it didn't build out of the box with recent gcc, so I fixed compilation.
In more, updated documentation to explain how to build a sanitized glib to avoid
false positives related to glib synchronisation primitives.

v2
--

- forgot to signoff commits

Pierrick Bouvier (4):
  meson: hide tsan related warnings
  target/i386: fix build warning (gcc-12 -fsanitize=thread)
  target/s390x: fix build warning (gcc-12 -fsanitize=thread)
  docs/devel: update tsan build documentation

 docs/devel/testing.rst       | 26 ++++++++++++++++++++++----
 meson.build                  | 10 +++++++++-
 target/i386/kvm/kvm.c        |  4 ++--
 target/s390x/tcg/translate.c |  1 -
 4 files changed, 33 insertions(+), 8 deletions(-)

-- 
2.39.2

[PATCH v2 1/4] meson: hide tsan related warnings

[Pierrick Bouvier]

When building with gcc-12 -fsanitize=thread, gcc reports some
constructions not supported with tsan.
Found on debian stable.

qemu/include/qemu/atomic.h:36:52: error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’ [-Werror=tsan]
   36 | #define smp_mb()                     ({ barrier(); __atomic_thread_fence(__ATOMIC_SEQ_CST); })
      |                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 meson.build | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 81ecd4bae7c..52e5aa95cc0 100644
--- a/meson.build
+++ b/meson.build
@@ -499,7 +499,15 @@ if get_option('tsan')
                          prefix: '#include <sanitizer/tsan_interface.h>')
     error('Cannot enable TSAN due to missing fiber annotation interface')
   endif
-  qemu_cflags = ['-fsanitize=thread'] + qemu_cflags
+  tsan_warn_suppress = []
+  # gcc (>=11) will report constructions not supported by tsan:
+  # "error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’"
+  # https://gcc.gnu.org/gcc-11/changes.html
+  # However, clang does not support this warning and this triggers an error.
+  if cc.has_argument('-Wno-tsan')
+    tsan_warn_suppress = ['-Wno-tsan']
+  endif
+  qemu_cflags = ['-fsanitize=thread'] + tsan_warn_suppress + qemu_cflags
   qemu_ldflags = ['-fsanitize=thread'] + qemu_ldflags
 endif
 
-- 
2.39.2

[PATCH v2 2/4] target/i386: fix build warning (gcc-12 -fsanitize=thread)

[Pierrick Bouvier]

Found on debian stable.

../target/i386/kvm/kvm.c: In function ‘kvm_handle_rdmsr’:
../target/i386/kvm/kvm.c:5345:1: error: control reaches end of non-void function [-Werror=return-type]
 5345 | }
      | ^
../target/i386/kvm/kvm.c: In function ‘kvm_handle_wrmsr’:
../target/i386/kvm/kvm.c:5364:1: error: control reaches end of non-void function [-Werror=return-type]
 5364 | }

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 target/i386/kvm/kvm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 31f149c9902..ddec27edd5b 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -5770,7 +5770,7 @@ static int kvm_handle_rdmsr(X86CPU *cpu, struct kvm_run *run)
         }
     }
 
-    assert(false);
+    g_assert_not_reached();
 }
 
 static int kvm_handle_wrmsr(X86CPU *cpu, struct kvm_run *run)
@@ -5789,7 +5789,7 @@ static int kvm_handle_wrmsr(X86CPU *cpu, struct kvm_run *run)
         }
     }
 
-    assert(false);
+    g_assert_not_reached();
 }
 
 static bool has_sgx_provisioning;
-- 
2.39.2

[PATCH v2 3/4] target/s390x: fix build warning (gcc-12 -fsanitize=thread)

[Pierrick Bouvier]

Found on debian stable.

../target/s390x/tcg/translate.c: In function ‘get_mem_index’:
../target/s390x/tcg/translate.c:398:1: error: control reaches end of non-void function [-Werror=return-type]
  398 | }

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 target/s390x/tcg/translate.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index c81e035dea4..bcfff40b255 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -392,7 +392,6 @@ static int get_mem_index(DisasContext *s)
         return MMU_HOME_IDX;
     default:
         g_assert_not_reached();
-        break;
     }
 #endif
 }
-- 
2.39.2

[PATCH v2 4/4] docs/devel: update tsan build documentation

[Pierrick Bouvier]

Mention it's now possible to build with gcc, instead of clang, and
explain how to build a sanitized glib version.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 docs/devel/testing.rst | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index af73d3d64fb..f10cfc3f786 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -634,20 +634,38 @@ Building and Testing with TSan
 It is possible to build and test with TSan, with a few additional steps.
 These steps are normally done automatically in the docker.
 
-There is a one time patch needed in clang-9 or clang-10 at this time:
+TSan is supported for clang and gcc.
+One particularity of sanitizers is that all the code, including shared objects
+dependencies, should be built with it.
+In the case of TSan, any synchronization primitive from glib (GMutex for
+instance) will not be recognized, and will lead to false positives.
+
+To build a tsan version of glib:
 
 .. code::
 
-  sed -i 's/^const/static const/g' \
-      /usr/lib/llvm-10/lib/clang/10.0.0/include/sanitizer/tsan_interface.h
+   $ git clone --depth=1 --branch=2.81.0 https://github.com/GNOME/glib.git
+   $ cd glib
+   $ CFLAGS="-O2 -g -fsanitize=thread" meson build
+   $ ninja -C build
 
 To configure the build for TSan:
 
 .. code::
 
-  ../configure --enable-tsan --cc=clang-10 --cxx=clang++-10 \
+  ../configure --enable-tsan \
                --disable-werror --extra-cflags="-O0"
 
+When executing qemu, don't forget to point to tsan glib:
+
+.. code::
+
+   $ glib_dir=/path/to/glib
+   $ export LD_LIBRARY_PATH=$glib_dir/build/gio:$glib_dir/build/glib:$glib_dir/build/gmodule:$glib_dir/build/gobject:$glib_dir/build/gthread
+   # check correct version is used
+   $ ldd build/qemu-x86_64 | grep glib
+   $ qemu-system-x86_64 ...
+
 The runtime behavior of TSAN is controlled by the TSAN_OPTIONS environment
 variable.
 
-- 
2.39.2

Re: [PATCH v2 2/4] target/i386: fix build warning (gcc-12 -fsanitize=thread)

[Richard Henderson]

On 8/15/24 08:41, Pierrick Bouvier wrote:
> Found on debian stable.
> 
> ../target/i386/kvm/kvm.c: In function ‘kvm_handle_rdmsr’:
> ../target/i386/kvm/kvm.c:5345:1: error: control reaches end of non-void function [-Werror=return-type]
>   5345 | }
>        | ^
> ../target/i386/kvm/kvm.c: In function ‘kvm_handle_wrmsr’:
> ../target/i386/kvm/kvm.c:5364:1: error: control reaches end of non-void function [-Werror=return-type]
>   5364 | }
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   target/i386/kvm/kvm.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 31f149c9902..ddec27edd5b 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -5770,7 +5770,7 @@ static int kvm_handle_rdmsr(X86CPU *cpu, struct kvm_run *run)
>           }
>       }
>   
> -    assert(false);
> +    g_assert_not_reached();

While a good change, and while I have always hated the assert(false) idiom, I believe this 
points to a compiler bug and might be worth reporting -- assuming a later version of gcc 
still warns.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 3/4] target/s390x: fix build warning (gcc-12 -fsanitize=thread)

[Ilya Leoshkevich]

On Wed, 2024-08-14 at 15:41 -0700, Pierrick Bouvier wrote:
> Found on debian stable.
> 
> ../target/s390x/tcg/translate.c: In function ‘get_mem_index’:
> ../target/s390x/tcg/translate.c:398:1: error: control reaches end of
> non-void function [-Werror=return-type]
>   398 | }
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>

Re: [PATCH v2 2/4] target/i386: fix build warning (gcc-12 -fsanitize=thread)

[Thomas Huth]

On 15/08/2024 00.41, Pierrick Bouvier wrote:
> Found on debian stable.
> 
> ../target/i386/kvm/kvm.c: In function ‘kvm_handle_rdmsr’:
> ../target/i386/kvm/kvm.c:5345:1: error: control reaches end of non-void function [-Werror=return-type]
>   5345 | }
>        | ^
> ../target/i386/kvm/kvm.c: In function ‘kvm_handle_wrmsr’:
> ../target/i386/kvm/kvm.c:5364:1: error: control reaches end of non-void function [-Werror=return-type]
>   5364 | }
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   target/i386/kvm/kvm.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 31f149c9902..ddec27edd5b 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -5770,7 +5770,7 @@ static int kvm_handle_rdmsr(X86CPU *cpu, struct kvm_run *run)
>           }
>       }
>   
> -    assert(false);
> +    g_assert_not_reached();
>   }
>   
>   static int kvm_handle_wrmsr(X86CPU *cpu, struct kvm_run *run)
> @@ -5789,7 +5789,7 @@ static int kvm_handle_wrmsr(X86CPU *cpu, struct kvm_run *run)
>           }
>       }
>   
> -    assert(false);
> +    g_assert_not_reached();
>   }
>   
>   static bool has_sgx_provisioning;

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

Re: [PATCH v2 1/4] meson: hide tsan related warnings

[Thomas Huth]

On 15/08/2024 00.41, Pierrick Bouvier wrote:
> When building with gcc-12 -fsanitize=thread, gcc reports some
> constructions not supported with tsan.
> Found on debian stable.
> 
> qemu/include/qemu/atomic.h:36:52: error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’ [-Werror=tsan]
>     36 | #define smp_mb()                     ({ barrier(); __atomic_thread_fence(__ATOMIC_SEQ_CST); })
>        |                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   meson.build | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index 81ecd4bae7c..52e5aa95cc0 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -499,7 +499,15 @@ if get_option('tsan')
>                            prefix: '#include <sanitizer/tsan_interface.h>')
>       error('Cannot enable TSAN due to missing fiber annotation interface')
>     endif
> -  qemu_cflags = ['-fsanitize=thread'] + qemu_cflags
> +  tsan_warn_suppress = []
> +  # gcc (>=11) will report constructions not supported by tsan:
> +  # "error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’"
> +  # https://gcc.gnu.org/gcc-11/changes.html
> +  # However, clang does not support this warning and this triggers an error.
> +  if cc.has_argument('-Wno-tsan')
> +    tsan_warn_suppress = ['-Wno-tsan']
> +  endif
> +  qemu_cflags = ['-fsanitize=thread'] + tsan_warn_suppress + qemu_cflags
>     qemu_ldflags = ['-fsanitize=thread'] + qemu_ldflags
>   endif
>   

Not sure if we should hide these warnings ... they seem to be there for a 
reason? Paolo, any ideas?

  Thomas

Re: [PATCH v2 1/4] meson: hide tsan related warnings

[Peter Maydell]

On Wed, 14 Aug 2024 at 23:42, Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
>
> When building with gcc-12 -fsanitize=thread, gcc reports some
> constructions not supported with tsan.
> Found on debian stable.
>
> qemu/include/qemu/atomic.h:36:52: error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’ [-Werror=tsan]
>    36 | #define smp_mb()                     ({ barrier(); __atomic_thread_fence(__ATOMIC_SEQ_CST); })
>       |                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>  meson.build | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/meson.build b/meson.build
> index 81ecd4bae7c..52e5aa95cc0 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -499,7 +499,15 @@ if get_option('tsan')
>                           prefix: '#include <sanitizer/tsan_interface.h>')
>      error('Cannot enable TSAN due to missing fiber annotation interface')
>    endif
> -  qemu_cflags = ['-fsanitize=thread'] + qemu_cflags
> +  tsan_warn_suppress = []
> +  # gcc (>=11) will report constructions not supported by tsan:
> +  # "error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’"
> +  # https://gcc.gnu.org/gcc-11/changes.html
> +  # However, clang does not support this warning and this triggers an error.
> +  if cc.has_argument('-Wno-tsan')
> +    tsan_warn_suppress = ['-Wno-tsan']
> +  endif

That last part sounds like a clang bug -- -Wno-foo is supposed
to not be an error on compilers that don't implement -Wfoo for
any value of foo (unless some other warning/error would also
be emitted). At any rate, that's how gcc does it
(see the paragraph "When an unrecognized warning option ..."
in https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html )
and I thought clang did too...

thanks
-- PMM

Re: [PATCH v2 1/4] meson: hide tsan related warnings

[Daniel P. Berrangé]

On Thu, Aug 15, 2024 at 11:12:39AM +0100, Peter Maydell wrote:
> On Wed, 14 Aug 2024 at 23:42, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
> >
> > When building with gcc-12 -fsanitize=thread, gcc reports some
> > constructions not supported with tsan.
> > Found on debian stable.
> >
> > qemu/include/qemu/atomic.h:36:52: error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’ [-Werror=tsan]
> >    36 | #define smp_mb()                     ({ barrier(); __atomic_thread_fence(__ATOMIC_SEQ_CST); })
> >       |                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> > ---
> >  meson.build | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/meson.build b/meson.build
> > index 81ecd4bae7c..52e5aa95cc0 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -499,7 +499,15 @@ if get_option('tsan')
> >                           prefix: '#include <sanitizer/tsan_interface.h>')
> >      error('Cannot enable TSAN due to missing fiber annotation interface')
> >    endif
> > -  qemu_cflags = ['-fsanitize=thread'] + qemu_cflags
> > +  tsan_warn_suppress = []
> > +  # gcc (>=11) will report constructions not supported by tsan:
> > +  # "error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’"
> > +  # https://gcc.gnu.org/gcc-11/changes.html
> > +  # However, clang does not support this warning and this triggers an error.
> > +  if cc.has_argument('-Wno-tsan')
> > +    tsan_warn_suppress = ['-Wno-tsan']
> > +  endif
> 
> That last part sounds like a clang bug -- -Wno-foo is supposed
> to not be an error on compilers that don't implement -Wfoo for
> any value of foo (unless some other warning/error would also
> be emitted).

-Wno-foo isn't an error, but it is a warning... which we then
turn into an error due to -Werror, unless we pass -Wno-unknown-warning-option
to clang.

>               At any rate, that's how gcc does it
> (see the paragraph "When an unrecognized warning option ..."
> in https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html )
> and I thought clang did too...
> 
> thanks
> -- PMM
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 1/4] meson: hide tsan related warnings

[Pierrick Bouvier]

On 8/15/24 04:05, Daniel P. Berrangé wrote:
> On Thu, Aug 15, 2024 at 11:12:39AM +0100, Peter Maydell wrote:
>> On Wed, 14 Aug 2024 at 23:42, Pierrick Bouvier
>> <pierrick.bouvier@linaro.org> wrote:
>>>
>>> When building with gcc-12 -fsanitize=thread, gcc reports some
>>> constructions not supported with tsan.
>>> Found on debian stable.
>>>
>>> qemu/include/qemu/atomic.h:36:52: error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’ [-Werror=tsan]
>>>     36 | #define smp_mb()                     ({ barrier(); __atomic_thread_fence(__ATOMIC_SEQ_CST); })
>>>        |                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>>   meson.build | 10 +++++++++-
>>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index 81ecd4bae7c..52e5aa95cc0 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -499,7 +499,15 @@ if get_option('tsan')
>>>                            prefix: '#include <sanitizer/tsan_interface.h>')
>>>       error('Cannot enable TSAN due to missing fiber annotation interface')
>>>     endif
>>> -  qemu_cflags = ['-fsanitize=thread'] + qemu_cflags
>>> +  tsan_warn_suppress = []
>>> +  # gcc (>=11) will report constructions not supported by tsan:
>>> +  # "error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’"
>>> +  # https://gcc.gnu.org/gcc-11/changes.html
>>> +  # However, clang does not support this warning and this triggers an error.
>>> +  if cc.has_argument('-Wno-tsan')
>>> +    tsan_warn_suppress = ['-Wno-tsan']
>>> +  endif
>>
>> That last part sounds like a clang bug -- -Wno-foo is supposed
>> to not be an error on compilers that don't implement -Wfoo for
>> any value of foo (unless some other warning/error would also
>> be emitted).
> 
> -Wno-foo isn't an error, but it is a warning... which we then
> turn into an error due to -Werror, unless we pass -Wno-unknown-warning-option
> to clang.
> 

Right, it's a consequence.

>>                At any rate, that's how gcc does it
>> (see the paragraph "When an unrecognized warning option ..."
>> in https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html )
>> and I thought clang did too...
>>
>> thanks
>> -- PMM
>>
> 
> With regards,
> Daniel

Re: [PATCH v2 2/4] target/i386: fix build warning (gcc-12 -fsanitize=thread)

[Pierrick Bouvier]

On 8/14/24 15:47, Richard Henderson wrote:
> On 8/15/24 08:41, Pierrick Bouvier wrote:
>> Found on debian stable.
>>
>> ../target/i386/kvm/kvm.c: In function ‘kvm_handle_rdmsr’:
>> ../target/i386/kvm/kvm.c:5345:1: error: control reaches end of non-void function [-Werror=return-type]
>>    5345 | }
>>         | ^
>> ../target/i386/kvm/kvm.c: In function ‘kvm_handle_wrmsr’:
>> ../target/i386/kvm/kvm.c:5364:1: error: control reaches end of non-void function [-Werror=return-type]
>>    5364 | }
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    target/i386/kvm/kvm.c | 4 ++--
>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index 31f149c9902..ddec27edd5b 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -5770,7 +5770,7 @@ static int kvm_handle_rdmsr(X86CPU *cpu, struct kvm_run *run)
>>            }
>>        }
>>    
>> -    assert(false);
>> +    g_assert_not_reached();
> 
> While a good change, and while I have always hated the assert(false) idiom, I believe this
> points to a compiler bug and might be worth reporting -- assuming a later version of gcc
> still warns.
> 

Reported it: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116386

> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
> r~

Re: [PATCH v2 1/4] meson: hide tsan related warnings

[Peter Maydell]

On Thu, 15 Aug 2024 at 12:05, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Aug 15, 2024 at 11:12:39AM +0100, Peter Maydell wrote:
> > On Wed, 14 Aug 2024 at 23:42, Pierrick Bouvier
> > <pierrick.bouvier@linaro.org> wrote:
> > >
> > > When building with gcc-12 -fsanitize=thread, gcc reports some
> > > constructions not supported with tsan.
> > > Found on debian stable.
> > >
> > > qemu/include/qemu/atomic.h:36:52: error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’ [-Werror=tsan]
> > >    36 | #define smp_mb()                     ({ barrier(); __atomic_thread_fence(__ATOMIC_SEQ_CST); })
> > >       |                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> > > ---
> > >  meson.build | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/meson.build b/meson.build
> > > index 81ecd4bae7c..52e5aa95cc0 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -499,7 +499,15 @@ if get_option('tsan')
> > >                           prefix: '#include <sanitizer/tsan_interface.h>')
> > >      error('Cannot enable TSAN due to missing fiber annotation interface')
> > >    endif
> > > -  qemu_cflags = ['-fsanitize=thread'] + qemu_cflags
> > > +  tsan_warn_suppress = []
> > > +  # gcc (>=11) will report constructions not supported by tsan:
> > > +  # "error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’"
> > > +  # https://gcc.gnu.org/gcc-11/changes.html
> > > +  # However, clang does not support this warning and this triggers an error.
> > > +  if cc.has_argument('-Wno-tsan')
> > > +    tsan_warn_suppress = ['-Wno-tsan']
> > > +  endif
> >
> > That last part sounds like a clang bug -- -Wno-foo is supposed
> > to not be an error on compilers that don't implement -Wfoo for
> > any value of foo (unless some other warning/error would also
> > be emitted).
>
> -Wno-foo isn't an error, but it is a warning... which we then
> turn into an error due to -Werror, unless we pass -Wno-unknown-warning-option
> to clang.

Which is irritating if you want to be able to blanket say
'-Wno-silly-compiler-warning' and not see any of that
warning regardless of compiler version. That's why the
gcc behaviour is the way it is (i.e. -Wno-such-thingy
is neither a warning nor an error if it would be the only
warning/error), and if clang doesn't match it that's a shame.

thanks
-- PMM

Re: [PATCH v2 1/4] meson: hide tsan related warnings

[Pierrick Bouvier]

On 8/15/24 02:50, Thomas Huth wrote:
> On 15/08/2024 00.41, Pierrick Bouvier wrote:
>> When building with gcc-12 -fsanitize=thread, gcc reports some
>> constructions not supported with tsan.
>> Found on debian stable.
>>
>> qemu/include/qemu/atomic.h:36:52: error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’ [-Werror=tsan]
>>      36 | #define smp_mb()                     ({ barrier(); __atomic_thread_fence(__ATOMIC_SEQ_CST); })
>>         |                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    meson.build | 10 +++++++++-
>>    1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/meson.build b/meson.build
>> index 81ecd4bae7c..52e5aa95cc0 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -499,7 +499,15 @@ if get_option('tsan')
>>                             prefix: '#include <sanitizer/tsan_interface.h>')
>>        error('Cannot enable TSAN due to missing fiber annotation interface')
>>      endif
>> -  qemu_cflags = ['-fsanitize=thread'] + qemu_cflags
>> +  tsan_warn_suppress = []
>> +  # gcc (>=11) will report constructions not supported by tsan:
>> +  # "error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’"
>> +  # https://gcc.gnu.org/gcc-11/changes.html
>> +  # However, clang does not support this warning and this triggers an error.
>> +  if cc.has_argument('-Wno-tsan')
>> +    tsan_warn_suppress = ['-Wno-tsan']
>> +  endif
>> +  qemu_cflags = ['-fsanitize=thread'] + tsan_warn_suppress + qemu_cflags
>>      qemu_ldflags = ['-fsanitize=thread'] + qemu_ldflags
>>    endif
>>    
> 
> Not sure if we should hide these warnings ... they seem to be there for a
> reason? Paolo, any ideas?
> 

This is a new warning added in gcc-11, to prevent that not all 
constructions are supported by thread sanitizer. This was true before, 
and will be true after. Basically, manual memory barriers are not 
supported to model concurrency. We can hardly change something on QEMU 
side as it's a limitation of tsan itself.

Good news is that it does not seem to bring false positives when 
executing a qemu with tsan. Thus, this patch to ignore this for now.

>    Thomas
> 

Re: [PATCH v2 1/4] meson: hide tsan related warnings

[Pierrick Bouvier]

On 8/15/24 10:54, Peter Maydell wrote:
> On Thu, 15 Aug 2024 at 12:05, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>
>> On Thu, Aug 15, 2024 at 11:12:39AM +0100, Peter Maydell wrote:
>>> On Wed, 14 Aug 2024 at 23:42, Pierrick Bouvier
>>> <pierrick.bouvier@linaro.org> wrote:
>>>>
>>>> When building with gcc-12 -fsanitize=thread, gcc reports some
>>>> constructions not supported with tsan.
>>>> Found on debian stable.
>>>>
>>>> qemu/include/qemu/atomic.h:36:52: error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’ [-Werror=tsan]
>>>>     36 | #define smp_mb()                     ({ barrier(); __atomic_thread_fence(__ATOMIC_SEQ_CST); })
>>>>        |                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>> ---
>>>>   meson.build | 10 +++++++++-
>>>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/meson.build b/meson.build
>>>> index 81ecd4bae7c..52e5aa95cc0 100644
>>>> --- a/meson.build
>>>> +++ b/meson.build
>>>> @@ -499,7 +499,15 @@ if get_option('tsan')
>>>>                            prefix: '#include <sanitizer/tsan_interface.h>')
>>>>       error('Cannot enable TSAN due to missing fiber annotation interface')
>>>>     endif
>>>> -  qemu_cflags = ['-fsanitize=thread'] + qemu_cflags
>>>> +  tsan_warn_suppress = []
>>>> +  # gcc (>=11) will report constructions not supported by tsan:
>>>> +  # "error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’"
>>>> +  # https://gcc.gnu.org/gcc-11/changes.html
>>>> +  # However, clang does not support this warning and this triggers an error.
>>>> +  if cc.has_argument('-Wno-tsan')
>>>> +    tsan_warn_suppress = ['-Wno-tsan']
>>>> +  endif
>>>
>>> That last part sounds like a clang bug -- -Wno-foo is supposed
>>> to not be an error on compilers that don't implement -Wfoo for
>>> any value of foo (unless some other warning/error would also
>>> be emitted).
>>
>> -Wno-foo isn't an error, but it is a warning... which we then
>> turn into an error due to -Werror, unless we pass -Wno-unknown-warning-option
>> to clang.
> 
> Which is irritating if you want to be able to blanket say
> '-Wno-silly-compiler-warning' and not see any of that
> warning regardless of compiler version. That's why the
> gcc behaviour is the way it is (i.e. -Wno-such-thingy
> is neither a warning nor an error if it would be the only
> warning/error), and if clang doesn't match it that's a shame.
> 

It's why I chose to implement this using cc.has_argument(), instead of 
trying to identify compiler/version.

> thanks
> -- PMM

Re: [PATCH v2 1/4] meson: hide tsan related warnings

[Thomas Huth]

On 15/08/2024 19.54, Peter Maydell wrote:
> On Thu, 15 Aug 2024 at 12:05, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>
>> On Thu, Aug 15, 2024 at 11:12:39AM +0100, Peter Maydell wrote:
>>> On Wed, 14 Aug 2024 at 23:42, Pierrick Bouvier
>>> <pierrick.bouvier@linaro.org> wrote:
>>>>
>>>> When building with gcc-12 -fsanitize=thread, gcc reports some
>>>> constructions not supported with tsan.
>>>> Found on debian stable.
>>>>
>>>> qemu/include/qemu/atomic.h:36:52: error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’ [-Werror=tsan]
>>>>     36 | #define smp_mb()                     ({ barrier(); __atomic_thread_fence(__ATOMIC_SEQ_CST); })
>>>>        |                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>> ---
>>>>   meson.build | 10 +++++++++-
>>>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/meson.build b/meson.build
>>>> index 81ecd4bae7c..52e5aa95cc0 100644
>>>> --- a/meson.build
>>>> +++ b/meson.build
>>>> @@ -499,7 +499,15 @@ if get_option('tsan')
>>>>                            prefix: '#include <sanitizer/tsan_interface.h>')
>>>>       error('Cannot enable TSAN due to missing fiber annotation interface')
>>>>     endif
>>>> -  qemu_cflags = ['-fsanitize=thread'] + qemu_cflags
>>>> +  tsan_warn_suppress = []
>>>> +  # gcc (>=11) will report constructions not supported by tsan:
>>>> +  # "error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’"
>>>> +  # https://gcc.gnu.org/gcc-11/changes.html
>>>> +  # However, clang does not support this warning and this triggers an error.
>>>> +  if cc.has_argument('-Wno-tsan')
>>>> +    tsan_warn_suppress = ['-Wno-tsan']
>>>> +  endif
>>>
>>> That last part sounds like a clang bug -- -Wno-foo is supposed
>>> to not be an error on compilers that don't implement -Wfoo for
>>> any value of foo (unless some other warning/error would also
>>> be emitted).
>>
>> -Wno-foo isn't an error, but it is a warning... which we then
>> turn into an error due to -Werror, unless we pass -Wno-unknown-warning-option
>> to clang.
> 
> Which is irritating if you want to be able to blanket say
> '-Wno-silly-compiler-warning' and not see any of that
> warning regardless of compiler version. That's why the
> gcc behaviour is the way it is (i.e. -Wno-such-thingy
> is neither a warning nor an error if it would be the only
> warning/error), and if clang doesn't match it that's a shame.

I thought that Clang would behave the same way as GCC, but apparently it 
does not (anymore?):

$ gcc -Wno-flux-capacitors testprg.c -o testprg
$ clang -Wno-flux-capacitors testprg.c -o testprg
warning: unknown warning option '-Wno-flux-capacitors' 
[-Wunknown-warning-option]
1 warning generated.

  Thomas

Re: [PATCH v2 1/4] meson: hide tsan related warnings

[Daniel P. Berrangé]

On Fri, Aug 16, 2024 at 07:44:28AM +0200, Thomas Huth wrote:
> On 15/08/2024 19.54, Peter Maydell wrote:
> > On Thu, 15 Aug 2024 at 12:05, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > 
> > > On Thu, Aug 15, 2024 at 11:12:39AM +0100, Peter Maydell wrote:
> > > > On Wed, 14 Aug 2024 at 23:42, Pierrick Bouvier
> > > > <pierrick.bouvier@linaro.org> wrote:
> > > > > 
> > > > > When building with gcc-12 -fsanitize=thread, gcc reports some
> > > > > constructions not supported with tsan.
> > > > > Found on debian stable.
> > > > > 
> > > > > qemu/include/qemu/atomic.h:36:52: error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’ [-Werror=tsan]
> > > > >     36 | #define smp_mb()                     ({ barrier(); __atomic_thread_fence(__ATOMIC_SEQ_CST); })
> > > > >        |                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > 
> > > > > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> > > > > ---
> > > > >   meson.build | 10 +++++++++-
> > > > >   1 file changed, 9 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/meson.build b/meson.build
> > > > > index 81ecd4bae7c..52e5aa95cc0 100644
> > > > > --- a/meson.build
> > > > > +++ b/meson.build
> > > > > @@ -499,7 +499,15 @@ if get_option('tsan')
> > > > >                            prefix: '#include <sanitizer/tsan_interface.h>')
> > > > >       error('Cannot enable TSAN due to missing fiber annotation interface')
> > > > >     endif
> > > > > -  qemu_cflags = ['-fsanitize=thread'] + qemu_cflags
> > > > > +  tsan_warn_suppress = []
> > > > > +  # gcc (>=11) will report constructions not supported by tsan:
> > > > > +  # "error: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’"
> > > > > +  # https://gcc.gnu.org/gcc-11/changes.html
> > > > > +  # However, clang does not support this warning and this triggers an error.
> > > > > +  if cc.has_argument('-Wno-tsan')
> > > > > +    tsan_warn_suppress = ['-Wno-tsan']
> > > > > +  endif
> > > > 
> > > > That last part sounds like a clang bug -- -Wno-foo is supposed
> > > > to not be an error on compilers that don't implement -Wfoo for
> > > > any value of foo (unless some other warning/error would also
> > > > be emitted).
> > > 
> > > -Wno-foo isn't an error, but it is a warning... which we then
> > > turn into an error due to -Werror, unless we pass -Wno-unknown-warning-option
> > > to clang.
> > 
> > Which is irritating if you want to be able to blanket say
> > '-Wno-silly-compiler-warning' and not see any of that
> > warning regardless of compiler version. That's why the
> > gcc behaviour is the way it is (i.e. -Wno-such-thingy
> > is neither a warning nor an error if it would be the only
> > warning/error), and if clang doesn't match it that's a shame.
> 
> I thought that Clang would behave the same way as GCC, but apparently it
> does not (anymore?):

It is nothing new - clang has behaved this way wrt unknown warning flags
for as long as I remember.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 2/4] target/i386: fix build warning (gcc-12 -fsanitize=thread)

[Philippe Mathieu-Daudé]

On 15/8/24 00:41, Pierrick Bouvier wrote:
> Found on debian stable.
> 
> ../target/i386/kvm/kvm.c: In function ‘kvm_handle_rdmsr’:
> ../target/i386/kvm/kvm.c:5345:1: error: control reaches end of non-void function [-Werror=return-type]
>   5345 | }
>        | ^
> ../target/i386/kvm/kvm.c: In function ‘kvm_handle_wrmsr’:
> ../target/i386/kvm/kvm.c:5364:1: error: control reaches end of non-void function [-Werror=return-type]
>   5364 | }
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   target/i386/kvm/kvm.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

But what about the other cases?

$ git grep 'assert(false)'
block/qcow2.c:5302:        assert(false);
hw/hyperv/hyperv_testdev.c:91:    assert(false);
hw/hyperv/hyperv_testdev.c:190:    assert(false);
hw/hyperv/hyperv_testdev.c:240:    assert(false);
hw/hyperv/vmbus.c:1877:    assert(false);
hw/hyperv/vmbus.c:1892:    assert(false);
hw/hyperv/vmbus.c:1934:    assert(false);
hw/hyperv/vmbus.c:1949:    assert(false);
hw/hyperv/vmbus.c:1999:    assert(false);
hw/hyperv/vmbus.c:2023:    assert(false);
hw/net/e1000e_core.c:564:        assert(false);
hw/net/igb_core.c:400:        assert(false);
hw/net/net_rx_pkt.c:378:        assert(false);
hw/nvme/ctrl.c:1819:        assert(false);
hw/nvme/ctrl.c:1873:        assert(false);
hw/nvme/ctrl.c:4657:        assert(false);
hw/nvme/ctrl.c:7208:        assert(false);
hw/pci/pci-stub.c:49:    g_assert(false);
hw/pci/pci-stub.c:55:    g_assert(false);
hw/ppc/spapr_events.c:648:        g_assert(false);
include/hw/s390x/cpu-topology.h:60:    assert(false);
include/qemu/osdep.h:240: * assert(false) as unused.  We rely on this 
within the code base to delete
migration/dirtyrate.c:231:        assert(false); /* unreachable */
target/i386/kvm/kvm.c:5773:    assert(false);
target/i386/kvm/kvm.c:5792:    assert(false);

Re: [PATCH v2 2/4] target/i386: fix build warning (gcc-12 -fsanitize=thread)

[Pierrick Bouvier]

On 8/16/24 03:59, Philippe Mathieu-Daudé wrote:
> On 15/8/24 00:41, Pierrick Bouvier wrote:
>> Found on debian stable.
>>
>> ../target/i386/kvm/kvm.c: In function ‘kvm_handle_rdmsr’:
>> ../target/i386/kvm/kvm.c:5345:1: error: control reaches end of non-void function [-Werror=return-type]
>>    5345 | }
>>         | ^
>> ../target/i386/kvm/kvm.c: In function ‘kvm_handle_wrmsr’:
>> ../target/i386/kvm/kvm.c:5364:1: error: control reaches end of non-void function [-Werror=return-type]
>>    5364 | }
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    target/i386/kvm/kvm.c | 4 ++--
>>    1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> But what about the other cases?
> 
> $ git grep 'assert(false)'
> block/qcow2.c:5302:        assert(false);
> hw/hyperv/hyperv_testdev.c:91:    assert(false);
> hw/hyperv/hyperv_testdev.c:190:    assert(false);
> hw/hyperv/hyperv_testdev.c:240:    assert(false);
> hw/hyperv/vmbus.c:1877:    assert(false);
> hw/hyperv/vmbus.c:1892:    assert(false);
> hw/hyperv/vmbus.c:1934:    assert(false);
> hw/hyperv/vmbus.c:1949:    assert(false);
> hw/hyperv/vmbus.c:1999:    assert(false);
> hw/hyperv/vmbus.c:2023:    assert(false);
> hw/net/e1000e_core.c:564:        assert(false);
> hw/net/igb_core.c:400:        assert(false);
> hw/net/net_rx_pkt.c:378:        assert(false);
> hw/nvme/ctrl.c:1819:        assert(false);
> hw/nvme/ctrl.c:1873:        assert(false);
> hw/nvme/ctrl.c:4657:        assert(false);
> hw/nvme/ctrl.c:7208:        assert(false);
> hw/pci/pci-stub.c:49:    g_assert(false);
> hw/pci/pci-stub.c:55:    g_assert(false);
> hw/ppc/spapr_events.c:648:        g_assert(false);
> include/hw/s390x/cpu-topology.h:60:    assert(false);
> include/qemu/osdep.h:240: * assert(false) as unused.  We rely on this
> within the code base to delete
> migration/dirtyrate.c:231:        assert(false); /* unreachable */
> target/i386/kvm/kvm.c:5773:    assert(false);
> target/i386/kvm/kvm.c:5792:    assert(false);
> 

They don't seem to be a problem, but I'll do a series to clean this 
completely from the code base, so assert(false) is eradicated.