Page MenuHomePhabricator

Unbork `std::memory_order` ABI.
ClosedPublic

Authored by EricWF on Mar 6 2019, 4:53 PM.

Details

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.

Diff Detail

Repository
rCXX libc++

Event Timeline

EricWF created this revision.Mar 6 2019, 4:53 PM

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

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 added inline comments.Mar 6 2019, 5:41 PM
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 added inline comments.Mar 6 2019, 7:22 PM
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 marked an inline comment as done.Mar 6 2019, 7:56 PM

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 added inline comments.Mar 6 2019, 8:05 PM
include/atomic
632–633

Ah okay. Thanks for the explanation.

ldionne accepted this revision.Mar 7 2019, 6:47 AM

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.

This revision is now accepted and ready to land.Mar 7 2019, 6:47 AM
EricWF updated this revision to Diff 189947.Mar 8 2019, 3:05 PM

Address review comments.

This revision was automatically updated to reflect the committed changes.