[Qemu-devel] [RFC PATCH 0/4] ThreadSanitizer support

[Qemu-devel] [RFC PATCH 0/4] ThreadSanitizer support

[Alex Bennée]

This is mainly motivated at being able to use the ThreadSanitizer to
give the MTTCG work a good testing for corner cases. However this
should be generally useful.

The first two patches are simple configure tweaks to make the passing
of flags sane. To build on my machine I installed the latest/greatest
GCC PPA and configured with:

    ./configure ${TARGET_LIST} --cc=gcc-5 --cxx=g++-5 \
  --extra-cflags="-fsanitize=thread" --extra-libs="-ltsan" \
  --with-coroutine=gthread

The compiler selection gives the new GCC and I pass the -fsanitize
option in the cflags. I've introduced --extra-libs so we can link in
the ThreadSanitizer support library.

The third patch ensure we use the __atomic builtins for all atomic
functions if we have it available. This is needed so the
ThreadSanitizer can properly instrument the code but we should use it
anyway for consistency. The code generation for atomic accesses on x86
it unaffected.

Finally there is a patch with a few of the fixes for bugs thrown up
running make check. It doesn't fix them all as there are still some
warnings I need to look into more deeply.


Alex Bennée (4):
  configure: move EXTRA_CFLAGS append to the end
  configure: introduce --extra-libs
  include/qemu/atomic.h: default to __atomic functions
  tsan: various fixes for make check

 async.c                  |   4 +-
 configure                |  18 ++++++-
 include/qemu/atomic.h    | 126 ++++++++++++++++++++++++++++++-----------------
 tests/test-thread-pool.c |   2 +-
 thread-pool.c            |   9 ++--
 5 files changed, 105 insertions(+), 54 deletions(-)

-- 
2.7.0

[Qemu-devel] [RFC PATCH 1/4] configure: move EXTRA_CFLAGS append to the end

[Alex Bennée]

When using --extra-cflags to override defaults the flags need to be set
at the end lest they be reset by later options. This affects
optimisation as well where "-O0 .. -O3" will just takes the most recent
option.

We also set CFLAGS so the options are passed to other built binaries.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 configure | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 44ac9ab..7d23c6c 100755
--- a/configure
+++ b/configure
@@ -360,8 +360,7 @@ for opt do
   ;;
   --cpu=*) cpu="$optarg"
   ;;
-  --extra-cflags=*) QEMU_CFLAGS="$QEMU_CFLAGS $optarg"
-                    EXTRA_CFLAGS="$optarg"
+  --extra-cflags=*) EXTRA_CFLAGS="$optarg"
   ;;
   --extra-ldflags=*) LDFLAGS="$LDFLAGS $optarg"
                      EXTRA_LDFLAGS="$optarg"
@@ -4715,6 +4714,10 @@ fi
 QEMU_CFLAGS="$pixman_cflags $fdt_cflags $QEMU_CFLAGS"
 libs_softmmu="$pixman_libs $libs_softmmu"
 
+# Now it the time to append extra-cflags
+CFLAGS="$CFLAGS $EXTRA_CFLAGS"
+QEMU_CFLAGS="$QEMU_CFLAGS $EXTRA_CFLAGS"
+
 echo "Install prefix    $prefix"
 echo "BIOS directory    `eval echo $qemu_datadir`"
 echo "binary directory  `eval echo $bindir`"
-- 
2.7.0

Re: [Qemu-devel] [RFC PATCH 1/4] configure: move EXTRA_CFLAGS append to the end

[Peter Maydell]

On 25 January 2016 at 16:49, Alex Bennée <alex.bennee@linaro.org> wrote:
> When using --extra-cflags to override defaults the flags need to be set
> at the end lest they be reset by later options. This affects
> optimisation as well where "-O0 .. -O3" will just takes the most recent
> option.
>
> We also set CFLAGS so the options are passed to other built binaries.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  configure | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/configure b/configure
> index 44ac9ab..7d23c6c 100755
> --- a/configure
> +++ b/configure
> @@ -360,8 +360,7 @@ for opt do
>    ;;
>    --cpu=*) cpu="$optarg"
>    ;;
> -  --extra-cflags=*) QEMU_CFLAGS="$QEMU_CFLAGS $optarg"
> -                    EXTRA_CFLAGS="$optarg"
> +  --extra-cflags=*) EXTRA_CFLAGS="$optarg"
>    ;;
>    --extra-ldflags=*) LDFLAGS="$LDFLAGS $optarg"
>                       EXTRA_LDFLAGS="$optarg"
> @@ -4715,6 +4714,10 @@ fi
>  QEMU_CFLAGS="$pixman_cflags $fdt_cflags $QEMU_CFLAGS"
>  libs_softmmu="$pixman_libs $libs_softmmu"
>
> +# Now it the time to append extra-cflags
> +CFLAGS="$CFLAGS $EXTRA_CFLAGS"

We don't in general use plain CFLAGS. What breaks without this?
(For instance, when we build pixman we set its CFLAGS to include
extra_cflags in the makefile rune.)

> +QEMU_CFLAGS="$QEMU_CFLAGS $EXTRA_CFLAGS"
> +
>  echo "Install prefix    $prefix"
>  echo "BIOS directory    `eval echo $qemu_datadir`"
>  echo "binary directory  `eval echo $bindir`"

I think this will break the use case where you were using --extra-cflags
to pass flags that are needed for the test code in configure to build
correctly.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 1/4] configure: move EXTRA_CFLAGS append to the end

[Peter Maydell]

On 25 January 2016 at 16:49, Alex Bennée <alex.bennee@linaro.org> wrote:
> When using --extra-cflags to override defaults the flags need to be set
> at the end lest they be reset by later options. This affects
> optimisation as well where "-O0 .. -O3" will just takes the most recent
> option.
>
> We also set CFLAGS so the options are passed to other built binaries.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

By the way, is this fixing a regression since your commit de3852877f1e,
or did that not fully fix the issue it claimed to?

thanks
-- PMM

[Qemu-devel] [RFC PATCH 2/4] configure: introduce --extra-libs

[Alex Bennée]

If for example you want to use the thread sanitizer you want to ensure all
binaries are linked with the library:

  ./configure ${TARGETS} --cc=gcc-5 --cxx=g++-5 \
    --extra-cflags="-fsanitize=thread" --extra-libs="-ltsan"

This is more explicit than just specifying --extra-ldflags which might
not get applied in the right place all the time.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 configure | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/configure b/configure
index 7d23c6c..194bae9 100755
--- a/configure
+++ b/configure
@@ -365,6 +365,8 @@ for opt do
   --extra-ldflags=*) LDFLAGS="$LDFLAGS $optarg"
                      EXTRA_LDFLAGS="$optarg"
   ;;
+  --extra-libs=*)    EXTRA_LIBS="$optarg"
+  ;;
   --enable-debug-info) debug_info="yes"
   ;;
   --disable-debug-info) debug_info="no"
@@ -785,6 +787,8 @@ for opt do
   ;;
   --extra-ldflags=*)
   ;;
