[PATCH] D20328: [libcxx] Externally threaded libc++ variant

[Ben Craig via cfe-commits]

bcraig added inline comments.

================
Comment at: include/__threading_support:201
@@ +200,3 @@
+// Mutex
+#define _LIBCPP_MUTEX_INITIALIZER nullptr
+struct __libcpp_platform_mutex_t;
----------------
rmaprath wrote:
> rmaprath wrote:
> > bcraig wrote:
> > > rmaprath wrote:
> > > > bcraig wrote:
> > > > > rmaprath wrote:
> > > > > > bcraig wrote:
> > > > > > > I'm not sure I like taking the freedom to define _LIBCPP_MUTEX_INITIALIZER away from implementers.
> > > > > > > 
> > > > > > > Would it be too terrible to replace this entire #elif block with something like the following?
> > > > > > > 
> > > > > > > ```
> > > > > > > #if !defined(__has_include) || __has_include(<os_provided_thread.h>)
> > > > > > > #include <os_provided_thread.h>
> > > > > > > #else
> > > > > > > #error "_LIBCPP_THREAD_API_EXTERNAL requires the implementer to provide <os_provided_thread.h> in the include path"
> > > > > > > #endif
> > > > > > > ```
> > > > > > > 
> > > > > > > 
> > > > > > The problem is that, `std::mutex` constructor needs to be `constexpr` (as you pointed out earlier). And since `__libcpp_mutex_t` is a pointer type (for this externally threaded variant), sensible definitions for `_LIBCPP_MUTEX_INITIALIZER` are limited.
> > > > > > 
> > > > > > Other than `nullptr`, one may be able to define `_LIBCPP_MUTEX_INITIALIZER` to be a pointer to some constant mutex (presuming that make it `constexpr` OK?) but I'm not convinced if such a setup would be very useful.
> > > > > > 
> > > > > > Hope that sounds sensible?
> > > > > If the implementer gets to provide an entire header, then they also get to choose what libcpp_mutex_t will be.  They could make it a struct.
> > > > This externally-threaded library variant needs to be compiled against a set API, so we have this header with declarations like `__libcpp_mutex_lock()` which are referred from within the library sources (same function signatures as those used in `LIBCPP_THREAD_API_PTHREAD` - this allows us to keep the library source changes to a minimum).
> > > > 
> > > > Now, in this particular library variant, we want to differ these calls to runtime rather than libcxx compile-time! 
> > > > 
> > > > On the other hand, I think you are proposing a compile-time (static) thread porting setup where the platform vendors need to supply a header file with appropriate definitions for these functions / types, and they would compile libcxx themselves? That sounds like a good idea, but the current patch is aiming for a different goal: we provide a pre-compiled libcxx library which calls out to those `__libcpp_xxx` functions at runtime, and platform vendors need to provide those functions, they don't have to compile libcxx themselves. This is what forces us to use opaque pointer types to capture platform-defined threading primitives.
> > > > 
> > > > Perhaps I could improve the naming here, may be `LIBCPP_HAS_RUNTIME_THREAD_API`? Or `LIBCPP_HAS_DYNAMIC_THREAD_API`?
> > > That is an excellent summary, and it does a good job of explaining the disconnect we were having.  I'm going to push forward with the disconnect though :)
> > > 
> > > I think you can implement the dynamic case in terms of the static case without loss of generality or performance.  You ("Vendor X") could pre-compile your library with a pointer-sized libcpp_mutex_t defined in <os_provided_thread.h>, and at runtime call out to a different library that has an implementation of libcpp_mutex_lock.  Someone else ("Vendor Y") could have a larger libcpp_mutex_t defined, and provide a static inline definition of libcpp_mutex_lock.
> > OK, I think I understand your point :)
> > 
> > Will meditate on it a bit and spin a new patch tomorrow. Thanks!
> > 
> > 
> There is a slight complication here. If we fully offload the external thread-API to platform vendors, nobody will be able to build this externally-threaded library variant using the vanilla libcxx sources, because they would have to first come up with a `platform_threads.h` header.
> 
> We could provide a `dynamic_threads.h` header with the method signatures only (which is selected if the `platform_threads.h` header is absent - and you get a "dynamically-threaded" library build). Now the library can be built + tested with the vanilla upstream sources. But note that we are introducing yet another header. I remember @mclow.lists mentioned that each additional header adds to the overhead of header lookup and we should try to keep that to a minimum.
> 
> We can workaround this additional header by not collecting it when building the normal library variant (so it doesn't add to the header lookup overhead).
> 
> Does that sound like an OK approach?
Sounds reasonable.


http://reviews.llvm.org/D20328