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

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Comment 1

[Nathan Froyd [:froydnj]]

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

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.

Comment 2

[Nathan Froyd [:froydnj]]

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.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: use the four argument form of compare_exchange_strong (v2)

(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.

Comment 4

[Jeff Walden [:Waldo]]

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.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26846fdeeb41