+  --extra-libs=*)
+  ;;
   --enable-debug-info)
   ;;
   --disable-debug-info)
@@ -1281,6 +1285,7 @@ Advanced options (experts only):
   --objcc=OBJCC            use Objective-C compiler OBJCC [$objcc]
   --extra-cflags=CFLAGS    append extra C compiler flags QEMU_CFLAGS
   --extra-ldflags=LDFLAGS  append extra linker flags LDFLAGS
+  --extra-libs=LIBS        append extra libraries when linking
   --make=MAKE              use specified make [$make]
   --install=INSTALL        use specified install [$install]
   --python=PYTHON          use specified python [$python]
@@ -4718,6 +4723,11 @@ libs_softmmu="$pixman_libs $libs_softmmu"
 CFLAGS="$CFLAGS $EXTRA_CFLAGS"
 QEMU_CFLAGS="$QEMU_CFLAGS $EXTRA_CFLAGS"
 
+# extra-libs
+LIBS="$LIBS $EXTRA_LIBS"
+libs_softmmu="$libs_softmmu $EXTRA_LIBS"
+libs_qga="$libs_qga $EXTRA_LIBS"
+
 echo "Install prefix    $prefix"
 echo "BIOS directory    `eval echo $qemu_datadir`"
 echo "binary directory  `eval echo $bindir`"
@@ -4888,6 +4898,7 @@ fi
 echo "qemu_helperdir=$libexecdir" >> $config_host_mak
 echo "extra_cflags=$EXTRA_CFLAGS" >> $config_host_mak
 echo "extra_ldflags=$EXTRA_LDFLAGS" >> $config_host_mak
+echo "extra_libs=$EXTRA_LIBS" >> $config_host_mak
 echo "qemu_localedir=$qemu_localedir" >> $config_host_mak
 echo "libs_softmmu=$libs_softmmu" >> $config_host_mak
 
-- 
2.7.0

Re: [Qemu-devel] [RFC PATCH 2/4] configure: introduce --extra-libs

[Peter Maydell]

On 25 January 2016 at 16:49, Alex Bennée <alex.bennee@linaro.org> wrote:
> If for example you want to use the thread sanitizer you want to ensure all
> binaries are linked with the library:
>
>   ./configure ${TARGETS} --cc=gcc-5 --cxx=g++-5 \
>     --extra-cflags="-fsanitize=thread" --extra-libs="-ltsan"
>
> This is more explicit than just specifying --extra-ldflags which might
> not get applied in the right place all the time.

When would they differ? The commit message makes this sound like
it's working around a bug in the code that uses LDFLAGS...

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  configure | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/configure b/configure
> index 7d23c6c..194bae9 100755
> --- a/configure
> +++ b/configure
> @@ -365,6 +365,8 @@ for opt do
>    --extra-ldflags=*) LDFLAGS="$LDFLAGS $optarg"
>                       EXTRA_LDFLAGS="$optarg"
>    ;;
> +  --extra-libs=*)    EXTRA_LIBS="$optarg"
> +  ;;
>    --enable-debug-info) debug_info="yes"
>    ;;
>    --disable-debug-info) debug_info="no"

The reason for having this special case to set the variables for
CC related things early is to ensure that they apply for all
compilations including any that get done very early in configure...

> @@ -785,6 +787,8 @@ for opt do
>    ;;
>    --extra-ldflags=*)
>    ;;
> +  --extra-libs=*)
> +  ;;
>    --enable-debug-info)
>    ;;
>    --disable-debug-info)
> @@ -1281,6 +1285,7 @@ Advanced options (experts only):
>    --objcc=OBJCC            use Objective-C compiler OBJCC [$objcc]
>    --extra-cflags=CFLAGS    append extra C compiler flags QEMU_CFLAGS
>    --extra-ldflags=LDFLAGS  append extra linker flags LDFLAGS
> +  --extra-libs=LIBS        append extra libraries when linking
>    --make=MAKE              use specified make [$make]
>    --install=INSTALL        use specified install [$install]
>    --python=PYTHON          use specified python [$python]
> @@ -4718,6 +4723,11 @@ libs_softmmu="$pixman_libs $libs_softmmu"
>  CFLAGS="$CFLAGS $EXTRA_CFLAGS"
>  QEMU_CFLAGS="$QEMU_CFLAGS $EXTRA_CFLAGS"
>
> +# extra-libs
> +LIBS="$LIBS $EXTRA_LIBS"

...but if you don't set LIBS until way down here there's no
point in defining EXTRA_LIBS early.

We should handle all the compiler-option related flags the
same way, I think, for consistency.

> +libs_softmmu="$libs_softmmu $EXTRA_LIBS"
> +libs_qga="$libs_qga $EXTRA_LIBS"
> +
>  echo "Install prefix    $prefix"
>  echo "BIOS directory    `eval echo $qemu_datadir`"
>  echo "binary directory  `eval echo $bindir`"
> @@ -4888,6 +4898,7 @@ fi
>  echo "qemu_helperdir=$libexecdir" >> $config_host_mak
>  echo "extra_cflags=$EXTRA_CFLAGS" >> $config_host_mak
>  echo "extra_ldflags=$EXTRA_LDFLAGS" >> $config_host_mak
> +echo "extra_libs=$EXTRA_LIBS" >> $config_host_mak
>  echo "qemu_localedir=$qemu_localedir" >> $config_host_mak
>  echo "libs_softmmu=$libs_softmmu" >> $config_host_mak

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 2/4] configure: introduce --extra-libs

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 25 January 2016 at 16:49, Alex Bennée <alex.bennee@linaro.org> wrote:
>> If for example you want to use the thread sanitizer you want to ensure all
>> binaries are linked with the library:
>>
>>   ./configure ${TARGETS} --cc=gcc-5 --cxx=g++-5 \
>>     --extra-cflags="-fsanitize=thread" --extra-libs="-ltsan"
>>
>> This is more explicit than just specifying --extra-ldflags which might
>> not get applied in the right place all the time.
>
> When would they differ? The commit message makes this sound like
> it's working around a bug in the code that uses LDFLAGS...

Well LDFLAGS doesn't get applied everywhere so with:

--cc=gcc-5 --cxx=g++-5 --extra-cflags="-fsanitize=thread"
  --extra-ldflags="-ltsan" --with-coroutine=gthread

You get compile failures in the ancillary binaries that are used during
testing.

  LINK  tests/qemu-iotests/socket_scm_helper
tests/qemu-iotests/socket_scm_helper.o: In function `main':
/home/alex/lsrc/qemu/qemu.git/tests/qemu-iotests/socket_scm_helper.c:95: undefined reference to `__tsan_func_entry'
/home/alex/lsrc/qemu/qemu.git/tests/qemu-iotests/socket_scm_helper.c:106: undefined reference to `__tsan_read8'
/home/alex/lsrc/qemu/qemu.git/tests/qemu-iotests/socket_scm_helper.c:106: undefined reference to `__tsan_read8'
...

