[PATCH] D21968: [libcxx] Externally threaded libc++ variant - Take 2

[Asiri Rathnayake via cfe-commits]

rmaprath added inline comments.

================
Comment at: include/__config:830
@@ -829,1 +829,3 @@
+    !defined(_LIBCPP_HAS_THREAD_API_PTHREAD) && \
+    !defined(_LIBCPP_HAS_THREAD_API_EXTERNAL)
 # if defined(__FreeBSD__) || \
----------------
compnerd wrote:
> I meant on the lines that you added :-).
Will update to reflect rest of the source file.

================
Comment at: include/__threading_support:261
@@ -200,1 +260,3 @@
+void* __libcpp_tl_get(__libcpp_tl_key __key);
+void __libcpp_tl_set(__libcpp_tl_key __key, void* __p);
 
----------------
compnerd wrote:
> I think we should explicitly spell out `tls`.  The extra character won't hurt.
Will fix.

================
Comment at: lib/CMakeLists.txt:216
@@ +215,3 @@
+  )
+endif()
+
----------------
compnerd wrote:
> Have you considered a slightly alternative approach of basically implementing pthreads as an "external" threading model as well?  You could override it, or take the default implementation that is provided.  It avoids having a second supporting DSO for threading as we could always statically link against it.
So, the particulars of this design was discussed and (generally) agreed upon sometime back.

Currently pthreads **is** hidden behind the same threading API as the externally threaded variant (on this patch). To override the default (pthread) implementation, you just have to drop in an `__external_threading` header and specify the `-DLIBCXX_HAS_EXTERNAL_THREAD_API` cmake option.

There's no need to provide a second DSO if your `__external_threading` header (and your  thread-system sources) are available at `libc++` build time.

But we have a **requirement** that the library needs to be build-able in such a way you can provide a second DSO holding the implementation of the API. That is, we need to be able to build and ship a library to which customers can drop-in their own thread implementation (i.e. the thread-support DSO).

Note that in one of my earlier patches, I proposed an even more de-coupled external threading system where even the types like `__libcpp_mutex_t` were opaque pointers. @mclow.lists opposed that (perhaps rightfully so, now I think about it) and currently these types need to be known at `libc++` compile time. The implementation of the API functions like `__libcpp_mutex_lock()` can be deferred to link time (this is what is provided in the second DSO).

Hope that clarifies everything here?







================
Comment at: test/support/external_threads.cpp:26
@@ +25,3 @@
+    if (__ec)
+      goto fail;
+
----------------
compnerd wrote:
> I think this would be nicer as:
> 
>   if (int error = pthread_mutexattr_init(&attr))
>     return error;
I think I'm following the conventions used in the `libc++` source files here. Will double check tomorrow.

================
Comment at: test/support/external_threads.cpp:52
@@ +51,3 @@
+fail:
+    return __ec;
+}
----------------
compnerd wrote:
> I don't think that the label for failure is adding anything since you really are handling the destruction of the mutex at all failure sites.
Not sure what I did here. Will double check and fix.

================
Comment at: test/support/external_threads.cpp:70
@@ +69,3 @@
+
+int __libcpp_mutex_destroy(__libcpp_mutex_t* __m) {
+    return pthread_mutex_destroy(__m);
----------------
compnerd wrote:
> Please choose a single style.
Will fix.

================
Comment at: test/support/external_threads.cpp:103
@@ +102,3 @@
+
+bool __libcpp_thread_id_equal(__libcpp_thread_id t1, __libcpp_thread_id t2) {
+    return pthread_equal(t1, t2) != 0;
----------------
compnerd wrote:
> Please choose a single style (and below).
Will fix.

================
Comment at: test/support/external_threads.cpp:131
@@ +130,3 @@
+    // before.
+    if (*__t == 0)
+        return -1;
----------------
compnerd wrote:
> Can you compare to `nullptr` instead?
I can't remember exactly why, but I think there was a problem with using `nullptr` here. I will check and get back.

================
Comment at: test/support/external_threads.cpp:142
@@ +141,3 @@
+    // before.
+    if (*__t == 0)
+        return -1;
----------------
compnerd wrote:
> Same.
Need to check.


https://reviews.llvm.org/D21968