888548 - Support Atomic<T> where T is an enum

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Justin Lebar (not reading bugmail)]

We discussed this on IRC.  It would be nice to be able to use Atomic<T> where T is an enum.  See for example bug 876029 comment 20.

Comment 1

[Biru [:poiru]]

Attachment: Part 1: Add mozilla::IsEnum<T> to TypeTraits.h

This uses the __is_enum(T) compiler intrinsic, which seems to be supported on all our target platforms. See: https://tbpl.mozilla.org/?tree=Try&rev=e2098937ac81

Comment 2

[Biru [:poiru]]

Attachment: Part 1: Add mozilla::IsEnum to TypeTraits.h

Comment 3

[Biru [:poiru]]

Attachment: Part 2: Make mozilla::Atomic<T> use mozilla::EnableIf for specializations

Comment 4

[Biru [:poiru]]

Attachment: Part 3: Add enum support to mozilla::Atomic<T>

For some odd reason, this patch causes the following error with GCC. Clang and MSVC work fine.

In file included from ../../dist/include/mozilla/Atomics.h:176:0,
                 from ../../../mfbt/tests/TestAtomics.cpp:6:
/tools/gcc-4.7.2-0moz1/lib/gcc/x86_64-unknown-linux-gnu/4.7.2/../../../../include/c++/4.7.2/atomic: In function 'int main()':
/tools/gcc-4.7.2-0moz1/lib/gcc/x86_64-unknown-linux-gnu/4.7.2/../../../../include/c++/4.7.2/atomic:259:69: error: invalid failure memory model for '__atomic_compare_exchange'
/tools/gcc-4.7.2-0moz1/lib/gcc/x86_64-unknown-linux-gnu/4.7.2/../../../../include/c++/4.7.2/atomic:259:69: error: invalid failure memory model for '__atomic_compare_exchange'

See: https://tbpl.mozilla.org/?tree=Try&rev=ba498905e72f


GCC is able to compile TestAtomics.cpp with the following changes. I do not know if there exists a general solution for GCC (other than not defining MOZ_HAVE_CXX11_ATOMICS for it). Any ideas?

@@ -128,16 +128,20 @@
   MOZ_ASSERT(atomic == array1 + 3, "CAS should have changed atomic's value.");
 }
 