I think this stems from my confusion from exactly which binaries are
meant to be affected by which flags. QEMU_CFLAGS seems to imply it's
QEMU only, but we don't have a QEMU_LDFLAGS so should LDFLAGS be shared
with all binaries we build?

--
Alex Bennée

Re: [Qemu-devel] [RFC PATCH 2/4] configure: introduce --extra-libs

[Peter Maydell]

On 25 January 2016 at 17:25, Alex Bennée <alex.bennee@linaro.org> wrote:
> Well LDFLAGS doesn't get applied everywhere so with:
>
> --cc=gcc-5 --cxx=g++-5 --extra-cflags="-fsanitize=thread"
>   --extra-ldflags="-ltsan" --with-coroutine=gthread
>
> You get compile failures in the ancillary binaries that are used during
> testing.
>
>   LINK  tests/qemu-iotests/socket_scm_helper
> tests/qemu-iotests/socket_scm_helper.o: In function `main':
> /home/alex/lsrc/qemu/qemu.git/tests/qemu-iotests/socket_scm_helper.c:95: undefined reference to `__tsan_func_entry'
> /home/alex/lsrc/qemu/qemu.git/tests/qemu-iotests/socket_scm_helper.c:106: undefined reference to `__tsan_read8'
> /home/alex/lsrc/qemu/qemu.git/tests/qemu-iotests/socket_scm_helper.c:106: undefined reference to `__tsan_read8'
> ...

That seems like a bug -- we should be applying LDFLAGS there
I think. (Consider that we put things like -g and -m32 there.)

> I think this stems from my confusion from exactly which binaries are
> meant to be affected by which flags. QEMU_CFLAGS seems to imply it's
> QEMU only, but we don't have a QEMU_LDFLAGS so should LDFLAGS be shared
> with all binaries we build?

Back in 2012 and commit caa50971f2e the distinction was apparently
that QEMU_CFLAGS was for flags without which QEMU can't compile,
whereas CFLAGS was for flags like "-g -O2" which the user can
safely override. This may even still be true :-)

Given that I suspect the reason we don't have a QEMU_LDFLAGS is
that users are less in the habit of trying to run make with a
hand-specified CFLAGS.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 2/4] configure: introduce --extra-libs

[Paolo Bonzini]

On 25/01/2016 17:49, Alex Bennée wrote:
> If for example you want to use the thread sanitizer you want to ensure all
> binaries are linked with the library:
> 
>   ./configure ${TARGETS} --cc=gcc-5 --cxx=g++-5 \
>     --extra-cflags="-fsanitize=thread" --extra-libs="-ltsan"

Shouldn't -fsanitize=thread work as a linker command line flag too?
Perhaps if that works there's a better place to put -fsanitizer flags.

Paolo

> This is more explicit than just specifying --extra-ldflags which might
> not get applied in the right place all the time.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  configure | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/configure b/configure
> index 7d23c6c..194bae9 100755
> --- a/configure
> +++ b/configure
> @@ -365,6 +365,8 @@ for opt do
>    --extra-ldflags=*) LDFLAGS="$LDFLAGS $optarg"
>                       EXTRA_LDFLAGS="$optarg"
>    ;;
> +  --extra-libs=*)    EXTRA_LIBS="$optarg"
> +  ;;
>    --enable-debug-info) debug_info="yes"
>    ;;
>    --disable-debug-info) debug_info="no"
> @@ -785,6 +787,8 @@ for opt do
>    ;;
>    --extra-ldflags=*)
>    ;;
> +  --extra-libs=*)
> +  ;;
>    --enable-debug-info)
>    ;;
>    --disable-debug-info)
> @@ -1281,6 +1285,7 @@ Advanced options (experts only):
>    --objcc=OBJCC            use Objective-C compiler OBJCC [$objcc]
>    --extra-cflags=CFLAGS    append extra C compiler flags QEMU_CFLAGS
>    --extra-ldflags=LDFLAGS  append extra linker flags LDFLAGS
> +  --extra-libs=LIBS        append extra libraries when linking
>    --make=MAKE              use specified make [$make]
>    --install=INSTALL        use specified install [$install]
>    --python=PYTHON          use specified python [$python]
> @@ -4718,6 +4723,11 @@ libs_softmmu="$pixman_libs $libs_softmmu"
>  CFLAGS="$CFLAGS $EXTRA_CFLAGS"
>  QEMU_CFLAGS="$QEMU_CFLAGS $EXTRA_CFLAGS"
>  
> +# extra-libs
> +LIBS="$LIBS $EXTRA_LIBS"
> +libs_softmmu="$libs_softmmu $EXTRA_LIBS"
> +libs_qga="$libs_qga $EXTRA_LIBS"
> +
>  echo "Install prefix    $prefix"
>  echo "BIOS directory    `eval echo $qemu_datadir`"
>  echo "binary directory  `eval echo $bindir`"
> @@ -4888,6 +4898,7 @@ fi
>  echo "qemu_helperdir=$libexecdir" >> $config_host_mak
>  echo "extra_cflags=$EXTRA_CFLAGS" >> $config_host_mak
>  echo "extra_ldflags=$EXTRA_LDFLAGS" >> $config_host_mak
> +echo "extra_libs=$EXTRA_LIBS" >> $config_host_mak
>  echo "qemu_localedir=$qemu_localedir" >> $config_host_mak
>  echo "libs_softmmu=$libs_softmmu" >> $config_host_mak
>  
> 

Re: [Qemu-devel] [RFC PATCH 2/4] configure: introduce --extra-libs

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 25/01/2016 17:49, Alex Bennée wrote:
>> If for example you want to use the thread sanitizer you want to ensure all
>> binaries are linked with the library:
>>
>>   ./configure ${TARGETS} --cc=gcc-5 --cxx=g++-5 \
>>     --extra-cflags="-fsanitize=thread" --extra-libs="-ltsan"
>
> Shouldn't -fsanitize=thread work as a linker command line flag too?

No, the sanitizers are compile time options as they instrument the
generated code. It's just in the case of the ThreadSanitizer you also
need the support library.

