[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:
> 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.


http://reviews.llvm.org/D20328