This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Allow libc++ to be built on systems without POSIX threads
ClosedPublic

Authored by jroelofs on May 30 2014, 2:28 PM.

Details

Reviewers
danalbert
joerg

Diff Detail

Event Timeline

jroelofs updated this revision to Diff 9964.May 30 2014, 2:28 PM
jroelofs retitled this revision from to [libc++] Allow libc++ to be built on systems without POSIX threads.
jroelofs updated this object.
jroelofs edited the test plan for this revision. (Show Details)
jroelofs added a reviewer: joerg.
jroelofs added a subscriber: Unknown Object (MLST).
joerg edited edge metadata.May 30 2014, 3:07 PM

I'm not sure I like the name _LIBCPP_SINGLE_THREADED, especially since it is pretty much exclusively used in the negative sense. Otherwise fine with me.

sources/cxx-stl/llvm-libc++/libcxx/include/__config
124
#if _POSIX_THREADS - 0 > 0

might be simpler

I picked the name to match the one I recently added to libc++abi. I'm open to suggestions though.

The other one I considered was _LIBCPP_HAS_POSIX_THREADS, but that seemed to be less fitting (one could have a system with pthreads, but still not want to build libc++ with threads enabled, for example).

sources/cxx-stl/llvm-libc++/libcxx/include/__config
124

Is there much of a precedent either way? I haven't seen any laid out like that...

In general, libc++ uses the negative for missing capabilities.
_LIBCPP_HAS_NO_RVALUE_REFERENCES
_LIBCPP_HAS_NO_ADVANCED_SFINAE
_LIBCPP_HAS_NO_VARIADICS
_LIBCPP_HAS_NO_TRAILING_RETURN

I would prefer to keep this convention.

I would also like to understand more about the motivation behind this patch.
Concurrency is a core part of C++11library, and just disabling it seems wrong to me.

I know there are systems w/o POSIX threads, but to support those systems, libc++ should use whatever primitives exist there, rather than just disabling all the threading support.

What are you trying to accomplish here?

jroelofs added a comment.EditedMay 31 2014, 7:43 AM

mclow, the motivation for this is for bare-metal systems where we don't have threads. I suppose my title for this left a little too much room to read into it.

The other alternative would be to write no-op shims around all of these primitives, but that seems like it would be a lot of mess for very little benefit.

sources/cxx-stl/llvm-libc++/libcxx/include/shared_mutex
419

line should read:

#endif // !_LIBCPP_SINGLE_THREADED

sources/cxx-stl/llvm-libc++/libcxx/src/memory.cpp
175

line should read:

#endif // __has_feature(cxx_atomic) && !_LIBCPP_SINGLE_THREADED

jroelofs added inline comments.May 31 2014, 7:48 AM
sources/cxx-stl/llvm-libc++/libcxx/include/__mutex_base
53

awong pointed out that it makes sense to exclude defer_lock_t and friends also,

so delete this line....

269

... and this one.

jroelofs added inline comments.May 31 2014, 8:51 AM
sources/cxx-stl/llvm-libc++/libcxx/include/__config
14

awong also suggested that I wrap this in an #if !_WIN32.

joerg added a comment.Jul 29 2014, 5:43 AM

What is the status of this patch?

jroelofs added inline comments.Aug 13 2014, 8:05 AM
sources/cxx-stl/llvm-libc++/libcxx/src/thread.cpp
129

This ought to get moved to http://reviews.llvm.org/D4045 (i.e. place sleep_for inside a _LIBCPP_HAS_MONOTONIC_CLOCK)

jroelofs updated this revision to Diff 12452.Aug 13 2014, 8:08 AM
jroelofs edited edge metadata.

Hi Joerg,

I'm finally resurrecting this...

jroelofs updated this revision to Diff 12454.Aug 13 2014, 8:13 AM

Renamed according to mclow's suggestion: s/_LIBCPP_SINGLE_THREADED/_LIBCPP_HAS_NO_THREADS/

jroelofs updated this revision to Diff 12456.Aug 13 2014, 8:16 AM

Fix a couple of typos I just caught.

See the inline comments; I think that _LIBCPP_HAS_NO_THREADS should be defined when building single-threaded, and not defined otherwise (that would match the rest of the libc++ config macros).

Also, aren't there a bunch of tests that are going to need to change?

