This patch is a refactoring of pthread usage in libcxx to support/pthread in order to ease porting to Windows.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
include/__mutex_base | ||
---|---|---|
18 | I think it might make sense to create a file: <support/mutex.h> where the contents are just: #ifndef _WIN32 #include <support/pthread/mutex.hpp> #endif And include that file here so that the platform decision of "which mutex implementation should we use" doesn't have these #ifndef guards spread everywhere. Maybe you could also add some defines & guards to enforce the idea that <support/pthread/mutex.hpp> is never included directly, but rather only via <support/mutex.h>. Same should go for thread.hpp, too. |
include/__mutex_base | ||
---|---|---|
269 | Does this change the ABI for a mutex on *NIX platforms? We can't change the class layout for existing platforms (though we can indirect things via typedefs). | |
include/mutex | ||
181–182 | As above, there should probably be in a cross-platform support file that includes these. In particular, not-win32 is not the correct condition for uses-pthreads. We should probably have something in __config that determines the thread library to use. It would be quite nice to have a C11 thread back end, for example (though most implementations currently wrap pthreads). | |
287 | I didn't realise this idiocy made it into C++11, I thought it was confined to C11. Locking a mutex using anything other than time intervals on the monotonic time source is a great way of producing non-working programs. We should probably warn if anyone actually calls this... |
include/__mutex_base | ||
---|---|---|
18 | A very good idea indeed. I'll make the change and update the patch. | |
269 | My hunch is that it doesn't, std::condition_variable has no data member now and the first base class (__libcxx_support::condition_variable) contains only pthread_cond_t which will be effectively laid out at the starting address of the object, as it was previously for std::condition_variable,. I will check this out later in the evening though. | |
include/mutex | ||
181–182 | In this case having a series of #ifdef FreeBSD (or _WIN32, etc.) doesn't quite cut it as we want to be able to select C11 or pthread on most of them and on Windows one day it might even be C11, pthread or the win32 api. I guess the alternative is to provide a cmake variable that default to something different on each platform? |
include/__mutex_base | ||
---|---|---|
269 | I *think* that it's correct, but it's worth compiling a program that has one compilation unit using the old header and another using the new one and check that it's able to interoperate correctly. Also check whether this depends on the implementation of the condition variable. On FreeBSD, the pthread types are all pointers (currently - we're going to have some painful ABI breakage at some point when we move them to being something that can live in shared memory). In glibc, they're structures. I don't think that you'll end up with different padding in the ABI from wrapping them in a class, but it's worth checking. | |
include/mutex | ||
181–182 | We'll probably end up with a set of #ifdef FreeBSD (or whatever) things, but making sure that they're all in a single file will help. If you're doing bring-up of a new platform, just having to set #define LIBCXX_THREAD_API LIBCXX_PTHREAD, or LIBCXX_C11_THREADS, (or Haiku threads, or whatever) in one place makes life a bit easier. One of the annoyances with trying to port asan was that the original developers used #ifdef APPLE__ to mean 'is not Linux' and 'is Darwin' in various places, so we needed to look at every single change and determine whether they were shared between multiple non-GNU platforms or whether they were specific to OS X / iOS. |
include/__mutex_base | ||
---|---|---|
269 |
Unfortunately this isn't going to work, some simbols are now defined in __libcxx_support and not in std (they are just made available through using), so an object file that includes the old header won't link against the new library (unreferenced symbols). The alternative is to create inline forwarding methods. |
include/__mutex_base | ||
---|---|---|
269 | That's going to need some more refactoring then. Breaking the public ABI of libc++ would be a show-stopper. |
include/__mutex_base | ||
---|---|---|
269 | Yes, that's what I thought. In this case it's better to create a new diff and abandon this one or just update it? |
To avoid breaking public api and ABI, the os abstraction layer is made up of typedefs for pthread types + plain functions that are called from the std:: stuff.
The recursive mutex layer is identical for pthread to the mutex layer because pthread internally select locking implementation depending on the mutex attributes, but we need both because on Windows the recursive stuff must be implemented with a counter.
This version looks much better (well, the previous one is cleaner, but this one looks like it shouldn't break the ABI - did you test this? It looks like it should work, but I've not actually tried it).
include/__config | ||
---|---|---|
751 | #ifdef unix will catch most of these (for some reason, not OS X, even though it's the only one that actually is certified as UNIX...) |
I tried it on Linux, using the example from http://en.cppreference.com/w/cpp/thread/condition_variable, but splitting the main and the worker function in two different translation units and compiling one with the libc++ coming from ArchLinux and the other with my branch, and then linking both with the new lib. I don't have freebsd installed though, I can set up another virtual machine but I never used FreeBsd so I don't know how easy is to install and set up.
include/__config | ||
---|---|---|
751 | I didn't know that and I'm not sure I've included all the supported platforms (in include/__config there are definitions for __ sun __ and __ CloudABI __ which I know very little about). Is there somewhere a list of supported platforms? |
include/__config | ||
---|---|---|
751 | CloudABI has pthreads, but it does not have unix defined. Both FreeBSD and CloudABI also support C11 <threads.h>. If you're going to stick to the code you have right now, be sure to add defined(__CloudABI__) to that list of operating systems. The POSIX way of testing whether pthreads is available is: #include <unistd.h> #if _POSIX_THREADS > 0 ... #endif See: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/unistd.h.html |
A general note I have regarding this change:
Now that we're introducing separate implementations for mutexes and condition variables, could we also consider letting shared_mutex and friends simply use pthread_rwlock_*()? We currently have it implemented as a wrapper on top of mutexes and condition variables, but the downside of this approach is that it potentially has more overhead, but also works around priority inheritance if supported by the implementation.
It would be nice to use rwlocks, but that's an ABI-breaking change. We'd need to bump the .so version and other annoyances. Definitely something I'd be in favour of seeing hidden behind some #ifdefs (and enabled for FreeBSD 11 and probably for the libc++ in packages), but it's beyond the scope of this set of changes.
include/__config | ||
---|---|---|
751 | @espositofulvio: @ed meant this: #ifndef _WIN32 # include <unistd.h> # if _POSIX_THREADS > 0 ... # endif #endif Which is the correct way to test for this. |
include/__config | ||
---|---|---|
751 | That being said, there have been discussions before about whether or not we should #include <unistd.h> in <__config>, with the conclusion being that we shouldn't. It would be better if this were a CMake configure-time check that sets _LIBCPP_THREAD_API, rather than these build-time guards. |
include/__config | ||
---|---|---|
751 | Tried adding that as configure time checks, but then libcxxabi fails to compile because of the guard in __config to check that _LIBCPP_THREAD_API has beed defined when _LIBCPP_HAS_NO_THREADS is not. As a side note: Is Windows the only OS which hasn't got unistd.h? |
include/__config | ||
---|---|---|
751 |
Can you put the patch for that up on gist.github.com, or a pastebin?... I'll take a look.
For the platforms libcxx currently builds on, yes. |
include/__config | ||
---|---|---|
751 |
It's here https://gist.github.com/espositofulvio/eac2fb08acf2e430c516 |
include/__config | ||
---|---|---|
751 | I'm sorry to have triggered this bikeshed. Given that, of the currently supported platforms, Windows is the only one where we want to use threads but don't want to use PTHREADS, I think it's fine to have this check be !WIN32. The important thing is having the check *in one place* so that the person who wants to add the *second* non-pthread threading implementation doesn't have a load of different places to look: they can just change it in __config and then chase all of the compile failures. |
include/__config | ||
---|---|---|
751 | I meant something like this: https://gist.github.com/jroelofs/279cb2639ad910b953d2 ... which doesn't quite work yet, but should give you the idea. I don't know how to get CMake to treat ${CMAKE_BINARY_DIR}/include/c++/v1 as the includes directory instead of ${CMAKE_CURRENT_SOURCE_DIR}/../include. Maybe @EricWF can help with that? Basically the idea is that we have cmake create a config_site header which config includes. This new header would then have the definitions for these kinds of configure-time decisions. |
include/__config | ||
---|---|---|
751 | I tinkered around with this a bit more, and got it to work. I've updated the gist to reflect that. To simplify things for @espositofulvio, I'm going to split out the __config_site part of this change into its own review. |
include/__config | ||
---|---|---|
751 | I was working on your idea and make it to compile as well, I'm just re-running the test suite to be 100% sure. Looking at your diff it seems there's a difference in the path. ${CMAKE_BINARY_DIR}/include/c++/v1/__config_site the above doesn't work for me, I've used: ${CMAKE_CURRENT_SOURCE_DIR}/include/__config_site Should I update my review? |
include/__config | ||
---|---|---|
751 | The reason to use CMAKE_BINARY_DIR over CMAKE_CURRENT_SOURCE_DIR as the location for this build product is so that the source directory doesn't get polluted by builds. Unfortunately that means that both libcxx and libcxxabi need to be pointed at the headers in the build directory, and not the ones from the source directory. I do have a pair of patches for this, which I'll post shortly. |
The difference between this and @jroelofs's one is that this copy the __config_site in the source directory which makes everything compile fine without having to point libcxx and libcxxabi to the build directory. Jonathan thinks it's better not to pollute the source dir so he made those patches.
This feels a bit like a regression. Before, libc++ works fine by just pointing into the include directory.
With my change it still is fine pointing at the include directory. But as I said, Jonathan feels it's not right to have the configure generated include file in the source directory hence he's proposing a separate patch to address that.
This patch has a long way to go but it has also come a long way. Here are a couple of problems I see with it.
- There are still plenty of ABI breaks. I'll try and point them all out.
- This patch adds a lot of headers. libc++ has historically tried to keep the number of headers to a minimum for the reason that filesystem operations are expensive and its cheaper to include a few big headers as opposed to many small ones.
- There where some subtle static initialization changes. However they may have been fixed.
I'll do a more through review soon.
include/mutex | ||
---|---|---|
536 | This looks like an ABI break to me. |
Added more inline comments.
include/__mutex_base | ||
---|---|---|
38 | Why was the cast insignificant? | |
277 | Why was the cast insignificant? | |
include/thread | ||
421 | I'm 99% this is an ABI break. Don't inline this function and make the change within the definition | |
include/type_traits | ||
222 ↗ | (On Diff #31874) | This change seems to have snuck in. |
src/algorithm.cpp | ||
52 | I think this prevents __rs_mut from being initialized during constant initialization. (http://en.cppreference.com/w/cpp/language/constant_initialization) | |
src/memory.cpp | ||
130 | I have no idea what is going on here. Do you understand what this code was trying to do? |
I know but it was a request from reviewers so that platform specific decision were localized. I can obviously merge all of the support/mutex.h, support/thread.h and support/condition_variable.h in a single support/thread.h if that's the case.
include/type_traits | ||
---|---|---|
222 ↗ | (On Diff #31874) | Yes, and actually I'm not sure how it did as it's something I haven't touched. Unfortunately I don't have time before later next week to address all the issues. |
src/algorithm.cpp | ||
52 | I think this falls in the second case: Static or thread-local object of class type that is initialized by a constructor call, if the constructor is constexpr and all constructor arguments (including implicit conversions) are constant expressions, and if the initializers in the constructor's initializer list and the brace-or-equal initializers of the class memebers only contain constant expressions. but I was actually relying on the fact that we have constexpr, which now I understand could not be the case. | |
src/memory.cpp | ||
130 | My understanding is that this is trying to init the array during constant initialization and later relying on the internal layout of std::mutex to be equal to a pthread_mutex_t to cast it and use it as a std::mutex. from cppreference: Again, I think my change should be ok as long as constexpr is available, but I could be wrong. |
include/__mutex_base | ||
---|---|---|
38 | The cast was significant, but it's not needed anymore. PTHREAD_MUTEX_INITIALIZER can be only used in initialization expression, not regular assignment and that's why there was the cast. But now I'm assigning an already-initialized mutex: _LIBCPP_CONSTEXPR pthread_mutex_t __os_mutex_init = PTHREAD_MUTEX_INITIALIZER; thus the cast is not needed anymore. |
Sorry for giving conflicting feedback. I'm happy with multiple headers for now. We can always address it later.
src/algorithm.cpp | ||
---|---|---|
52 | The top of the documentation it shows the syntax that is required to get constant initialization. The initialization must have the form T obj = <initalizer>. I would be fine relying on constexpr though. |
- Addressed possible ABI breaks
- Reverted to not using a __config_file as @jroelofs has two separate patches for that
Hi, could I know the status of this? I'd like to push this forward.
@espositofulvio: Are you working on this? (just checking since this has gone stale for a while). @EricWF: I can create a separate diff for further review if this one is too cluttered.
Cheers,
/ Asiri
I would like to see this patch without the support/pthread/*.cpp files. There are a couple of reasons for this.
- The symbols in those files are private to the dylib, but they are declared in the headers and given external visibility.
- Those symbols frequently use std::chrono types in their interface. This creates a layering violating between the main STL and the support headers.
- Moving the implementations of those functions now creates a bunch of noise in an already noisy patch. Keep the implementations in place for now.
Those changes can be re-added in a follow up patch.
Please combine all everything in support into a single file in the top level directory called __thread_support (Or similar, the name really doesn't matter but it should start with two underscores). Libc++ keeps the number of private headers is kept to a minimum and only introduces a new one when it needs to break a cycle or factor out code used in two places.
Overall this is looking good. Have you implemented a support layer for windows at all? If so I would be curious to see it, I would like to get a better idea of the end goal.
include/support/pthread/condition_variable.h | ||
---|---|---|
54 | This is an internal function. It shouldn't be declared in these headers. | |
include/support/pthread/mutex.h | ||
27 | I think with would work for C++11 onward but unfortunately the mutex internals need to support C++03. For this reason we have to use a macro to ensure the initializer is constexpr. | |
59 | This is an internal function and shouldn't be declared in the headers. | |
62 | Shouldn't these functions take __os_recursive_mutex* types? | |
85 | This is an internal function and shouldn't be declared in the headers. | |
include/support/pthread/thread.h | ||
60 | res needs to be an int, not a bool. | |
67 | This function seems superfluous and introduces a dependency on IO streams. Get rid of it. | |
114 | This is an internal function and shouldn't be declared in the headers. | |
include/thread | ||
238 | This now call's pthread_equal, across a library boundary, where it didn't before. That seems like a pessimization. | |
244 | Same as above. | |
260 | As mentioned elsewhere just do this operation inline. No need for an extra function. | |
393 | Typo _Gp should be _Fp. |
Hi Eric, Thanks for the comments!
I'll wait a bit for @espositofulvio in case if he wants to push this, otherwise I'll create a new diff with the suggested changes.
For us (ARM), a threads porting layer is important on several RTOSes (where a full-blown pthreads implementations is not available). I will see if I can publish one of those porting layer implementations, but perhaps a windows porting layer is more interesting to the community, in which case I'll look into hacking one up (unless of course, if @espositofulvio has already got one).
Cheers,
/ Asiri
Thanks!
If you meant the mbed OS [1], I think it is still single-threaded (although that might change in the near future [2]). Most of our customers (of armclang) have proprietary RTOSes, but we might be able upstream a sample implementation (I'm thinking Keil RTX [3], but need to double check a few things), if it seems generally useful to the community.
Cheers,
/ Asiri
[1] https://www.mbed.com/
[2] https://www.mbed.com/en/development/software/mbed-os/ ("...we intend to re-introduce it in 2016")
[3] http://www.keil.com/rl-arm/kernel.asp
Unfortunately I don't have much spare time at the moment, and that being something I was doing to learn about libc++ and not something I needed, it got sidetracked. So go ahead with your work.
Best regards,
Fulvio
I think you've done all the difficult bits already :) I'll polish it up and address the remaining comments.
Thanks!!
/ Asiri
I stand corrected, mbed already has an RTOS layer: https://developer.mbed.org/handbook/RTOS. I'm not very familiar with mbed (obviously). I will do some digging to see how difficult it would be to port libcxx to mbed (we already build libcxx for cortex-m bare-metal systems, so this shouldn't be that hard).
After reading through the past threads, providing a windows thread porting layer sounds like a useless thing, given that getting libcxx to build on windows itself is a rather large piece of work.
Cheers,
- Asiri
This revision is no longer relevant after. The __threading_support changes have applied changes similar to this.
#ifdef unix will catch most of these (for some reason, not OS X, even though it's the only one that actually is certified as UNIX...)