> Perhaps if that works there's a better place to put -fsanitizer flags.
>
> Paolo
>
>> This is more explicit than just specifying --extra-ldflags which might
>> not get applied in the right place all the time.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  configure | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/configure b/configure
>> index 7d23c6c..194bae9 100755
>> --- a/configure
>> +++ b/configure
>> @@ -365,6 +365,8 @@ for opt do
>>    --extra-ldflags=*) LDFLAGS="$LDFLAGS $optarg"
>>                       EXTRA_LDFLAGS="$optarg"
>>    ;;
>> +  --extra-libs=*)    EXTRA_LIBS="$optarg"
>> +  ;;
>>    --enable-debug-info) debug_info="yes"
>>    ;;
>>    --disable-debug-info) debug_info="no"
>> @@ -785,6 +787,8 @@ for opt do
>>    ;;
>>    --extra-ldflags=*)
>>    ;;
>> +  --extra-libs=*)
>> +  ;;
>>    --enable-debug-info)
>>    ;;
>>    --disable-debug-info)
>> @@ -1281,6 +1285,7 @@ Advanced options (experts only):
>>    --objcc=OBJCC            use Objective-C compiler OBJCC [$objcc]
>>    --extra-cflags=CFLAGS    append extra C compiler flags QEMU_CFLAGS
>>    --extra-ldflags=LDFLAGS  append extra linker flags LDFLAGS
>> +  --extra-libs=LIBS        append extra libraries when linking
>>    --make=MAKE              use specified make [$make]
>>    --install=INSTALL        use specified install [$install]
>>    --python=PYTHON          use specified python [$python]
>> @@ -4718,6 +4723,11 @@ libs_softmmu="$pixman_libs $libs_softmmu"
>>  CFLAGS="$CFLAGS $EXTRA_CFLAGS"
>>  QEMU_CFLAGS="$QEMU_CFLAGS $EXTRA_CFLAGS"
>>
>> +# extra-libs
>> +LIBS="$LIBS $EXTRA_LIBS"
>> +libs_softmmu="$libs_softmmu $EXTRA_LIBS"
>> +libs_qga="$libs_qga $EXTRA_LIBS"
>> +
>>  echo "Install prefix    $prefix"
>>  echo "BIOS directory    `eval echo $qemu_datadir`"
>>  echo "binary directory  `eval echo $bindir`"
>> @@ -4888,6 +4898,7 @@ fi
>>  echo "qemu_helperdir=$libexecdir" >> $config_host_mak
>>  echo "extra_cflags=$EXTRA_CFLAGS" >> $config_host_mak
>>  echo "extra_ldflags=$EXTRA_LDFLAGS" >> $config_host_mak
>> +echo "extra_libs=$EXTRA_LIBS" >> $config_host_mak
>>  echo "qemu_localedir=$qemu_localedir" >> $config_host_mak
>>  echo "libs_softmmu=$libs_softmmu" >> $config_host_mak
>>
>>


--
Alex Bennée

Re: [Qemu-devel] [RFC PATCH 2/4] configure: introduce --extra-libs

[Paolo Bonzini]

On 25/01/2016 19:15, Alex Bennée wrote:
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 25/01/2016 17:49, Alex Bennée wrote:
>>> If for example you want to use the thread sanitizer you want to ensure all
>>> binaries are linked with the library:
>>>
>>>   ./configure ${TARGETS} --cc=gcc-5 --cxx=g++-5 \
>>>     --extra-cflags="-fsanitize=thread" --extra-libs="-ltsan"
>>
>> Shouldn't -fsanitize=thread work as a linker command line flag too?
> 
> No, the sanitizers are compile time options as they instrument the
> generated code. It's just in the case of the ThreadSanitizer you also
> need the support library.

That's certainly not the case.  My system has at least a libubsan, 
libasan and liblsan (in addition to libtsan), and "gcc -dumpspecs"
suggests that the -fsanitize options are also valid at link time:

   %{%:sanitize(address):%{!shared:libasan_preinit%O%s} %{static-libasan:%{!shared:-Bstatic --whole-archive -lasan --no-whole-archive -Bdynamic}}%{!static-libasan:-lasan}}
   %{%:sanitize(thread):%{static-libtsan:%{!shared:-Bstatic --whole-archive -ltsan --no-whole-archive -Bdynamic}}%{!static-libtsan:-ltsan}}
   %{%:sanitize(leak):%{static-liblsan:%{!shared:-Bstatic --whole-archive -llsan --no-whole-archive -Bdynamic}}%{!static-liblsan:-llsan}}

(GCC specs are what they are, but you get the idea).

Paolo

Re: [Qemu-devel] [RFC PATCH 2/4] configure: introduce --extra-libs

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 25/01/2016 19:15, Alex Bennée wrote:
>>
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> On 25/01/2016 17:49, Alex Bennée wrote:
>>>> If for example you want to use the thread sanitizer you want to ensure all
>>>> binaries are linked with the library:
>>>>
>>>>   ./configure ${TARGETS} --cc=gcc-5 --cxx=g++-5 \
>>>>     --extra-cflags="-fsanitize=thread" --extra-libs="-ltsan"
>>>
>>> Shouldn't -fsanitize=thread work as a linker command line flag too?
>>
>> No, the sanitizers are compile time options as they instrument the
>> generated code. It's just in the case of the ThreadSanitizer you also
>> need the support library.
>
> That's certainly not the case.  My system has at least a libubsan,
> libasan and liblsan (in addition to libtsan), and "gcc -dumpspecs"
> suggests that the -fsanitize options are also valid at link time:
>
>    %{%:sanitize(address):%{!shared:libasan_preinit%O%s} %{static-libasan:%{!shared:-Bstatic --whole-archive -lasan --no-whole-archive -Bdynamic}}%{!static-libasan:-lasan}}
>    %{%:sanitize(thread):%{static-libtsan:%{!shared:-Bstatic --whole-archive -ltsan --no-whole-archive -Bdynamic}}%{!static-libtsan:-ltsan}}
>    %{%:sanitize(leak):%{static-liblsan:%{!shared:-Bstatic --whole-archive -llsan --no-whole-archive -Bdynamic}}%{!static-liblsan:-llsan}}
>
> (GCC specs are what they are, but you get the idea).

Hmm odd. I ran the undefined and address sanitizers without having to
mess with the ldflags. I'll have a deeper dive into the docs to see
whats going on.

>
> Paolo


--
Alex Bennée

[Qemu-devel] [RFC PATCH 3/4] include/qemu/atomic.h: default to __atomic functions

[Alex Bennée]

The __atomic primitives have been available since GCC 4.7 and provide
a richer interface for describing memory ordering requirements. As a
bonus by using the primitives instead of hand-rolled functions we can
use tools such as the AddressSanitizer which need the use of well
defined APIs for its analysis.

If we have __ATOMIC defines we exclusively use the __atomic primitives
for all our atomic access. Otherwise we fall back to the mixture of
__sync and hand-rolled barrier cases.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/qemu/atomic.h | 126 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 82 insertions(+), 44 deletions(-)

diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index bd2c075..414c81a 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -15,12 +15,90 @@
 
 #include "qemu/compiler.h"
 
-/* For C11 atomic ops */
 
 /* Compiler barrier */
 #define barrier()   ({ asm volatile("" ::: "memory"); (void)0; })
 
