Diff Detail
Event Timeline
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?
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 |
sources/cxx-stl/llvm-libc++/libcxx/include/__config | ||
---|---|---|
14 | awong also suggested that I wrap this in an #if !_WIN32. |
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) |
Renamed according to mclow's suggestion: s/_LIBCPP_SINGLE_THREADED/_LIBCPP_HAS_NO_THREADS/
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. |
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 |
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".
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? |
src/mutex.cpp | ||
---|---|---|
224–233 ↗ | (On Diff #12804) | Sigh. Yes. *hits head on desk*. Why did I do it like that? |
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 |
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 |
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.
awong also suggested that I wrap this in an #if !_WIN32.