Page MenuHomePhabricator

Implement is_always_lock_free
ClosedPublic

Authored by jfb on Mar 7 2016, 6:19 PM.

Details

Summary

This was voted into C++17 at last week's Jacksonville meeting. The final P0152R1 paper will be in the upcoming post-Jacksonville mailing, and is also available here:

http://jfbastien.github.io/papers/P0152R1.html

Diff Detail

Repository
rL LLVM

Event Timeline

jfb updated this revision to Diff 50017.Mar 7 2016, 6:19 PM
jfb retitled this revision from to Implement is_always_lock_free.
jfb updated this object.
jfb added reviewers: mclow.lists, rsmith.
jfb added a subscriber: cfe-commits.
jfb added inline comments.Mar 7 2016, 6:26 PM
include/atomic
840 ↗(On Diff #50017)

Moving these from the bottom of the file since I need them.

887 ↗(On Diff #50017)

This is slightly ugly, but I don't see a better way to go about without breaking libc++ when it's not exactly in sync with clang, or when it's used with GCC (until GCC picks up these macros).

test/std/atomics/atomics.lockfree/lockfree.pass.cpp
23 ↗(On Diff #50017)

BOOL and POINTER weren't tested, I'm adding them while I'm here.

bcraig added a subscriber: bcraig.Mar 16 2016, 7:29 AM
bcraig added inline comments.
include/atomic
859 ↗(On Diff #50017)

Do we need to support configurations were wchar_t is a typedef to an integer type, or is that an abomination too painful to deal with?

I have no idea if the rest of libcxx attempts to deal with wchar_t typedefs.

jfb added inline comments.Mar 16 2016, 10:04 AM
include/atomic
859 ↗(On Diff #50017)

Do you have examples of such platforms? The standard is pretty clear that they're wrong:

[basic.fundamental]

Type wchar_t is a distinct type whose values can represent distinct codes for all members of the largest extended character set specified among the supported locales. Type wchar_t shall have the same size, signedness, and alignment requirements as one of the other integral types, called its *underlying type*.

I'll defer to @mclow.lists on whether we can about such implementations or not.

EricWF added inline comments.Mar 16 2016, 10:19 AM
include/atomic
859 ↗(On Diff #50017)

I don't think we have to deal with such typedefs for wchar_t but we do have to deal with non-sense typedefs (that we unfortunately supply) for char16_t and char32_t in C++03 mode with GCC.

And our <atomic> implementation needs to compile in C++03 (Sorry, I did that).

898 ↗(On Diff #50017)

@mclow.lists I'm fine exposing this typedef retroactively in C++11 and earlier. Do you have any objections to that?

jfb added inline comments.Mar 16 2016, 10:25 AM
include/atomic
859 ↗(On Diff #50017)

__cpp_lib_atomic_is_always_lock_free takes care of it: it's only defined in C++17 and later so <atomic> should compile just fine.

Is there a test that would catch C++03 breakage?

898 ↗(On Diff #50017)

Which typedef do you mean? Looks like phabricator may have tagged the wrong line.

EricWF added inline comments.Mar 16 2016, 10:32 AM
include/atomic
859 ↗(On Diff #50017)

Ah I didn't realize that. We have a C++03 bot that will catch any breakage but if your only exposing this in C++17 then we are fine.

898 ↗(On Diff #50017)

Sorry I haven't had my coffee yet :-S. I meant the "is_always_lock_free" variable. IDK why I said typedef. However I missed that this is already only exposed in C++17 and beyond. That's even better.

test/std/atomics/atomics.lockfree/lockfree.pass.cpp
66 ↗(On Diff #50017)

This needs to be guarded by #if TEST_STD_VER > 14. The macro lives in test_macros.h.

EricWF edited edge metadata.Mar 16 2016, 10:34 AM

@jfb: Actually I changed my mind about guarding the new tests. Can you move the new tests (other than the missing additions) to a new test file? Then add // UNSUPPORTED: c++98, c++03, c++11, c++14 to the top of it?

Visual Studio has a flag, /Zc:wchar_t-, that turns wchar_t into a short. This flag is for people that need to maintain ABI compatibility with ancient Visual Studios (MSVC6? MSVC5?). It is definitely non-conformant, and many things support it poorly.

I still have nightmares of supporting boost::filesystem users where boost::filesystem was built with real wchar_t, but clients tried to use it with wchar_t as short. The linker generally disapproved of those shenanigans.

I'm totally fine if libcxx doesn't try to support this situation... but if it did want to support it, it would need to selectively not define wchar_t.

Visual Studio has a flag, /Zc:wchar_t-, that turns wchar_t into a short. This flag is for people that need to maintain ABI compatibility with ancient Visual Studios (MSVC6? MSVC5?). It is definitely non-conformant, and many things support it poorly.

I still have nightmares of supporting boost::filesystem users where boost::filesystem was built with real wchar_t, but clients tried to use it with wchar_t as short. The linker generally disapproved of those shenanigans.

I'm totally fine if libcxx doesn't try to support this situation... but if it did want to support it, it would need to selectively not define wchar_t.

+1 for libcxx not supporting that. I think there are plenty of places where this is already broken in libc++.

jfb added a comment.Mar 16 2016, 12:43 PM

Visual Studio has a flag, /Zc:wchar_t-, that turns wchar_t into a short. This flag is for people that need to maintain ABI compatibility with ancient Visual Studios (MSVC6? MSVC5?). It is definitely non-conformant, and many things support it poorly.

I still have nightmares of supporting boost::filesystem users where boost::filesystem was built with real wchar_t, but clients tried to use it with wchar_t as short. The linker generally disapproved of those shenanigans.

I'm totally fine if libcxx doesn't try to support this situation... but if it did want to support it, it would need to selectively not define wchar_t.

That would be pre-C++ standardization (*any* standard) code trying to interact with C++17. I'm inclined to say we shouldn't care ;-)

jfb updated this revision to Diff 50846.Mar 16 2016, 12:58 PM
jfb edited edge metadata.
  • Split up tests.
jfb added inline comments.Mar 16 2016, 1:38 PM
include/atomic
900 ↗(On Diff #50846)

Actually, this should be based on sizeof(__a_) and *not* sizeof(_Tp).

jfb updated this revision to Diff 50903.Mar 16 2016, 6:39 PM
  • Size is based on __a_ not _Tp
  • Check value is consistent with C macros
jfb updated this revision to Diff 51049.Mar 18 2016, 11:32 AM
  • Use __atomic_always_lock_free instead
jfb updated this object.Mar 22 2016, 2:21 PM

As discussed in now-abandoned D17950 I'll implement everything in libc++. I just have to move the feature test macro to libc++.

Re-building clang now so I can test the most recent changes.

include/atomic
850 ↗(On Diff #51049)

I think this needs a definition.

jfb updated this revision to Diff 51346.Mar 22 2016, 2:47 PM
  • Define __cpp_lib_atomic_is_always_lock_free in libc++
jfb added inline comments.Mar 23 2016, 1:45 PM
include/atomic
859 ↗(On Diff #51346)

I don't think so: it's similar to std::integral_constant<_Tp, __v>::value.

EricWF added inline comments.Mar 23 2016, 1:48 PM
include/atomic
859 ↗(On Diff #51346)

integral_constant::value has a definition on line 481 of type_traits.

jfb updated this revision to Diff 51473.Mar 23 2016, 2:07 PM
  • Add definition.
include/atomic
859 ↗(On Diff #51346)

Ha! I'd totally missed that. That explains why I was confused :-)

Added, thanks!

EricWF accepted this revision.Mar 24 2016, 2:44 PM
EricWF edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Mar 24 2016, 2:44 PM
jfb updated this revision to Diff 51607.Mar 24 2016, 2:53 PM
jfb edited edge metadata.
  • Check feature test macro exists
This revision was automatically updated to reflect the committed changes.