This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Add atomic_support.h header to src that handles needed atomic operations.
ClosedPublic

Authored by EricWF on Jun 11 2015, 5:48 PM.

Details

Summary

In some places in libc++ we need to use the __atomic_* builtins. This patch adds a header that provides access to those builtins in a uniform way from within the dylib source.

If the compiler building the dylib does not support these builtins then a warning is issued.

Only relaxed loads are needed within the headers. A singe function to do these relaxed loads has been added to <memory>.

This patch applies the new atomic builtins to __shared_count and call_once.

Diff Detail

Event Timeline

EricWF updated this revision to Diff 27557.Jun 11 2015, 5:48 PM
EricWF retitled this revision from to [libcxx] Add __atomic header that handles needed atomic operations..
EricWF updated this object.
EricWF edited the test plan for this revision. (Show Details)
EricWF added a reviewer: mclow.lists.
EricWF added a subscriber: Unknown Object (MLST).
jroelofs added inline comments.
include/__atomic
85

Should these fall back on the __sync_* ones instead of just dropping the atomicness? It seems a little precarious to silently default to non-atomic implementations for these (unless this is for a single-threaded build of the library).

Respond to @jroelofs comments.

include/__atomic
85

Yes and no. The only atomic operations needed in the headers are relaxed atomic loads. AFAIK all loads are relaxed atomic loads on x86 so not using atomic builtins isn't a big deal (its what we already do).

The rest of the atomic operations happen within the dylib. I agree we don't want to drop the atomicness on these unless it is a single threaded build.

What we really should do is limit the libc++ dylib build to compilers that provide the required atomic intrinsics (single-threaded builds not withstanding). If the compiler is not supported then the build should fail. For GCC and Clang the required versions would be 4.7 and 3.3 (possibly lower) respectively.

Since most of the stuff in __atomic is only used in the dylib build perhaps it should be moved out of the shipped headers. I'll figure out how to handle that tomorrow.

jroelofs added inline comments.Jun 12 2015, 8:58 AM
include/__atomic
85

AFAIK all loads are relaxed atomic loads on x86 so not using atomic builtins isn't a big deal

What about PPC-darwin?

What we really should do is limit the libc++ dylib build to compilers that provide the required atomic intrinsics (single-threaded builds not withstanding)

Agreed.

majnemer added inline comments.
src/mutex.cpp
255–267

Can you please leave a note that says that these relaxed stores pair with the relaxed load in the header? The atomic nature of the stores look superfluous because they are performed under the mutex.

EricWF updated this revision to Diff 27619.Jun 12 2015, 5:18 PM
EricWF retitled this revision from [libcxx] Add __atomic header that handles needed atomic operations. to [libcxx] Add atomic_support.h header to src that handles needed atomic operations..
EricWF updated this object.

Remove the __atomic header and put the contents in the source files. Enforce that we have atomics when building libc++.

EricWF updated this revision to Diff 27621.Jun 12 2015, 5:23 PM

Address @majnemer's request for a comment.

mclow.lists edited edge metadata.Jun 22 2015, 6:20 PM

I really like that this stuff (support/atomic) is only used in the dylib.

Does this patch fix: https://llvm.org/bugs/show_bug.cgi?id=22803 ?

And do we have an outstanding bug against call_once ?

I really like that this stuff (support/atomic) is only used in the dylib.

Does this patch fix: https://llvm.org/bugs/show_bug.cgi?id=22803 ?

Same patch. (It links here :P)

And do we have an outstanding bug against call_once ?

No outstanding bug. The call_once changes are to fix/suppress a couple of TSAN diagnostics. (See the TSAN buildbot)

Woops Misread that comment. This patch does not currently fix that bug but it can and will once I update it. The change is trivial.

EricWF updated this revision to Diff 29126.Jul 6 2015, 2:33 PM
EricWF edited edge metadata.
  1. Fix increment/decrement consistency to fix PR22803.
  2. Put functions in atomic_support.h inside an inline namespace.
  3. Generate a build warning on MSVC when atomics aren't supported.
  4. Fix bad assertion inside shared_ptr race condition test case.

I don't think we should make any attempt to support GCC and Clang versions that do not support the __atomic* builtins. This would limit *building* (not using) libc++ to GCC 4.7 and beyond and clang 3.3 (possible earlier). I don't have an issue with this.

EricWF updated this revision to Diff 29138.Jul 6 2015, 5:06 PM

Remove unrelated change in <thread>.

mclow.lists accepted this revision.Jul 6 2015, 5:08 PM
mclow.lists edited edge metadata.

Except for the stuff in <thread>, this looks good.

include/thread
338 ↗(On Diff #29126)

I think this snuck in from somewhere else. Please don't commit this.

This revision is now accepted and ready to land.Jul 6 2015, 5:08 PM
EricWF closed this revision.Jul 6 2015, 5:27 PM