-#ifndef __ATOMIC_RELAXED
+#ifdef __ATOMIC_RELAXED
+/* For C11 atomic ops */
+
+/* Manual memory barriers
+ *
+ *__atomic_thread_fence does not include a compiler barrier; instead,
+ * the barrier is part of __atomic_load/__atomic_store's "volatile-like"
+ * semantics. If smp_wmb() is a no-op, absence of the barrier means that
+ * the compiler is free to reorder stores on each side of the barrier.
+ * Add one here, and similarly in smp_rmb() and smp_read_barrier_depends().
+ */
+
+#define smp_mb()    ({ barrier(); __atomic_thread_fence(__ATOMIC_CONSUME); barrier(); })
+#define smp_wmb()   ({ barrier(); __atomic_thread_fence(__ATOMIC_RELEASE); barrier(); })
+#define smp_rmb()   ({ barrier(); __atomic_thread_fence(__ATOMIC_ACQUIRE); barrier(); })
+
+#define smp_read_barrier_depends() ({ barrier(); __atomic_thread_fence(__ATOMIC_CONSUME); barrier(); })
+
+/* Weak atomic operations prevent the compiler moving other
+ * loads/stores past the atomic operation load/store.
+ */
+#define atomic_read(ptr)                          \
+    ({                                            \
+    typeof(*ptr) _val;                            \
+     __atomic_load(ptr, &_val, __ATOMIC_CONSUME); \
+    _val;                                         \
+    })
+
+#define atomic_rcu_read(ptr)    atomic_read(ptr)
+
+#define atomic_set(ptr, i)  do {                        \
+    typeof(*ptr) _val = (i);                            \
+    __atomic_store(ptr, &_val, __ATOMIC_RELEASE);       \
+} while(0)
+
+#define atomic_rcu_set(ptr, i)  atomic_set(ptr, i)
+
+/* Sequentially consistent atomic access */
+
+#define atomic_xchg(ptr, i)    ({                           \
+    typeof(*ptr) _new = (i), _old;                          \
+    __atomic_exchange(ptr, &_new, &_old, __ATOMIC_SEQ_CST); \
+    _old;                                                   \
+})
+
+/* Returns the eventual value, failed or not */
+#define atomic_cmpxchg(ptr, old, new)                                   \
+    ({                                                                  \
+    typeof(*ptr) _old = (old), _new = (new);                            \
+    __atomic_compare_exchange(ptr, &_old, &_new, false,                 \
+                              __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);      \
+    *ptr; /* can this race if cmpxchg not used elsewhere? */            \
+    })
+
+#define atomic_mb_set(ptr, i)   ((void)atomic_xchg(ptr, i))
+#define atomic_mb_read(ptr)                             \
+    ({                                                  \
+    typeof(*ptr) _val;                                  \
+     __atomic_load(ptr, &_val, __ATOMIC_ACQUIRE);       \
+    _val;                                               \
+    })
+
+
+/* Provide shorter names for GCC atomic builtins, return old value */
+#define atomic_fetch_inc(ptr)  __atomic_fetch_add(ptr, 1, __ATOMIC_SEQ_CST)
+#define atomic_fetch_dec(ptr)  __atomic_fetch_sub(ptr, 1, __ATOMIC_SEQ_CST)
+#define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, __ATOMIC_SEQ_CST)
+#define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, __ATOMIC_SEQ_CST)
+#define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, __ATOMIC_SEQ_CST)
+#define atomic_fetch_or(ptr, n)  __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST)
+
+/* And even shorter names that return void.  */
+#define atomic_inc(ptr)    ((void) __atomic_fetch_add(ptr, 1, __ATOMIC_SEQ_CST))
+#define atomic_dec(ptr)    ((void) __atomic_fetch_sub(ptr, 1, __ATOMIC_SEQ_CST))
+#define atomic_add(ptr, n) ((void) __atomic_fetch_add(ptr, n, __ATOMIC_SEQ_CST))
+#define atomic_sub(ptr, n) ((void) __atomic_fetch_sub(ptr, n, __ATOMIC_SEQ_CST))
+#define atomic_and(ptr, n) ((void) __atomic_fetch_and(ptr, n, __ATOMIC_SEQ_CST))
+#define atomic_or(ptr, n)  ((void) __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST))
+
+#else /* __ATOMIC_RELAXED */
 
 /*
  * We use GCC builtin if it's available, as that can use mfence on
@@ -85,8 +163,6 @@
 
 #endif /* _ARCH_PPC */
 
-#endif /* C11 atomics */
-
 /*
  * For (host) platforms we don't have explicit barrier definitions
  * for, we use the gcc __sync_synchronize() primitive to generate a
@@ -98,34 +174,16 @@
 #endif
 
 #ifndef smp_wmb
-#ifdef __ATOMIC_RELEASE
-/* __atomic_thread_fence does not include a compiler barrier; instead,
- * the barrier is part of __atomic_load/__atomic_store's "volatile-like"
- * semantics. If smp_wmb() is a no-op, absence of the barrier means that
- * the compiler is free to reorder stores on each side of the barrier.
- * Add one here, and similarly in smp_rmb() and smp_read_barrier_depends().
- */
-#define smp_wmb()   ({ barrier(); __atomic_thread_fence(__ATOMIC_RELEASE); barrier(); })
-#else
 #define smp_wmb()   __sync_synchronize()
 #endif
-#endif
 
 #ifndef smp_rmb
-#ifdef __ATOMIC_ACQUIRE
-#define smp_rmb()   ({ barrier(); __atomic_thread_fence(__ATOMIC_ACQUIRE); barrier(); })
-#else
 #define smp_rmb()   __sync_synchronize()
 #endif
-#endif
 
 #ifndef smp_read_barrier_depends
-#ifdef __ATOMIC_CONSUME
-#define smp_read_barrier_depends()   ({ barrier(); __atomic_thread_fence(__ATOMIC_CONSUME); barrier(); })
-#else
 #define smp_read_barrier_depends()   barrier()
 #endif
-#endif
 
 #ifndef atomic_read
 #define atomic_read(ptr)       (*(__typeof__(*ptr) volatile*) (ptr))
@@ -156,20 +214,12 @@
  * Should match atomic_rcu_set(), atomic_xchg(), atomic_cmpxchg().
  */
 #ifndef atomic_rcu_read
-#ifdef __ATOMIC_CONSUME
-#define atomic_rcu_read(ptr)    ({                \
-    typeof(*ptr) _val;                            \
-     __atomic_load(ptr, &_val, __ATOMIC_CONSUME); \
-    _val;                                         \
-})
-#else
 #define atomic_rcu_read(ptr)    ({                \
     typeof(*ptr) _val = atomic_read(ptr);         \
     smp_read_barrier_depends();                   \
     _val;                                         \
 })
 #endif
-#endif
 
 /**
  * atomic_rcu_set - assigns (publicizes) a pointer to a new data structure
@@ -183,18 +233,11 @@
  * Should match atomic_rcu_read().
  */
 #ifndef atomic_rcu_set
-#ifdef __ATOMIC_RELEASE
-#define atomic_rcu_set(ptr, i)  do {              \
-    typeof(*ptr) _val = (i);                      \
-    __atomic_store(ptr, &_val, __ATOMIC_RELEASE); \
-} while(0)
-#else
 #define atomic_rcu_set(ptr, i)  do {              \
     smp_wmb();                                    \
     atomic_set(ptr, i);                           \
 } while (0)
 #endif
