[PATCH v3 0/3] build qemu with gcc and tsan

[PATCH v3 0/3] 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.

v3
- rebased on top of master
- previous conversation shifted on why clang does not implement some warnings
- hopefully we can review the content of patches this time

v2
- forgot to signoff commits

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

 docs/devel/testing/main.rst | 26 ++++++++++++++++++++++----
 meson.build                 | 10 +++++++++-
 target/i386/kvm/kvm.c       |  4 ++--
 3 files changed, 33 insertions(+), 7 deletions(-)

-- 
2.39.2

[PATCH v3 1/3] 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 fbda17c987e..8a587bd52cc 100644
--- a/meson.build
+++ b/meson.build
@@ -504,7 +504,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 v3 2/3] 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 | }

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
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 2fa88ef1e37..308b0e1cb37 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 v3 3/3] 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/main.rst | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/docs/devel/testing/main.rst b/docs/devel/testing/main.rst
index e9921a4b10a..2e57390d990 100644
--- a/docs/devel/testing/main.rst
+++ b/docs/devel/testing/main.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 v3 0/3] build qemu with gcc and tsan

[Thomas Huth]

On 10/09/2024 19.40, Pierrick Bouvier wrote:
> 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.
> 
> v3
> - rebased on top of master
> - previous conversation shifted on why clang does not implement some warnings
> - hopefully we can review the content of patches this time
> 
> v2
> - forgot to signoff commits
> 
> Pierrick Bouvier (3):
>    meson: hide tsan related warnings
>    target/i386: fix build warning (gcc-12 -fsanitize=thread)
>    docs/devel: update tsan build documentation
> 
>   docs/devel/testing/main.rst | 26 ++++++++++++++++++++++----
>   meson.build                 | 10 +++++++++-
>   target/i386/kvm/kvm.c       |  4 ++--
>   3 files changed, 33 insertions(+), 7 deletions(-)
> 

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

Re: [PATCH v3 0/3] build qemu with gcc and tsan

[Alex Bennée]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> 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.

Queued to testing/next, thanks.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 0/3] build qemu with gcc and tsan

[Pierrick Bouvier]

On 9/25/24 03:43, Alex Bennée wrote:
> Alex Bennée <alex.bennee@linaro.org> writes:
> 
> Queued to testing/next, thanks.
> 

Gentle ping. I can't see this series on testing/next. Was it lost on the 
way?

Thanks,
Pierrick