[Bug libstdc++/40297] New: [C++0x] debug mode vs atomics

[Bug libstdc++/40297] New: [C++0x] debug mode vs atomics

[jwakely dot gcc at gmail dot com]

The atomic types in <cstdatomic> have debug-mode assertions that will always
fail.

#define _GLIBCXX_DEBUG
#include <cstdatomic>

int main()
{
    std::atomic_address ptr;
    return ptr.load() ? 0 : 1;
}

Fails with:

/dev/shm/wakelyjo/insroot/gcc/lib/gcc/x86_64-unknown-linux-gnu/4.5.0/../../../../include/c++/4.5.0/bits/atomic_2.h:114:
void* std::__atomic2::atomic_address::load(std::memory_order) const volatile:
Assertion '__m == memory_order_release' failed.

Due to:
    void*
    load(memory_order __m = memory_order_seq_cst) const volatile
    {
      __glibcxx_assert(__m == memory_order_release);
      __glibcxx_assert(__m == memory_order_acq_rel);

      __sync_synchronize();
      void* __ret = _M_i;
      __sync_synchronize();
      return __ret;
    }

Since memory_order is an enumeration and each enumerator has a distinct value,
it's not possible for both assertions to pass.

I'm not sure what they were intended to check, unless it's to prevent atomics
being used in debug mode!


-- 
           Summary: [C++0x] debug mode vs atomics
           Product: gcc
           Version: 4.5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: jwakely dot gcc at gmail dot com


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

[Bug libstdc++/40297] [C++0x] debug mode vs atomics

[paolo dot carlini at oracle dot com]

------- Comment #1 from paolo dot carlini at oracle dot com  2009-06-04 15:16 -------
Let's CC Benjamin...


-- 

paolo dot carlini at oracle dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bkoz at redhat dot com


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

[Bug libstdc++/40297] [C++0x] debug mode vs atomics

[jwakely dot gcc at gmail dot com]

------- Comment #2 from jwakely dot gcc at gmail dot com  2009-06-16 12:35 -------
I think all the assertions are simply backwards, the load() operation requires:
"The order argument shall not be memory_order_release nor
memory_order_acq_rel."

so the assertions should be 
      __glibcxx_assert(__m != memory_order_release);
      __glibcxx_assert(__m != memory_order_acq_rel);

I can prepare a patch to fix that everywhere if Benjamin agrees.

Debug mode apart, I think there are more problems with the memory ordering.

__atomic2::atomic_flag::test_and_set() issues a full barrier when m ==
memory_order_relaxed (too much synchronization) but is not a release operation
when m == memory_order_acq_rel (not enough synchronization.) 
sync_lock_test_and_set is always an acquire barrier, so the full barrier is
only needed when the ordering specifies it should also be a release operation
i.e.

    bool
    test_and_set(memory_order __m = memory_order_seq_cst) volatile
    {
      // Redundant synchronize if built-in for lock is a full barrier.
      if (__m >= memory_order_release)
        __sync_synchronize();
      return __sync_lock_test_and_set(&_M_i, 1);
    }


__atomic2::atomic_flag::clear also issues an unwanted full memory barrier when
m == memory_order_relaxed, and in debug mode doesn't enforce the requirement
that m != acquire and m != acq_rel.  

It seems strange to me that clear() allows memory_order_consume but not
acquire. I'll ask on the lib reflector if that's an oversight, assuming it is,
I would write it:

    void
    clear(memory_order __m = memory_order_seq_cst) volatile
    {
      __glibcxx_assert(__m != memory_order_acquire);
      __glibcxx_assert(__m != memory_order_acq_rel);
      __sync_lock_release(&_M_i);
      if (__m == memory_order_seq_cst)
        __sync_synchronize();
    }

atomic_*::store() has no memory barrier when m == memory_order_release.

atomic_*::fetch() would benefit from an atomic load operation.

atomic_*::exchange() needs a release barrier when m >= memory_order_release.

N.B. I have not fully grokked the atomic operations library, so everything I've
said above might be wrong!


-- 


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

[Bug libstdc++/40297] [C++0x] debug mode vs atomics

[bkoz at gcc dot gnu dot org]

------- Comment #3 from bkoz at gcc dot gnu dot org  2009-06-18 00:33 -------