-#endif
 
 /* These have the same semantics as Java volatile variables.
  * See http://gee.cs.oswego.edu/dl/jmm/cookbook.html:
@@ -237,12 +280,6 @@
 #ifndef atomic_xchg
 #if defined(__clang__)
 #define atomic_xchg(ptr, i)    __sync_swap(ptr, i)
-#elif defined(__ATOMIC_SEQ_CST)
-#define atomic_xchg(ptr, i)    ({                           \
-    typeof(*ptr) _new = (i), _old;                          \
-    __atomic_exchange(ptr, &_new, &_old, __ATOMIC_SEQ_CST); \
-    _old;                                                   \
-})
 #else
 /* __sync_lock_test_and_set() is documented to be an acquire barrier only.  */
 #define atomic_xchg(ptr, i)    (smp_mb(), __sync_lock_test_and_set(ptr, i))
@@ -266,4 +303,5 @@
 #define atomic_and(ptr, n)     ((void) __sync_fetch_and_and(ptr, n))
 #define atomic_or(ptr, n)      ((void) __sync_fetch_and_or(ptr, n))
 
-#endif
+#endif /* __ATOMIC_RELAXED */
+#endif /* __QEMU_ATOMIC_H */
-- 
2.7.0

Re: [Qemu-devel] [RFC PATCH 3/4] include/qemu/atomic.h: default to __atomic functions

[Paolo Bonzini]

On 25/01/2016 17:49, Alex Bennée wrote:
> The __atomic primitives have been available since GCC 4.7 and provide
> a richer interface for describing memory ordering requirements. As a
> bonus by using the primitives instead of hand-rolled functions we can
> use tools such as the AddressSanitizer which need the use of well
> defined APIs for its analysis.
> 
> If we have __ATOMIC defines we exclusively use the __atomic primitives
> for all our atomic access. Otherwise we fall back to the mixture of
> __sync and hand-rolled barrier cases.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  include/qemu/atomic.h | 126 ++++++++++++++++++++++++++++++++------------------
>  1 file changed, 82 insertions(+), 44 deletions(-)
> 
> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
> index bd2c075..414c81a 100644
> --- a/include/qemu/atomic.h
> +++ b/include/qemu/atomic.h
> @@ -15,12 +15,90 @@
>  
>  #include "qemu/compiler.h"
>  
> -/* For C11 atomic ops */
>  
>  /* Compiler barrier */
>  #define barrier()   ({ asm volatile("" ::: "memory"); (void)0; })
>  
> -#ifndef __ATOMIC_RELAXED
> +#ifdef __ATOMIC_RELAXED
> +/* For C11 atomic ops */
> +
> +/* Manual memory barriers
> + *
> + *__atomic_thread_fence does not include a compiler barrier; instead,
> + * the barrier is part of __atomic_load/__atomic_store's "volatile-like"
> + * semantics. If smp_wmb() is a no-op, absence of the barrier means that
> + * the compiler is free to reorder stores on each side of the barrier.
> + * Add one here, and similarly in smp_rmb() and smp_read_barrier_depends().
> + */
> +
> +#define smp_mb()    ({ barrier(); __atomic_thread_fence(__ATOMIC_CONSUME); barrier(); })

This should be seq_cst.

> +#define smp_wmb()   ({ barrier(); __atomic_thread_fence(__ATOMIC_RELEASE); barrier(); })
> +#define smp_rmb()   ({ barrier(); __atomic_thread_fence(__ATOMIC_ACQUIRE); barrier(); })
> +
> +#define smp_read_barrier_depends() ({ barrier(); __atomic_thread_fence(__ATOMIC_CONSUME); barrier(); })
> +
> +/* Weak atomic operations prevent the compiler moving other
> + * loads/stores past the atomic operation load/store.
> + */
> +#define atomic_read(ptr)                          \
> +    ({                                            \
> +    typeof(*ptr) _val;                            \
> +     __atomic_load(ptr, &_val, __ATOMIC_CONSUME); \

This should be relaxed.

> +    _val;                                         \
> +    })
> +
> +#define atomic_rcu_read(ptr)    atomic_read(ptr)

This should be consume.

> +
> +#define atomic_set(ptr, i)  do {                        \
> +    typeof(*ptr) _val = (i);                            \
> +    __atomic_store(ptr, &_val, __ATOMIC_RELEASE);       \

This should be relaxed.

> +} while(0)
> +
> +#define atomic_rcu_set(ptr, i)  atomic_set(ptr, i)

This should be release.

> +/* Sequentially consistent atomic access */
> +
> +#define atomic_xchg(ptr, i)    ({                           \
> +    typeof(*ptr) _new = (i), _old;                          \
> +    __atomic_exchange(ptr, &_new, &_old, __ATOMIC_SEQ_CST); \
> +    _old;                                                   \
> +})
> +
> +/* Returns the eventual value, failed or not */
> +#define atomic_cmpxchg(ptr, old, new)                                   \
> +    ({                                                                  \
> +    typeof(*ptr) _old = (old), _new = (new);                            \
> +    __atomic_compare_exchange(ptr, &_old, &_new, false,                 \
> +                              __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);      \
> +    *ptr; /* can this race if cmpxchg not used elsewhere? */            \

If I read the manual correctly, you can return _old here (that's why it
is a pointer).

> +    })
> +
> +#define atomic_mb_set(ptr, i)   ((void)atomic_xchg(ptr, i))
> +#define atomic_mb_read(ptr)                             \
> +    ({                                                  \
> +    typeof(*ptr) _val;                                  \
> +     __atomic_load(ptr, &_val, __ATOMIC_ACQUIRE);       \
> +    _val;                                               \
> +    })

Please leave these defined in terms of relaxed accesses and memory barriers.

Paolo

Re: [Qemu-devel] [RFC PATCH 3/4] include/qemu/atomic.h: default to __atomic functions

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 25/01/2016 17:49, Alex Bennée wrote:
>> The __atomic primitives have been available since GCC 4.7 and provide
>> a richer interface for describing memory ordering requirements. As a
>> bonus by using the primitives instead of hand-rolled functions we can
>> use tools such as the AddressSanitizer which need the use of well
>> defined APIs for its analysis.
>>
>> If we have __ATOMIC defines we exclusively use the __atomic primitives
>> for all our atomic access. Otherwise we fall back to the mixture of
>> __sync and hand-rolled barrier cases.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  include/qemu/atomic.h | 126 ++++++++++++++++++++++++++++++++------------------
>>  1 file changed, 82 insertions(+), 44 deletions(-)
>>
>> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
>> index bd2c075..414c81a 100644
>> --- a/include/qemu/atomic.h
>> +++ b/include/qemu/atomic.h
>> @@ -15,12 +15,90 @@
>>
>>  #include "qemu/compiler.h"
>>
>> -/* For C11 atomic ops */
>>
>>  /* Compiler barrier */
>>  #define barrier()   ({ asm volatile("" ::: "memory"); (void)0; })
>>
>> -#ifndef __ATOMIC_RELAXED
>> +#ifdef __ATOMIC_RELAXED
>> +/* For C11 atomic ops */
>> +
>> +/* Manual memory barriers
>> + *
>> + *__atomic_thread_fence does not include a compiler barrier; instead,
>> + * the barrier is part of __atomic_load/__atomic_store's "volatile-like"
>> + * semantics. If smp_wmb() is a no-op, absence of the barrier means that
>> + * the compiler is free to reorder stores on each side of the barrier.
>> + * Add one here, and similarly in smp_rmb() and smp_read_barrier_depends().
>> + */
>> +
>> +#define smp_mb()    ({ barrier(); __atomic_thread_fence(__ATOMIC_CONSUME); barrier(); })
>
> This should be seq_cst.