+enum EnumType {
+  EnumType_0 = 0,
+  EnumType_1 = 1,
+  EnumType_2 = 2,
+  EnumType_3 = 3
+};
+
 template<MemoryOrdering Order>
 static void
 TestEnumWithOrdering()
 {
-  enum EnumType {
-    EnumType_0 = 0,
-    EnumType_1 = 1,
-    EnumType_2 = 2,
-    EnumType_3 = 3
-  };
+  std::atomic<EnumType> dummyStdAtomic;
+  EnumType expected = EnumType_0;
+  dummyStdAtomic.compare_exchange_strong(expected, EnumType_1);
 
   Atomic<EnumType, Order> atomic(EnumType_2);
   MOZ_ASSERT(atomic == EnumType_2, "Atomic variable did not initialize");

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 778943 [details] [diff] [review]
Part 1: Add mozilla::IsEnum to TypeTraits.h

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

::: mfbt/TypeTraits.h
@@ +129,5 @@
> +namespace detail {
> +
> +template<typename T>
> +struct IsEnumHelper
> +  : IntegralConstant<bool, __is_enum(T)>

I can see the helpfulness of a comment here saying:

// __is_enum is a supported extension across all of our supported compilers.

On the other hand, I can see how people might say this is redundant, given that the code clearly compiles. :)  Up to you.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 778944 [details] [diff] [review]
Part 2: Make mozilla::Atomic<T> use mozilla::EnableIf for specializations

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

::: mfbt/Atomics.h
@@ +885,5 @@
>  {
> +  private:
> +    Atomic() MOZ_DELETE;
> +    Atomic(Atomic<T, Order>& aOther) MOZ_DELETE;
> +};

It would be better to simply not declare a class here at all; compiler error messages should be slightly more clear then.

@@ +908,5 @@
>  
> +    T operator++(int) { return Base::Intrinsics::inc(Base::mValue); }
> +    T operator--(int) { return Base::Intrinsics::dec(Base::mValue); }
> +    T operator++() { return Base::Intrinsics::inc(Base::mValue) + 1; }
> +    T operator--() { return Base::Intrinsics::dec(Base::mValue) - 1; }

These should reside in a new detail::AtomicBaseIncDec or similar.  Atomic<integer-type> and Atomic<T*> can then inherit from detail::AtomicBaseIncDec (and ideally not be too uglified).

@@ +926,5 @@
>      Atomic(Atomic<T, Order>& aOther) MOZ_DELETE;
>  };
>  
>  /**
> + * Atomic<T> implementation for pointer types.

Why are you changing this to use EnableIf rather than just letting the template specialization mechanism work?

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 778945 [details] [diff] [review]
Part 3: Add enum support to mozilla::Atomic<T>

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

This looks reasonable to me, though |T operator=(T a)| should probably be moved into AtomicBase as a followup.

I'm not sure what's wrong with GCC 4.7; the behavior there looks like a bug.  Unfortunately, that bug is going to prevent us from landing this patch. :(  I see two ways forward:

1) Don't permit compareExchange on Atomic<enum>.  Move it into a common base shared by Atomic<integral> and Atomic<pointer> instead.  The suggested AtomicBaseIncDec from part 2 would be a great place for it, albeit with an XXX comment that indicates we know about the wrongness of putting it there, but there's nothing we can do currently.
2) File upstream bug report, get it fixed in the next 4.7.x, and upgrade to that on the builders (or backport the fix).

These are not mutually exclusive.  I will try reducing the testcase to file the bug.

f+'ing instead of r+'ing so this patch doesn't get landed accidentally.  Will r+ once we figure out a satisfactory way forward.

Comment 8

[Biru [:poiru]]

Attachment: Part 1: Add mozilla::IsEnum to TypeTraits.h

Comment 9

[Biru [:poiru]]

Attachment: Part 2: Refactor and cleanup mozilla::Atomic<T> implementation

Addresses comment #6.

Comment 10

[Biru [:poiru]]

Attachment: Add enum support to mozilla::Atomic<T>

This addresses comment #7 and does not permit compareExchange on Atomic<enum T> for now.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ae599871fdf6

By the way, it may be possible to change `T operator=(T aValue) { return Base::operator=(aValue); }` to simply `using Base::operator=;`, but I don't know if all our compilers support it. I can test on try if you want.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 779845 [details] [diff] [review]
Part 2: Refactor and cleanup mozilla::Atomic<T> implementation

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

Looks great, thanks for the changes!

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 779850 [details] [diff] [review]
Add enum support to mozilla::Atomic<T>

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

r=me with or without the |using Base::operator=| bit (though I'd think the normal rules of function lookup would find the operator in the base class?).

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 779850 [details] [diff] [review]
Add enum support to mozilla::Atomic<T>

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

We may run into issues here if this gets used with enums that the compiler doesn't back by a size-4 (or on 64-bit, size-8) integer type.  I can't quite remember the C++ rules well enough to know what it'd take for that to happen.

If you don't declare a copy assignment operator explicitly, then one is declared implicitly by the compiler.  In this case the implicitly declared version would have the default implementation.  A |using Base::operator=;| or explicitly declaring one gets around that.  C++ is fun.  See C++11 12.8 p17/18, i.e. [class.copy]p17-18.

::: mfbt/Atomics.h
@@ +985,5 @@
> +    typedef typename detail::AtomicBase<T, Order> Base;
> +
> +  public:
> +    Atomic() : detail::AtomicBase<T, Order>() {}
> +    Atomic(T aInit) : detail::AtomicBase<T, Order>(aInit) {}

If you're going to typedef Base, you might as well use it for both of these, seems to me.

Comment 14

[Jeff Walden [:Waldo]]

Hmm, guess the Base bit's in the existing classes as well.  Someone should driveby-cleanup that sometime, or something.

Comment 15

[Biru [:poiru]]

Attachment: Part 4: Cleanup Atomic<T>

Changed to use |using Base::operator=| since it compiled fine on try. This also addresses Waldo's suggestion in comment #13.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 780390 [details] [diff] [review]
Part 4: Cleanup Atomic<T>

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

Thanks for the cleanup!

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Birunthan Mohanathas [:poiru] from comment #4)
> Created attachment 778945 [details] [diff] [review]
> Part 3: Add enum support to mozilla::Atomic<T>
> 
> For some odd reason, this patch causes the following error with GCC. Clang
> and MSVC work fine.
> 
> In file included from ../../dist/include/mozilla/Atomics.h:176:0,
>                  from ../../../mfbt/tests/TestAtomics.cpp:6:
> /tools/gcc-4.7.2-0moz1/lib/gcc/x86_64-unknown-linux-gnu/4.7.2/../../../../
> include/c++/4.7.2/atomic: In function 'int main()':
> /tools/gcc-4.7.2-0moz1/lib/gcc/x86_64-unknown-linux-gnu/4.7.2/../../../../
> include/c++/4.7.2/atomic:259:69: error: invalid failure memory model for
> '__atomic_compare_exchange'
> /tools/gcc-4.7.2-0moz1/lib/gcc/x86_64-unknown-linux-gnu/4.7.2/../../../../
> include/c++/4.7.2/atomic:259:69: error: invalid failure memory model for
> '__atomic_compare_exchange'
> 
> See: https://tbpl.mozilla.org/?tree=Try&rev=ba498905e72f
> 
> 
> GCC is able to compile TestAtomics.cpp with the following changes. I do not
> know if there exists a general solution for GCC (other than not defining
> MOZ_HAVE_CXX11_ATOMICS for it). Any ideas?

This is a bug in GCC's <atomic> header.  Once bug 898491 goes in, could you please amend your patches to provide compareExchange for atomics and uncomment the relevant testcases?

Comment 18

[Biru [:poiru]]

Attachment: Part 2: Refactor and cleanup mozilla::Atomic<T> implementation

Comment 19

[Biru [:poiru]]

Attachment: Part 3: Add enum support to mozilla::Atomic<T>

(In reply to Nathan Froyd (:froydnj) from comment #17)
> This is a bug in GCC's <atomic> header.  Once bug 898491 goes in, could you
> please amend your patches to provide compareExchange for atomics and
> uncomment the relevant testcases?

Done.

Comment 20

[Nathan Froyd [:froydnj]]

Comment on attachment 783661 [details] [diff] [review]
Part 3: Add enum support to mozilla::Atomic<T>

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

r=me with the changes below.

::: mfbt/Atomics.h
@@ +989,5 @@
> + * The atomic store and load operations and the atomic swap method is provided.
> + */
> +template<typename T, MemoryOrdering Order>
> +class Atomic<T, Order, typename EnableIf<IsEnum<T>::value>::Type>
> +  : public detail::AtomicBaseIncDec<T, Order>

