Re: [Valgrind-users] helgrind and atomic operations

[Dave G. <go...@mc...>]

On Jan 23, 2013, at 8:08 AM CST, David Faure wrote:

> On Monday 27 August 2012 15:25:14 Marc Mutz wrote:
>> If atomic loads and stores on x86 are implemented with a volatile cast,
>> then  the compiler can't reorder stuff around them, either. Not more than
>> with a std::atomic, at least. QAtomic does that. For load-relaxed, Thiago
>> thinks that a normal read (non-volatile) is correct and I couldn't prove
>> him wrong.
[…]
> Where does the claim about "volatile cast means the compiler can't reorder stuff around them"
> come from?

Julian already addressed this in a separate post, and I agree with his assessment re: access to volatile-qualified variables.

> In the Qt source code I see qatomic_gcc.h (which is unused, unfortunately) calling
> __sync_synchronize() in loadAcquire(). Shouldn't Qt's code call that, when compiled with gcc?

If you don't want to write inline assembly, this might be your best bet.  But on TSO systems like x86, you only need a "compiler barrier".  In x86 inline assembly syntax, this looks like:

__asm__ __volatile__  ( "" ::: "memory" )

This prevents GCC (and compilers that correctly support this syntax) from reordering accesses across this statement or making assumptions about the state of memory across this point.

The only case you need to worry about on x86 (assuming you are not fiddling with "special" memory) is that earlier stores could be reordered after subsequent loads.  That should be the only time you need the real "mfence" instructions that you have below.  "load-acquire" and "store-release" don't fall into that category.

> This does lead to different assembly, too, on x86.
> So the claim that x86 doesn't need memory barriers seems wrong?
> 
> --- testcase_atomic_ops_helgrind.s.orig 2013-01-23 15:04:20.889417624 +0100
> +++ testcase_atomic_ops_helgrind.s      2013-01-23 15:07:06.938422071 +0100
> @@ -380,6 +380,7 @@ _ZN10QAtomicOpsIiE11loadAcquireERKi:
>        movq    -24(%rbp), %rax
>        movl    (%rax), %eax
>        movl    %eax, -4(%rbp)
> +       mfence
>        movl    -4(%rbp), %eax
>        popq    %rbp
>        .cfi_def_cfa 7, 8
> @@ -403,6 +404,7 @@ _ZN10QAtomicOpsIiE12storeReleaseERii:
>        movq    -8(%rbp), %rax
>        movl    -12(%rbp), %edx
>        movl    %edx, (%rax)
> +       mfence
>        popq    %rbp
>        .cfi_def_cfa 7, 8
>        ret
> 

-Dave