[PATCH] D11781: Refactored pthread usage in libcxx

[Eric Fiselier via cfe-commits]

EricWF added a comment.

I would like to see this patch without the `support/pthread/*.cpp` files. There are a couple of reasons for this.

1. The symbols in those files are private to the dylib, but they are declared in the headers and given external visibility.
2. Those symbols frequently use std::chrono types in their interface. This creates a layering violating between the main STL and the support headers.
3. 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.


================
Comment at: include/support/pthread/condition_variable.h:54
@@ +53,3 @@
+_LIBCPP_FUNC_VIS
+void __os_cond_var_timed_wait(__os_cond_var* __cv,
+     __os_mutex* __m,
----------------
This is an internal function. It shouldn't be declared in these headers.

================
Comment at: include/support/pthread/mutex.h:27
@@ +26,3 @@
+typedef pthread_mutex_t __os_mutex;
+_LIBCPP_CONSTEXPR pthread_mutex_t __os_mutex_init = PTHREAD_MUTEX_INITIALIZER;
+
----------------
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.

================
Comment at: include/support/pthread/mutex.h:59
@@ +58,3 @@
+_LIBCPP_FUNC_VIS
+void __os_recursive_mutex_init(__os_mutex* __m);
+
----------------
This is an internal function and shouldn't be declared in the headers.

================
Comment at: include/support/pthread/mutex.h:62
@@ +61,3 @@
+inline _LIBCPP_INLINE_VISIBILITY
+int __os_recursive_mutex_lock(__os_mutex* __m)
+{
----------------
Shouldn't these functions take `__os_recursive_mutex*` types?

================
Comment at: include/support/pthread/mutex.h:85
@@ +84,3 @@
+
+_LIBCPP_FUNC_VIS void __os_call_once(volatile unsigned long&, void*, void(*)(void*));
+
----------------
This is an internal function and shouldn't be declared in the headers.

================
Comment at: include/support/pthread/thread.h:60
@@ +59,3 @@
+{
+    bool res = pthread_equal(t1, t2);
+    return res != 0 ? 0 : (t1 < t2 ? -1 : 1);
----------------
`res` needs to be an int, not a bool.

================
Comment at: include/support/pthread/thread.h:67
@@ +66,3 @@
+basic_ostream<_CharT, _Traits>&
+__os_write_to_ostream(basic_ostream<_CharT, _Traits>& __os, __os_thread_id __id)
+{
----------------
This function seems superfluous and introduces a dependency on IO streams. Get rid of it.

================
Comment at: include/support/pthread/thread.h:114
@@ +113,3 @@
+
+_LIBCPP_FUNC_VIS void __os_sleep_for(const chrono::nanoseconds& ns);
+
----------------
This is an internal function and shouldn't be declared in the headers.

================
Comment at: include/thread:238
@@ -236,3 +237,3 @@
         bool operator==(__thread_id __x, __thread_id __y) _NOEXCEPT
-        {return __x.__id_ == __y.__id_;}
+        {return __libcpp_support::__os_thread_id_compare(__x.__id_, __y.__id_) == 0;}
     friend _LIBCPP_INLINE_VISIBILITY
----------------
This now call's `pthread_equal`, across a library boundary, where it didn't before. That seems like a pessimization.
 

================
Comment at: include/thread:244
@@ -242,3 +243,3 @@
         bool operator< (__thread_id __x, __thread_id __y) _NOEXCEPT
-        {return __x.__id_ < __y.__id_;}
+        {return  __libcpp_support::__os_thread_id_compare(__x.__id_, __y.__id_) < 0;}
     friend _LIBCPP_INLINE_VISIBILITY
----------------
Same as above.

================
Comment at: include/thread:260
@@ -258,3 +259,3 @@
     operator<<(basic_ostream<_CharT, _Traits>& __os, __thread_id __id)
-        {return __os << __id.__id_;}
+        {return __libcpp_support::__os_write_to_ostream(__os, __id.__id_);}
 
----------------
As mentioned elsewhere just do this operation inline. No need for an extra function.

================
Comment at: include/thread:393
@@ -390,3 +392,3 @@
     std::unique_ptr<_Fp> __p(new _Fp(__f));
-    int __ec = pthread_create(&__t_, 0, &__thread_proxy<_Fp>, __p.get());
+    int __ec = __libcpp_support::__os_create_thread(&__t_, &__thread_proxy<_Gp>, __p.get());
     if (__ec == 0)
----------------
Typo `_Gp` should be `_Fp`.


Repository:
  rL LLVM

http://reviews.llvm.org/D11781