heh when I did this originally I had it as that (it was an overly
complex splitting into handrolled, __seq and __atomic implementations.)
I'll fix it. I think I can remove the barrier()'s as well can't I?

>
>> +#define smp_wmb()   ({ barrier(); __atomic_thread_fence(__ATOMIC_RELEASE); barrier(); })
>> +#define smp_rmb()   ({ barrier(); __atomic_thread_fence(__ATOMIC_ACQUIRE); barrier(); })
>> +
>> +#define smp_read_barrier_depends() ({ barrier(); __atomic_thread_fence(__ATOMIC_CONSUME); barrier(); })
>> +
>> +/* Weak atomic operations prevent the compiler moving other
>> + * loads/stores past the atomic operation load/store.
>> + */
>> +#define atomic_read(ptr)                          \
>> +    ({                                            \
>> +    typeof(*ptr) _val;                            \
>> +     __atomic_load(ptr, &_val, __ATOMIC_CONSUME); \
>
> This should be relaxed.
>
>> +    _val;                                         \
>> +    })
>> +
>> +#define atomic_rcu_read(ptr)    atomic_read(ptr)
>
> This should be consume.
>
>> +
>> +#define atomic_set(ptr, i)  do {                        \
>> +    typeof(*ptr) _val = (i);                            \
>> +    __atomic_store(ptr, &_val, __ATOMIC_RELEASE);       \
>
> This should be relaxed.
>
>> +} while(0)
>> +
>> +#define atomic_rcu_set(ptr, i)  atomic_set(ptr, i)
>
> This should be release.
>
>> +/* Sequentially consistent atomic access */
>> +
>> +#define atomic_xchg(ptr, i)    ({                           \
>> +    typeof(*ptr) _new = (i), _old;                          \
>> +    __atomic_exchange(ptr, &_new, &_old, __ATOMIC_SEQ_CST); \
>> +    _old;                                                   \
>> +})
>> +
>> +/* Returns the eventual value, failed or not */
>> +#define atomic_cmpxchg(ptr, old, new)                                   \
>> +    ({                                                                  \
>> +    typeof(*ptr) _old = (old), _new = (new);                            \
>> +    __atomic_compare_exchange(ptr, &_old, &_new, false,                 \
>> +                              __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);      \
>> +    *ptr; /* can this race if cmpxchg not used elsewhere? */            \
>
> If I read the manual correctly, you can return _old here (that's why it
> is a pointer).
>
>> +    })
>> +
>> +#define atomic_mb_set(ptr, i)   ((void)atomic_xchg(ptr, i))
>> +#define atomic_mb_read(ptr)                             \
>> +    ({                                                  \
>> +    typeof(*ptr) _val;                                  \
>> +     __atomic_load(ptr, &_val, __ATOMIC_ACQUIRE);       \
>> +    _val;                                               \
>> +    })
>
> Please leave these defined in terms of relaxed accesses and memory
> barriers.

May I ask why? Doesn't the __ATOMIC_ACQUIRE ensure the load barrier is
in place? Or should it be a fill CST barrier?

>
> Paolo


--
Alex Bennée

Re: [Qemu-devel] [RFC PATCH 3/4] include/qemu/atomic.h: default to __atomic functions

[Paolo Bonzini]

On 25/01/2016 19:23, Alex Bennée wrote:
>>> +    })
>>> +
>>> +#define atomic_mb_set(ptr, i)   ((void)atomic_xchg(ptr, i))
>>> +#define atomic_mb_read(ptr)                             \
>>> +    ({                                                  \
>>> +    typeof(*ptr) _val;                                  \
>>> +     __atomic_load(ptr, &_val, __ATOMIC_ACQUIRE);       \
>>> +    _val;                                               \
>>> +    })
>>
>> Please leave these defined in terms of relaxed accesses and memory
>> barriers.
> 
> May I ask why? Doesn't the __ATOMIC_ACQUIRE ensure the load barrier is
> in place? Or should it be a fill CST barrier?

First, because they are defined like this in docs/atomics.txt. :)

Second, because the right definition would be
__atomic_load(__ATOMIC_SEQ_CST) and __atomic_store(__ATOMIC_SEQ_CST).
These produce even better code than the memory barriers on x86 but,
unfortunately, on POWER they are implemented in such a way that make
things very slow.

Basically, instead of

    mb_read -> load; rmb()
    mb_set -> wmb(); store(); mb()

POWER uses this:

    mb_read -> load; mb()
    mb_set -> wmb(); store(); mb()

There are reasons for these, and they are proved in
http://www.cl.cam.ac.uk/~pes20/cppppc/popl079-batty.pdf (I'm not
pretending I understand this paper except inasmuch as it was necessary
to write docs/atomics.txt).  However, the cases that actually matter
basically never arise.  Again, this is documented in docs/atomics.txt
around line 90.

As an alternative to explicit memory barriers, you can use this on POWER:

   mb_read -> load-acquire
   mb_set -> store-release + mb()

and seq-cst load/store everywhere else.

Paolo

[Qemu-devel] [RFC PATCH 4/4] tsan: various fixes for make check

[Alex Bennée]

