Bug 40297 - [C++0x] debug mode vs atomics
Summary: [C++0x] debug mode vs atomics
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 4.5.0
: P3 normal
Target Milestone: ---
Assignee: Jonathan Wakely
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-29 17:37 UTC by Jonathan Wakely
Modified: 2009-06-24 07:09 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2009-06-18 12:48:21


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Wakely 2009-05-29 17:37:48 UTC
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!
Comment 1 Paolo Carlini 2009-06-04 15:16:19 UTC
Let's CC Benjamin...
Comment 2 Jonathan Wakely 2009-06-16 12:35:08 UTC
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!
Comment 3 Benjamin Kosnik 2009-06-18 00:33:02 UTC
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.



Comment 4 Jonathan Wakely 2009-06-18 12:43:30 UTC
(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.
Comment 5 Jonathan Wakely 2009-06-24 07:06:34 UTC
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

Comment 6 Jonathan Wakely 2009-06-24 07:08:03 UTC
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

Comment 7 Jonathan Wakely 2009-06-24 07:09:06 UTC
Fixed for 4.4.1 and 4.5.0