Closed Bug 898491 Opened 6 years ago Closed 6 years ago

work around a bug in some versions of GCC's <atomic> with compare-and-exchange

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: froydnj, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
The C++ standard (29.6p18-21 in N3126) specifies limitations on the failure ordering
for atomic compare-and-exchange.  Specifically, you can't pass memory_order_acq_rel or
memory_order_release.  For the (T&, T, std::memory_order) version, which we use, the
standard specifies that the provided argument should be "lowered" to comply with the
above restrictions on the failure ordering (29.6p20).

However, it seems that some versions of GCC's <atomic> header don't follow the spec
for the generic versions of std::atomic<>, though they do follow the spec with the
appropriate specializations (bool, integer, and pointer) of std::atomic<>.  This
results in mysterious failures when using atomic enums, as bug 888548 purports to
do, and ReleaseAcquire ordering.

Happily, we can work around this by using the more explicit version of
compare-and-exchange.  I've chosen to add another member to AtomicOrderConstraints,
even though it'd be the same as LoadOrder.  I feel explicitness is to be preferred
here.
Attachment #781759 - Flags: review?(jwalden+bmo)
Comment on attachment 781759 [details] [diff] [review]
work around a bug in some versions of GCC's <atomic> with compare-and-exchange

Review of attachment 781759 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Atomics.h
@@ +211,5 @@
>      static const std::memory_order AtomicRMWOrder = std::memory_order_seq_cst;
>      static const std::memory_order LoadOrder = std::memory_order_seq_cst;
>      static const std::memory_order StoreOrder = std::memory_order_seq_cst;
> +    static const std::memory_order CompareExchangeFailureOrder =
> +      std::memory_order_seq_cst

Duh, missing semicolon.  I will fix this before commit.
Blocks: 888548
(Apparently I can't git commit properly before posting patches.)

The C++ standard (29.6p18-21 in N3126) specifies limitations on the failure ordering
for atomic compare-and-exchange.  Specifically, you can't pass memory_order_acq_rel or
memory_order_release.  For the (T&, T, std::memory_order) version, which we use, the
standard specifies that the provided argument should be "lowered" to comply with the
above restrictions on the failure ordering (29.6p20).

However, it seems that some versions of GCC's <atomic> header don't follow the spec
for the generic versions of std::atomic<>, though they do follow the spec with the
appropriate specializations (bool, integer, and pointer) of std::atomic<>.  This
results in mysterious failures when using atomic enums, as bug 888548 purports to
do, and ReleaseAcquire ordering.

Happily, we can work around this by using the more explicit version of
compare-and-exchange.  I've chosen to add another member to AtomicOrderConstraints,
even though it'd be the same as LoadOrder.  I feel explicitness is to be preferred
here.
Attachment #781759 - Attachment is obsolete: true
Attachment #781759 - Flags: review?(jwalden+bmo)
Attachment #782556 - Flags: review?(jwalden+bmo)
Comment on attachment 782556 [details] [diff] [review]
use the four argument form of compare_exchange_strong (v2)

Review of attachment 782556 [details] [diff] [review]:
-----------------------------------------------------------------

Seeing as this accords with the "When only one memory_order argument is supplied" sentence, and what it says will be used for the value of the |failure| argument, this seems fine.

Note that http://stackoverflow.com/questions/81656/where-do-i-find-the-current-c-or-c-standard-documents includes links to N3337, which is apparently the final standard plus a few editorial tweaks, so you probably want to grab a new copy to use.

::: mfbt/Atomics.h
@@ +180,5 @@
>  namespace detail {
>  
> +/*
> + * We provide CompareExchangeFailureOrder to work around a bug in some
> + * versions of GCC's <atomic> header.

Probably worth a "See bug 898491." here.
Attachment #782556 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/26846fdeeb41
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.