[Bug libstdc++/54786] New: Missing fence in std::atomic<T>::store() triggers wrong reordering.

[Bug libstdc++/54786] New: Missing fence in std::atomic<T>::store() triggers wrong reordering.

[ozabluda at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54786

             Bug #: 54786
           Summary: Missing fence in std::atomic<T>::store() triggers
                    wrong reordering.
    Classification: Unclassified
           Product: gcc
           Version: 4.6.3
            Status: UNCONFIRMED
          Severity: major
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: ozabluda@gmail.com
                CC: hans_boehm@hp.com
              Host: x86_64-linux-gnu
            Target: x86_64-linux-gnu
             Build: x86_64-linux-gnu


Created attachment 28335
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28335
Preprocessed source

In the standard publish idiom:

#include <atomic>

double d;
std::atomic<int> aflag(0);

void publish() {
   d = 1.0;
   aflag = 1;
}


store to d is reordered (sunk) after store to aflag:

_Z7publishv:
.LFB382:
    .cfi_startproc
    movabsq    $4607182418800017408, %rax
    movl    $1, aflag(%rip) #<== store to aflag
    movq    %rax, d(%rip)   #<== store to d
    mfence
    ret


The reason for it is that in include/c++/4.6/bits/atomic_2.h store() is missing
needed __sync_synchronize() before non-atomic write:

      void
      store(__int_type __i, memory_order __m = memory_order_seq_cst)
      {
        __glibcxx_assert(__m != memory_order_acquire);
        __glibcxx_assert(__m != memory_order_acq_rel);
        __glibcxx_assert(__m != memory_order_consume);

        if (__m == memory_order_relaxed)
          _M_i = __i;
        else
          {
            // write_mem_barrier(); //OZ: Missing __sync_synchronize();  
            _M_i = __i;
            if (__m == memory_order_seq_cst)
              __sync_synchronize();
          }
      }


Incidentally, load() has unnecessary barrier before non-atomic read:

      __int_type
      load(memory_order __m = memory_order_seq_cst) const
      {
        __glibcxx_assert(__m != memory_order_release);
        __glibcxx_assert(__m != memory_order_acq_rel);

        __sync_synchronize(); //OZ: unnecessary
        __int_type __ret = _M_i;
        __sync_synchronize();
        return __ret;
      }

And by unnecessary, I don't mean just for "publish", but ever.

============================================================================

$ g++-4.6 -v -save-temps --std=c++0x -O2 -c reorder.cpp 
Using built-in specs.
COLLECT_GCC=g++-4.6
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/4.6/lto-wrapper
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu/Linaro
4.6.3-1ubuntu5' --with-bugurl=file:///usr/share/doc/gcc-4.6/README.Bugs
--enable-languages=c,c++,fortran,objc,obj-c++ --prefix=/usr
--program-suffix=-4.6 --enable-shared --enable-linker-build-id
--with-system-zlib --libexecdir=/usr/lib --without-included-gettext
--enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.6
--libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu
--enable-libstdcxx-debug --enable-libstdcxx-time=yes --enable-gnu-unique-object
--enable-plugin --enable-objc-gc --disable-werror --with-arch-32=i686
--with-tune=generic --enable-checking=release --build=x86_64-linux-gnu
--host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5) 
COLLECT_GCC_OPTIONS='-v' '-save-temps' '-std=c++0x' '-O2' '-c' '-shared-libgcc'
'-mtune=generic' '-march=x86-64'
 /usr/lib/gcc/x86_64-linux-gnu/4.6/cc1plus -E -quiet -v -imultilib .
-imultiarch x86_64-linux-gnu -D_GNU_SOURCE reorder.cpp -mtune=generic
-march=x86-64 -std=c++0x -O2 -fpch-preprocess -fstack-protector -o reorder.ii
ignoring nonexistent directory "/usr/local/include/x86_64-linux-gnu"
ignoring nonexistent directory
"/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../x86_64-linux-gnu/include"
#include "..." search starts here:
#include <...> search starts here:
 /usr/include/c++/4.6
 /usr/include/c++/4.6/x86_64-linux-gnu/.
 /usr/include/c++/4.6/backward
 /usr/lib/gcc/x86_64-linux-gnu/4.6/include
 /usr/local/include
 /usr/lib/gcc/x86_64-linux-gnu/4.6/include-fixed
 /usr/include/x86_64-linux-gnu
 /usr/include
