Fix a layering violation in mutex - prep for fixing PR42918

[mclow.lists]

Summary

While investigating https://llvm.org/PR42918, I noticed that the recursive_timed_mutex used the primitive thread-ids directly, rather than the convenience class __thread_id.

This fixes that, and makes it so we can fix the above issue only in __thread_id, rather than enshrining the "not a thread" knowledge in each of the OS-specific bits.

I'm a bit worried about ABI here, even though the field that I'm replacing is the same size.

[mclow.lists]

libcxx/include/mutex

199

If I were to commit this; I would probably move __thread_id and this_thread:: into <__threading_support> and remove this include.

[mclow.lists]

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

[mclow.lists]

libcxx/src/mutex.cpp

184

This change eliminates the "zero is special" knowledge from here, instead keeping it in __thread_id

[ldionne]

Apart from my comment, this LGTM.

libcxx/include/thread

242

I would rather call this __reset() to make it clear it's not part of the public API.

[mclow.lists]

Committed as revision 368867. I made Louis' suggested change (reset --> __reset) but somehow that didn't make it into the commit. I will fix that.

[DavidSpickett]

The move of thread_id to <threading_support> has caused a build error for us when _LIBCPP_HAS_THREAD_API_EXTERNAL is true

As far as I can tell __thread_id used to only require that _LIBCPP_HAS_NO_THREADS was false, but now it checks that both defines are false. I've been able to fix it locally but in a rather ugly way initially. I thought I'd let you know in case you had an idea of how it should be fixed. (or maybe I'm missing something)

I've put up my fix for review and added you as a reviewer:
https://reviews.llvm.org/D66301

[mclow.lists]

In D65895#1631335, @DavidSpickett wrote:

The move of thread_id to <threading_support> has caused a build error for us when _LIBCPP_HAS_THREAD_API_EXTERNAL is true

Well, crud. Sorry for the breakage.
I'll take a look at your patch.

[mclow.lists]

This has been landed, and the breakage has been fixed.

libcxx/include/mutex

 #include <__config>
 #include <__mutex_base>
 #include <cstdint>
 #include <functional>
 #include <memory>
 #ifndef _LIBCPP_CXX03_LANG
 #include <tuple>
 #endif
 #include <version>
 #include <__threading_support>
+#include <thread>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
 
 
 _LIBCPP_BEGIN_NAMESPACE_STD

         return true;
     }
     return false;
 }
 
 class _LIBCPP_TYPE_VIS recursive_timed_mutex
 {
     mutex              __m_;
     condition_variable __cv_;
     size_t             __count_;
-    __libcpp_thread_id __id_;
+    __thread_id        __id_;
 public:
      recursive_timed_mutex();
      ~recursive_timed_mutex();
 
 private:
     recursive_timed_mutex(const recursive_timed_mutex&); // = delete;
     recursive_timed_mutex& operator=(const recursive_timed_mutex&); // = delete;
 
 public:
     void lock();

         _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
         bool try_lock_until(const chrono::time_point<_Clock, _Duration>& __t);
     void unlock() _NOEXCEPT;
 };
 
 template <class _Clock, class _Duration>
 bool
 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;
         ++__count_;
         return true;
     }
     bool no_timeout = _Clock::now() < __t;
     while (no_timeout && __count_ != 0)
         no_timeout = __cv_.wait_until(lk, __t) == cv_status::no_timeout;
     if (__count_ == 0)

libcxx/include/thread

     friend _LIBCPP_INLINE_VISIBILITY
         bool operator<=(__thread_id __x, __thread_id __y) _NOEXCEPT
         {return !(__y < __x);}
     friend _LIBCPP_INLINE_VISIBILITY
         bool operator> (__thread_id __x, __thread_id __y) _NOEXCEPT
         {return   __y < __x ;}
     friend _LIBCPP_INLINE_VISIBILITY
         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
     basic_ostream<_CharT, _Traits>&
     operator<<(basic_ostream<_CharT, _Traits>& __os, __thread_id __id)
         {return __os << __id.__id_;}
 
 private:
     _LIBCPP_INLINE_VISIBILITY
     __thread_id(__libcpp_thread_id __id) : __id_(__id) {}

libcxx/src/mutex.cpp

 {
     lock_guard<mutex> _(__m_);
     __locked_ = false;
     __cv_.notify_one();
 }
 
 // recursive_timed_mutex
 
 recursive_timed_mutex::recursive_timed_mutex()
     : __count_(0),
-      __id_(0)
+      __id_{}
 {
 }
 
 recursive_timed_mutex::~recursive_timed_mutex()
 {
     lock_guard<mutex> _(__m_);
 }
 
 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");
         ++__count_;
         return;
     }
     while (__count_ != 0)
         __cv_.wait(lk);
     __count_ = 1;
     __id_ = id;
 }
 
 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;
         ++__count_;
         __id_ = id;
         return true;
     }
     return false;
 }
 
 void
 recursive_timed_mutex::unlock() _NOEXCEPT
 {
     unique_lock<mutex> lk(__m_);
     if (--__count_ == 0)
     {
-        __id_ = 0;
+        __id_.reset();
         lk.unlock();
         __cv_.notify_one();
     }
 }
 
 #endif // !_LIBCPP_HAS_NO_THREADS
 
 // If dispatch_once_f ever handles C++ exceptions, and if one can get to it
 // without illegal macros (unexpected macros not beginning with _UpperCase or
 // __lowercase), and if it stops spinning waiting threads, then call_once should