After building with the ThreadSanitizer I ran make check and started
going through the failures reported. Most are failures to use atomic
primitives to access variables previously atomically set. While this
likely will work on x86 it could cause problems on other architectures.

 - async: use atomic reads for scheduled/notify_me
 - thread-pool: use atomic_mb_read/set to for thread ->state
 - test-thread-pool: use atomic read for data.n

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 async.c                  | 4 ++--
 tests/test-thread-pool.c | 2 +-
 thread-pool.c            | 9 ++++-----
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/async.c b/async.c
index e106072..8d5f810 100644
--- a/async.c
+++ b/async.c
@@ -165,7 +165,7 @@ aio_compute_timeout(AioContext *ctx)
     QEMUBH *bh;
 
     for (bh = ctx->first_bh; bh; bh = bh->next) {
-        if (!bh->deleted && bh->scheduled) {
+        if (!bh->deleted && atomic_read(&bh->scheduled)) {
             if (bh->idle) {
                 /* idle bottom halves will be polled at least
                  * every 10ms */
@@ -286,7 +286,7 @@ void aio_notify(AioContext *ctx)
      * with atomic_or in aio_ctx_prepare or atomic_add in aio_poll.
      */
     smp_mb();
-    if (ctx->notify_me) {
+    if (atomic_read(&ctx->notify_me)) {
         event_notifier_set(&ctx->notifier);
         atomic_mb_set(&ctx->notified, true);
     }
diff --git a/tests/test-thread-pool.c b/tests/test-thread-pool.c
index ccdee39..5694ad9 100644
--- a/tests/test-thread-pool.c
+++ b/tests/test-thread-pool.c
@@ -46,7 +46,7 @@ static void test_submit(void)
 {
     WorkerTestData data = { .n = 0 };
     thread_pool_submit(pool, worker_cb, &data);
-    while (data.n == 0) {
+    while (atomic_read(&data.n) == 0) {
         aio_poll(ctx, true);
     }
     g_assert_cmpint(data.n, ==, 1);
diff --git a/thread-pool.c b/thread-pool.c
index 402c778..97b2c0c 100644
--- a/thread-pool.c
+++ b/thread-pool.c
@@ -99,15 +99,14 @@ static void *worker_thread(void *opaque)
 
         req = QTAILQ_FIRST(&pool->request_list);
         QTAILQ_REMOVE(&pool->request_list, req, reqs);
-        req->state = THREAD_ACTIVE;
+        atomic_mb_set(&req->state, THREAD_ACTIVE);
         qemu_mutex_unlock(&pool->lock);
 
         ret = req->func(req->arg);
 
         req->ret = ret;
         /* Write ret before state.  */
-        smp_wmb();
-        req->state = THREAD_DONE;
+        atomic_mb_set(&req->state, THREAD_DONE);
 
         qemu_mutex_lock(&pool->lock);
 
@@ -167,7 +166,7 @@ static void thread_pool_completion_bh(void *opaque)
 
 restart:
     QLIST_FOREACH_SAFE(elem, &pool->head, all, next) {
-        if (elem->state != THREAD_DONE) {
+        if (atomic_read(&elem->state) != THREAD_DONE) {
             continue;
         }
 
@@ -201,7 +200,7 @@ static void thread_pool_cancel(BlockAIOCB *acb)
     trace_thread_pool_cancel(elem, elem->common.opaque);
 
     qemu_mutex_lock(&pool->lock);
-    if (elem->state == THREAD_QUEUED &&
+    if (atomic_mb_read(&elem->state) == THREAD_QUEUED &&
         /* No thread has yet started working on elem. we can try to "steal"
          * the item from the worker if we can get a signal from the
          * semaphore.  Because this is non-blocking, we can do it with
-- 
2.7.0

Re: [Qemu-devel] [RFC PATCH 4/4] tsan: various fixes for make check

[Paolo Bonzini]

On 25/01/2016 17:49, Alex Bennée wrote:
> After building with the ThreadSanitizer I ran make check and started
> going through the failures reported. Most are failures to use atomic
> primitives to access variables previously atomically set. While this
> likely will work on x86 it could cause problems on other architectures.

It will work on other architectures, because there are memory barriers
(either explicit, or implicit in other atomic_* ops).  In fact, using
atomic_read/atomic_set would actually fix bugs in x86 :) if it weren't
for commit 3bbf572 ("atomics: add explicit compiler fence in __atomic
memory barriers", 2015-06-03).

>  - async: use atomic reads for scheduled/notify_me
>  - thread-pool: use atomic_mb_read/set to for thread ->state

Please use atomic_read/atomic_set and keep the memory barriers.  That's
a fine way to convert code that uses non-atomic accesses together with
memory barriers (not just thread-pool, also e.g. virtio).

Otherwise looks sane, thanks!

Paolo

>  - test-thread-pool: use atomic read for data.n
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  async.c                  | 4 ++--
>  tests/test-thread-pool.c | 2 +-
>  thread-pool.c            | 9 ++++-----
>  3 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/async.c b/async.c
> index e106072..8d5f810 100644
> --- a/async.c
> +++ b/async.c
> @@ -165,7 +165,7 @@ aio_compute_timeout(AioContext *ctx)
>      QEMUBH *bh;
>  
>      for (bh = ctx->first_bh; bh; bh = bh->next) {
> -        if (!bh->deleted && bh->scheduled) {
> +        if (!bh->deleted && atomic_read(&bh->scheduled)) {
>              if (bh->idle) {
>                  /* idle bottom halves will be polled at least
>                   * every 10ms */
> @@ -286,7 +286,7 @@ void aio_notify(AioContext *ctx)
>       * with atomic_or in aio_ctx_prepare or atomic_add in aio_poll.
>       */
>      smp_mb();
> -    if (ctx->notify_me) {
> +    if (atomic_read(&ctx->notify_me)) {
>          event_notifier_set(&ctx->notifier);
>          atomic_mb_set(&ctx->notified, true);
>      }
> diff --git a/tests/test-thread-pool.c b/tests/test-thread-pool.c
> index ccdee39..5694ad9 100644
> --- a/tests/test-thread-pool.c
> +++ b/tests/test-thread-pool.c
> @@ -46,7 +46,7 @@ static void test_submit(void)
>  {
>      WorkerTestData data = { .n = 0 };
>      thread_pool_submit(pool, worker_cb, &data);
> -    while (data.n == 0) {
> +    while (atomic_read(&data.n) == 0) {
>          aio_poll(ctx, true);
>      }
>      g_assert_cmpint(data.n, ==, 1);
> diff --git a/thread-pool.c b/thread-pool.c
> index 402c778..97b2c0c 100644
> --- a/thread-pool.c
> +++ b/thread-pool.c
> @@ -99,15 +99,14 @@ static void *worker_thread(void *opaque)
>  
>          req = QTAILQ_FIRST(&pool->request_list);
>          QTAILQ_REMOVE(&pool->request_list, req, reqs);
> -        req->state = THREAD_ACTIVE;
> +        atomic_mb_set(&req->state, THREAD_ACTIVE);
>          qemu_mutex_unlock(&pool->lock);
>  
>          ret = req->func(req->arg);
>  
>          req->ret = ret;
>          /* Write ret before state.  */
> -        smp_wmb();
> -        req->state = THREAD_DONE;
> +        atomic_mb_set(&req->state, THREAD_DONE);
>  
>          qemu_mutex_lock(&pool->lock);
>  
> @@ -167,7 +166,7 @@ static void thread_pool_completion_bh(void *opaque)
>  
>  restart:
>      QLIST_FOREACH_SAFE(elem, &pool->head, all, next) {
> -        if (elem->state != THREAD_DONE) {
> +        if (atomic_read(&elem->state) != THREAD_DONE) {
>              continue;
>          }
>  
> @@ -201,7 +200,7 @@ static void thread_pool_cancel(BlockAIOCB *acb)
>      trace_thread_pool_cancel(elem, elem->common.opaque);
>  
>      qemu_mutex_lock(&pool->lock);
> -    if (elem->state == THREAD_QUEUED &&
> +    if (atomic_mb_read(&elem->state) == THREAD_QUEUED &&
>          /* No thread has yet started working on elem. we can try to "steal"
>           * the item from the worker if we can get a signal from the
>           * semaphore.  Because this is non-blocking, we can do it with
>