End of search list.
COLLECT_GCC_OPTIONS='-v' '-save-temps' '-std=c++0x' '-O2' '-c' '-shared-libgcc'
'-mtune=generic' '-march=x86-64'
 /usr/lib/gcc/x86_64-linux-gnu/4.6/cc1plus -fpreprocessed reorder.ii -quiet
-dumpbase reorder.cpp -mtune=generic -march=x86-64 -auxbase reorder -O2
-std=c++0x -version -fstack-protector -o reorder.s
GNU C++ (Ubuntu/Linaro 4.6.3-1ubuntu5) version 4.6.3 (x86_64-linux-gnu)
    compiled by GNU C version 4.6.3, GMP version 5.0.2, MPFR version 3.1.0-p3,
MPC version 0.9
GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
GNU C++ (Ubuntu/Linaro 4.6.3-1ubuntu5) version 4.6.3 (x86_64-linux-gnu)
    compiled by GNU C version 4.6.3, GMP version 5.0.2, MPFR version 3.1.0-p3,
MPC version 0.9
GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
Compiler executable checksum: 65b5171ac1bd7b3f07dbea6bdb24be3d
COLLECT_GCC_OPTIONS='-v' '-save-temps' '-std=c++0x' '-O2' '-c' '-shared-libgcc'
'-mtune=generic' '-march=x86-64'
 as --64 -o reorder.o reorder.s
COMPILER_PATH=/usr/lib/gcc/x86_64-linux-gnu/4.6/:/usr/lib/gcc/x86_64-linux-gnu/4.6/:/usr/lib/gcc/x86_64-linux-gnu/:/usr/lib/gcc/x86_64-linux-gnu/4.6/:/usr/lib/gcc/x86_64-linux-gnu/
LIBRARY_PATH=/usr/lib/gcc/x86_64-linux-gnu/4.6/:/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../x86_64-linux-gnu/:/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../lib/:/lib/x86_64-linux-gnu/:/lib/../lib/:/usr/lib/x86_64-linux-gnu/:/usr/lib/../lib/:/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../:/lib/:/usr/lib/
COLLECT_GCC_OPTIONS='-v' '-save-temps' '-std=c++0x' '-O2' '-c' '-shared-libgcc'
'-mtune=generic' '-march=x86-64'

[Bug libstdc++/54786] Missing fence in std::atomic<T>::store() triggers wrong reordering.

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54786

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|major                       |normal

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> 2012-10-02 20:37:01 UTC ---
I think this has been fixed in 4.7.0 as it uses __atomic_store now instead.

[Bug libstdc++/54786] Missing fence in std::atomic<T>::store() triggers wrong reordering.

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54786

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|---                         |4.7.0

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> 2012-10-02 20:39:31 UTC ---
Note in 4.6.x we don't say we fully support C++11.  Even in 4.7.x we don't say
we do but it is much closer and even closer in implementing C++11/C11's
(threading) memory models.

[Bug libstdc++/54786] Missing fence in std::atomic<T>::store() triggers wrong reordering.

[ozabluda at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54786

--- Comment #3 from Oleg Zabluda <ozabluda at gmail dot com> 2012-10-02 22:04:37 UTC ---
I can confirm that it is fixed in 4.7.0, after the move to __atomic builtins,
at least on x86_64. It would be nice to have it fixed in currently hypothetical
4.6.4, especially for those whose distro is stuck on 4.6. But at least it's
documented now and users can act accordingly. For example add
__sync_syncronise() or asm volatile ("":::"memory") before atomic::store().

[Bug libstdc++/54786] Missing fence in std::atomic<T>::store() triggers wrong reordering.

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54786

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> 2012-10-03 00:56:25 UTC ---
(In reply to comment #3)
> I can confirm that it is fixed in 4.7.0, after the move to __atomic builtins,
> at least on x86_64. It would be nice to have it fixed in currently hypothetical
> 4.6.4, especially for those whose distro is stuck on 4.6. But at least it's
> documented now and users can act accordingly. For example add
> __sync_syncronise() or asm volatile ("":::"memory") before atomic::store().

Considering this is in an experimental part of the compiler (stdc++0x is
consider experimental for 4.6), I say you are out of luck from support from the
FSF.  You can request your distro to have the fix backported though.