include/__config
126 ↗(On Diff #12456)

This should be wrapped in an "#ifndef _LIBCPP_HAS_NO_THREADS" block, and should just define it, rather than give it a 0/1 value.

This will allow someone to build an "no threads" version of libcpp by -D _LIBCPP_HAS_NO_THREADS on the command line.

include/__mutex_base
25 ↗(On Diff #12456)

this should just be #ifndef _LIBCPP_HAS_NO_THREADS

33 ↗(On Diff #12456)

Whitespace change?

I have another set of patches for the tests that this affects. Should I merge them all and include them here?

include/__config
126 ↗(On Diff #12456)

Wait, do you mean that this hunk should just go away? Or do you mean that it should be:

#if !defined(_LIBCPP_HAS_NO_THREADS) && defined(_POSIX_THREADS) && _POSIX_THREADS > 0
#define _LIBCPP_HAS_NO_THREADS
#endif

I'm not sure I understand what you're asking for here.

include/__mutex_base
25 ↗(On Diff #12456)

Ok, I'll change all of them to that style.

33 ↗(On Diff #12456)

Looks like... I'll get rid of it.

jroelofs updated this revision to Diff 12803.Aug 21 2014, 1:49 PM

Implement a few of mclow's comments.

It'll take a bit more to fish out the changes to the tests (mostly adding 'XFAIL: libcxx-single-threaded')

include/__config
126 ↗(On Diff #12803)

@mclow earlier you suggested having just the build define this with -D_LIBCPP_HAS_NO_THREADS. I thought adding this as a error/warning might be nice.

Another thing I've just realized: if this is only set during the build, then how to I arrange for it to be set in an installed system? I don't want my users to have to -D_LIBCPP_HAS_NO_THREADS whenever they want to use libcxx.

include/thread
110 ↗(On Diff #12803)

s/system/single threaded system/

src/thread.cpp
33 ↗(On Diff #12803)

un-necessary whitespace change

jroelofs updated this revision to Diff 12804.Aug 21 2014, 2:02 PM

Fix a whiltespace issue, and re-word an error message.

XFAILS for this are here: http://reviews.llvm.org/D5013

I didn't add any of the lists as a subscriber as this diff is rather large and mechanical.

In summary it adds "// XFAIL: libcxx-single-threaded" to all of the tests which this "breaks".

EricWF added a subscriber: EricWF.Aug 21 2014, 4:07 PM
EricWF added inline comments.
src/mutex.cpp
224–233 ↗(On Diff #12804)

Wouldn't it be a lot cleaner to just separate the two implementation entirely instead of interweaving them like this?

jroelofs added inline comments.Aug 21 2014, 4:11 PM
src/mutex.cpp
224–233 ↗(On Diff #12804)

Sigh. Yes. *hits head on desk*. Why did I do it like that?

jroelofs added inline comments.Aug 21 2014, 5:56 PM
include/memory
615 ↗(On Diff #12804)

I missed a pair of changes in this file. The __has_feature(cxx_atomic) #ifdef block needs to be:

#if __has_feature(cxx_atomic) && !defined(_LIBCXX_HAS_NO_THREADS)
...
#endif  // __has_feature(cxx_atomic) && !defined(_LIBCXX_HAS_NO_THREADS)

I also missed a pair of these in include/ios near lines 216 and 367

jroelofs updated this revision to Diff 13079.Aug 29 2014, 7:13 AM

Separate out the two halves of __call_once as per EricWF's suggestion, and address my own review comment (about having forgotten a section).

I'm a little bit worried about the no-threads build bit-rotting (if any of us forget to wrap something in #ifndef _LIBCPP_HAS_NO_THREADS). Should we have a buildbot set up to build this configuration? Would it be possible to define noop implementations of all the things you've ifdef'd out so that we don't have to worry about that? It's possible that would make things more or a mess, but I'm not sure.

src/condition_variable.cpp
19 ↗(On Diff #13079)

nit

danalbert accepted this revision.Aug 29 2014, 10:39 AM
danalbert added a reviewer: danalbert.

From email thread:

Should we have a buildbot set up to build this configuration?

I would appreciate having one, but I can't host it at the moment.
At some point I would like to host a baremetal buildbot (targeting either QEMU, a raspberry pi, or some beagleboard), but I'm not quite ready for that... we need to work out the details of remote testing in lit before that is practical.

Keeping a buildbot going for this patch doesn't actually require baremetal (though that would be nice to have as well). @EricWF recently set up a buildbot. He might be willing to host it.

Would it be possible to define noop implementations of all the things you've ifdef'd out so that we don't have to worry about that? It's possible that would make things more or a mess, but I'm not sure.

I think that nop-shims have an even higher potential for bit-rot than these #ifdef's.
I originally attempted writing no-op shims for a few of them, but there were several where it didn't seem possible, and others where the shims were pretty hairy.

Okay, I thought that might be the case.

With those things accounted for, LGTM.

This revision is now accepted and ready to land.Aug 29 2014, 10:39 AM
jroelofs updated this revision to Diff 13336.Sep 5 2014, 11:54 AM
jroelofs edited edge metadata.

Fix a couple of nits, and add in the XFAILs as UNSUPPORTEDs.

Oh, and I also removed the autodetect part of include/__config

jroelofs closed this revision.Sep 5 2014, 1:00 PM

r217271