This should be inheriting from AtomicBase; we don't want to support increment and decrement on enums.

@@ +991,5 @@
> +template<typename T, MemoryOrdering Order>
> +class Atomic<T, Order, typename EnableIf<IsEnum<T>::value>::Type>
> +  : public detail::AtomicBaseIncDec<T, Order>
> +{
> +    typedef typename detail::AtomicBaseIncDec<T, Order> Base;

AtomicBase here as well.

Comment 21

[Biru [:poiru]]

Attachment: Part 3: Add enum support to mozilla::Atomic<T>

(In reply to Nathan Froyd (:froydnj) from comment #20)
> This should be inheriting from AtomicBase; we don't want to support
> increment and decrement on enums.

Heh, I guess I shouldn't write code when sleep deprived. Fixed now, thanks.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e31542e117c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f607ac59de19
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc98067f0aa4

Comment 23

[Daniel Holbert [:dholbert]]

This broke B2G builds, with errors like:
{
18:50:51    ERROR -  ../../../gecko/mfbt/tests/TestAtomics.cpp:142: error: template argument for 'template<class T, mozilla::MemoryOrdering Order, class Enable> struct mozilla::Atomic' uses local type 'TestEnumWithOrdering() [with mozilla::MemoryOrdering Order = (mozilla::MemoryOrdering)2u]::EnumType'
18:50:51    ERROR -  ../../../gecko/mfbt/tests/TestAtomics.cpp:142: error:   trying to instantiate 'template<class T, mozilla::MemoryOrdering Order, class Enable> struct mozilla::Atomic'
18:50:51    ERROR -  ../../../gecko/mfbt/tests/TestAtomics.cpp:146: error: template argument for 'template<class T> class mozilla::DebugOnly' uses local type 'TestEnumWithOrdering() [with mozilla::MemoryOrdering Order = (mozilla::MemoryOrdering)2u]::EnumType'
18:50:51    ERROR -  ../../../gecko/mfbt/tests/TestAtomics.cpp:146: error:   trying to instantiate 'template<class T> class mozilla::DebugOnly'
18:50:51    ERROR -  ../../../gecko/mfbt/tests/TestAtomics.cpp:142: error: template argument for 'template<class T, mozilla::MemoryOrdering Order, class Enable> struct mozilla::Atomic' uses local type 'TestEnumWithOrdering() [with mozilla::MemoryOrdering Order = (mozilla::MemoryOrdering)2u]::EnumType'
18:50:51    ERROR -  ../../../gecko/mfbt/tests/TestAtomics.cpp:142: error:   trying to instantiate 'template<class T, mozilla::MemoryOrdering Order, class Enable> struct mozilla::Atomic'
18:50:51    ERROR -  ../../../gecko/mfbt/tests/TestAtomics.cpp:146: error: template argument for 'template<class T> class mozilla::DebugOnly' uses local type 'TestEnumWithOrdering() [with mozilla::MemoryOrdering Order = (mozilla::MemoryOrdering)2u]::EnumType'
18:50:51    ERROR -  ../../../gecko/mfbt/tests/TestAtomics.cpp:146: error:   trying to instantiate 'template<class T> class mozilla::DebugOnly'
}
https://tbpl.mozilla.org/php/getParsedLog.php?id=25999956&tree=Mozilla-Inbound

For reference, here's the inbound cycle where this landed (with the busted builds):
 https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=bb8539a50e37

I backed it out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff607d314da1
https://hg.mozilla.org/integration/mozilla-inbound/rev/b045a3eb7507
https://hg.mozilla.org/integration/mozilla-inbound/rev/19c4b977342b

The

Comment 24

[Daniel Holbert [:dholbert]]

(sorry, disregard the trailing "The" in prev. comment)

Comment 25

[Jeff Walden [:Waldo]]

Comment 23 sounds like we're not compiling as C++11 there.  In C++98 templates couldn't be instantiated parametrized by types without linkage -- including non-namespace-level defined types.  In C++11 you can use local types defined inside function bodies.

I'd thought this not-compiling-as-C++11 thing was bug 895915, but this landed after that, so I dunno.  The easy workaround is just to move the various enums to top level, and we should probably do that in the short run.  In the longer run we should figure out what's up here and fix it.  (Could just be the b2g compiler's too old to support template types without linkage, of course, in which case we should be aware of the issue for next time.)

Comment 26

[Mike Hommey [:glandium]]

The log is clear: the failing command has -std=gnu++0x on the command line. And it's not a host build, it's a target build, so that would be bug 894242 instead of bug 895915.

Anyways, it's likely one of the many weaknesses of gcc 4.4 in the C++0x department. I'd be happier if b2g could just switch to gcc 4.7 already...

Comment 27

[Biru [:poiru]]

Attachment: Part 3: Add enum support to mozilla::Atomic<T>

(In reply to Daniel Holbert [:dholbert] from comment #23)
> This broke B2G builds, with errors like:

Updated patch to fix this.

Comment 28

[Justin Lebar (not reading bugmail)]

> I'd be happier if b2g could just switch to gcc 4.7 already...

We're going to be stuck with what we have until we're done with 1.1...

Comment 29

[Ryan VanderMeulen [:RyanVM]]

(In reply to Justin Lebar [:jlebar] from comment #28)
> We're going to be stuck with what we have until we're done with 1.1...

Why is 1.1 affected by what compiler we use on trunk/1.2?

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbce2a590a2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/99511f741225
https://hg.mozilla.org/integration/mozilla-inbound/rev/424e1cb4850c

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dbce2a590a2b
https://hg.mozilla.org/mozilla-central/rev/99511f741225
https://hg.mozilla.org/mozilla-central/rev/424e1cb4850c