Jonathan, you are right. These assertions are all backwards. I see this hitting
the following members:

load
store
compare_exchange_strong

I should have done tests for this, obviously. Ouch. Now you've done this for
me, so yes proceed please. I'm still not sure that this kind of debug mode only
error handling is correct but it seems like an approach that is vaguely
familiar from other parts of the library. So at least usage is consistent, even
if the original implementation was plain wrong....

There are many problems with memory ordering in the atomics2 implementation. It
is known to be incorrect and incomplete, as the saying goes. The goal was to
start experimenting with compliler builtins assuming x86_64 hardware and see
how far we got, what kind of compiler intrinsics we needed first, how we test
this stuff, etc etc.


-- 


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

[Bug libstdc++/40297] [C++0x] debug mode vs atomics

[jwakely dot gcc at gmail dot com]

------- Comment #4 from jwakely dot gcc at gmail dot com  2009-06-18 12:43 -------
(In reply to comment #2)
> It seems strange to me that clear() allows memory_order_consume but not
> acquire. I'll ask on the lib reflector if that's an oversight,

I asked and everyone agreed it should disallow consume so Lawrence Crowl will
address it in a forthcoming paper.

(In reply to comment #3)
> me, so yes proceed please. I'm still not sure that this kind of debug mode only
> error handling is correct but it seems like an approach that is vaguely
> familiar from other parts of the library. So at least usage is consistent, even
> if the original implementation was plain wrong....

The approach seems fine to me, but then I'm a frequent user of _GLIBCXX_DEBUG
anyway.

> There are many problems with memory ordering in the atomics2 implementation. It
> is known to be incorrect and incomplete, as the saying goes. The goal was to
> start experimenting with compliler builtins assuming x86_64 hardware and see
> how far we got, what kind of compiler intrinsics we needed first, how we test
> this stuff, etc etc.

The current code is more than good enough to let me start experimenting and
feeling my way around so please take my comments as feedback not complaints :-)

I'll prepare a patch to reverse the sense of the debug assertions, but won't
change anything else.


-- 


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

[Bug libstdc++/40297] [C++0x] debug mode vs atomics

[redi at gcc dot gnu dot org]

-- 

redi at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |jwakely dot gcc at gmail dot
                   |dot org                     |com
             Status|UNCONFIRMED                 |ASSIGNED
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2009-06-18 12:48:21
               date|                            |


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

[Bug libstdc++/40297] [C++0x] debug mode vs atomics

[redi at gcc dot gnu dot org]

------- Comment #5 from redi at gcc dot gnu dot org  2009-06-24 07:06 -------
Subject: Bug 40297

Author: redi
Date: Wed Jun 24 07:06:17 2009
New Revision: 148893

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=148893
Log:
2009-06-24  Jonathan Wakely  <jwakely.gcc@gmail.com>

        PR libstdc++/40297
        * include/bits/atomic_0.h: Reverse debug assertions.
        * include/bits/atomic_2.h: Likewise.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/atomic_0.h
    trunk/libstdc++-v3/include/bits/atomic_2.h


-- 


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

[Bug libstdc++/40297] [C++0x] debug mode vs atomics

[redi at gcc dot gnu dot org]

------- Comment #6 from redi at gcc dot gnu dot org  2009-06-24 07:08 -------
Subject: Bug 40297

Author: redi
Date: Wed Jun 24 07:07:49 2009
New Revision: 148894

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=148894
Log:
2009-06-24  Jonathan Wakely  <jwakely.gcc@gmail.com>

        PR libstdc++/40297
        * include/bits/atomic_0.h: Reverse debug assertions.
        * include/bits/atomic_2.h: Likewise.

Modified:
    branches/gcc-4_4-branch/libstdc++-v3/ChangeLog
    branches/gcc-4_4-branch/libstdc++-v3/include/bits/atomic_0.h
    branches/gcc-4_4-branch/libstdc++-v3/include/bits/atomic_2.h


-- 


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

[Bug libstdc++/40297] [C++0x] debug mode vs atomics

[jwakely dot gcc at gmail dot com]

------- Comment #7 from jwakely dot gcc at gmail dot com  2009-06-24 07:09 -------
Fixed for 4.4.1 and 4.5.0


-- 

jwakely dot gcc at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


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