[libcxx-commits] [PATCH] D65895: Fix a layering violation in mutex - prep for fixing PR42918

[Marshall Clow via Phabricator via libcxx-commits]

mclow.lists updated this revision to Diff 214165.
mclow.lists added a comment.

Re-gen diff with more context. No other changes.

The general problem that I want to solve here is:
C++11's threading model requires a "this is no thread" thread-id.  pthreads doesn't really have that.
We fake it by using the ID `0` for that. (this is complicated by the fact that the type of a `pthread_id` changes from system to system; fortunately, it appears always to be a scalar - int or pointer).

Bug 42918 <https://llvm.org/PR42918> shows a case where we pass this "not a thread" thread_id to pthreads, and in some weird cases, `pthread_equal` doesn't deal well with this.

We have an abstraction, `__thread_id`, which could manage this for us. But not everyone uses that abstraction; for example `recursive_timed_mutex`. This patch reworks `recursive_timed_mutex` so that it uses `__thread_id` instead of the lower-level `__libcpp_thread_id`.  That's all. `__thread_id` has a single field, which is a `__libcpp_thread_id`, and calls `__libcpp_thread_id_equal` to compare those ids, so there should be no behavior change from this patch.

However, once we fix `__thread_id::operator==` to handle the "not a thread" thread_id case, then `recursive_timed_mutex` will get that corrected behavior as well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65895/new/

https://reviews.llvm.org/D65895

Files:
  libcxx/include/mutex
  libcxx/include/thread
  libcxx/src/mutex.cpp


Index: libcxx/src/mutex.cpp
===================================================================
--- libcxx/src/mutex.cpp
+++ libcxx/src/mutex.cpp
@@ -132,7 +132,7 @@
 
 recursive_timed_mutex::recursive_timed_mutex()
     : __count_(0),
-      __id_(0)
+      __id_{}
 {
 }
 
@@ -144,9 +144,9 @@
 void
 recursive_timed_mutex::lock()
 {
-    __libcpp_thread_id id = __libcpp_thread_get_current_id();
+    __thread_id id = this_thread::get_id();
     unique_lock<mutex> lk(__m_);
-    if (__libcpp_thread_id_equal(id, __id_))
+    if (id ==__id_)
     {
         if (__count_ == numeric_limits<size_t>::max())
             __throw_system_error(EAGAIN, "recursive_timed_mutex lock limit reached");
@@ -162,9 +162,9 @@
 bool
 recursive_timed_mutex::try_lock() _NOEXCEPT
 {
-    __libcpp_thread_id id = __libcpp_thread_get_current_id();
+    __thread_id id = this_thread::get_id();
     unique_lock<mutex> lk(__m_, try_to_lock);
-    if (lk.owns_lock() && (__count_ == 0 || __libcpp_thread_id_equal(id, __id_)))
+    if (lk.owns_lock() && (__count_ == 0 || id == __id_))
     {
         if (__count_ == numeric_limits<size_t>::max())
             return false;
@@ -181,7 +181,7 @@
     unique_lock<mutex> lk(__m_);
     if (--__count_ == 0)
     {
-        __id_ = 0;
+        __id_.reset();
         lk.unlock();
         __cv_.notify_one();
     }
Index: libcxx/include/thread
===================================================================
--- libcxx/include/thread
+++ libcxx/include/thread
@@ -238,6 +238,9 @@
         bool operator>=(__thread_id __x, __thread_id __y) _NOEXCEPT
         {return !(__x < __y);}
 
+    _LIBCPP_INLINE_VISIBILITY
+    void reset() { __id_ = 0; }
+    
     template<class _CharT, class _Traits>
     friend
     _LIBCPP_INLINE_VISIBILITY
Index: libcxx/include/mutex
===================================================================
--- libcxx/include/mutex
+++ libcxx/include/mutex
@@ -196,6 +196,7 @@
 #endif
 #include <version>
 #include <__threading_support>
+#include <thread>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
@@ -280,7 +281,7 @@
     mutex              __m_;
     condition_variable __cv_;
     size_t             __count_;
-    __libcpp_thread_id __id_;
+    __thread_id        __id_;
 public:
      recursive_timed_mutex();
      ~recursive_timed_mutex();
@@ -307,9 +308,9 @@
 recursive_timed_mutex::try_lock_until(const chrono::time_point<_Clock, _Duration>& __t)
 {
     using namespace chrono;
-    __libcpp_thread_id __id = __libcpp_thread_get_current_id();
+    __thread_id __id = this_thread::get_id();
     unique_lock<mutex> lk(__m_);
-    if (__libcpp_thread_id_equal(__id, __id_))
+    if (__id == __id_)
     {
         if (__count_ == numeric_limits<size_t>::max())
             return false;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D65895.214165.patch
Type: text/x-patch
Size: 2795 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20190808/4c20cb9d/attachment-0001.bin>