Unbork `std::memory_order` ABI.

[EricWF]

Summary

We need to pin the underlying type of C++20' std::memory_order to match the C++17 version. Anything less is an ABI break.

At the moment it's unsigned before C++20 and int after. Or if you're using -fshort-enums it's unsigned char before C++20 and int after.

This patch explicitly specifies the underlying type of the C++20 memory_order to be w/e type the compiler would have chosen for the C++17 version.

[mclow.lists]

Other than naming nits, this looks like what I came up with. This resolves my compatibility objections. It's ugly, though. (not your fault)

include/atomic

591

if it were me, I'd give them real names, and assign them below.

__mo_relaxed, .....

603–609

relaxed = __mo_relaxed, ...

621–622

memory_order_relaxed = mo_relaxed ....

[zoecarver]

LGTM other than giving them real names as @mclow.lists suggested. Another option (not sure if it's better or worse) might be using a namespace for __legacy_memory_order so there wouldn't be duplicate enums. Something like this:

namespace __legacy_memory_order {
	enum memory_order { memory_order_relaxed, memory_order_consume, memory_order_acquire, ... };
};

#if over 17

// enum class based on `underlying_type<__legacy_memory_order::memory_order>::type`

#else

using namespace __legacy_memory_order;

#endif

Might have unintended consequences though.

[zoecarver]

include/atomic

632–633

I know I wrote this but what is libc++'s stance on things like underlying_type::type vs underlying_type_t or does it even matter?

[mclow.lists]

include/atomic

632–633

I prefer the _t variants; but they're only available in C++17 or later. So use them in new code, but not for stuff that has to run back to 03.

When in doubt, use the ::type version.

[EricWF]

In D59063#1420886, @zoecarver wrote:

LGTM other than giving them real names as @mclow.lists suggested. Another option (not sure if it's better or worse) might be using a namespace for __legacy_memory_order so there wouldn't be duplicate enums. Something like this:

namespace __legacy_memory_order {
	enum memory_order { memory_order_relaxed, memory_order_consume, memory_order_acquire, ... };
};

using namespace __legacy_memory_order;

I had the same idea, but that changes the mangled name of memory_resource which is an ABI break.

include/atomic

632–633

<atomic> is C++11, the _t traits are C++14. We can't use them here.

[zoecarver]

include/atomic

632–633

Ah okay. Thanks for the explanation.

[ldionne]

This patch is roughly what I suggested in https://reviews.llvm.org/D59029#1420019. I'm OK with this, but it's a lot more complicated than I'd wish.

[EricWF]

Address review comments.

include/atomic

 
 #define _LIBCPP_CHECK_EXCHANGE_MEMORY_ORDER(__m, __f) \
   _LIBCPP_DIAGNOSE_WARNING(__f == memory_order_release || \
                            __f == memory_order_acq_rel,   \
                         "memory order argument to atomic operation is invalid")
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
+// Figure out what the underlying type for `memory_order` would be if it were
+// declared as an unscoped enum (accounting for -fshort-enums). Use this result
+// to pin the underlying type in C++20.
+enum __legacy_memory_order {
+    __mo_relaxed,
+    __mo_consume,
+    __mo_acquire,
+    __mo_release,
+    __mo_acq_rel,
+    __mo_seq_cst
+};
+
+typedef underlying_type<__legacy_memory_order>::type __memory_order_underlying_t;
+
 #if _LIBCPP_STD_VER > 17
 
-enum class memory_order {
-  relaxed, consume, acquire, release, acq_rel, seq_cst
+enum class memory_order : __memory_order_underlying_t {
+  relaxed = __mo_relaxed,
+  consume = __mo_consume,
+  acquire = __mo_acquire,
+  release = __mo_release,
+  acq_rel = __mo_acq_rel,
+  seq_cst = __mo_seq_cst
 };
 
 inline constexpr auto memory_order_relaxed = memory_order::relaxed;
 inline constexpr auto memory_order_consume = memory_order::consume;
 inline constexpr auto memory_order_acquire = memory_order::acquire;
 inline constexpr auto memory_order_release = memory_order::release;
 inline constexpr auto memory_order_acq_rel = memory_order::acq_rel;
 inline constexpr auto memory_order_seq_cst = memory_order::seq_cst;
 
 #else
 
 typedef enum memory_order {
-  memory_order_relaxed, memory_order_consume, memory_order_acquire,
-  memory_order_release, memory_order_acq_rel, memory_order_seq_cst
+  memory_order_relaxed = __mo_relaxed,
+  memory_order_consume = __mo_consume,
+  memory_order_acquire = __mo_acquire,
+  memory_order_release = __mo_release,
+  memory_order_acq_rel = __mo_acq_rel,
+  memory_order_seq_cst = __mo_seq_cst,
 } memory_order;
 
 #endif // _LIBCPP_STD_VER > 17
 
-typedef underlying_type<memory_order>::type __memory_order_underlying_t;
-
+static_assert(is_same<underlying_type<memory_order>::type, __memory_order_underlying_t>::value,
+  "unexpected underlying type for std::memory_order");
 
 #if defined(_LIBCPP_HAS_GCC_ATOMIC_IMP) || \
   defined(_LIBCPP_ATOMIC_ONLY_USE_BUILTINS)
 
 // [atomics.types.generic]p1 guarantees _Tp is trivially copyable. Because
 // the default operator= in an object is not volatile, a byte-by-byte copy
 // is required.
 template <typename _Tp, typename _Tv> _LIBCPP_INLINE_VISIBILITY