[libcxx] switch debug.cpp from using std::mutex to __libcpp_mutex_t

[DanielMcIntosh-IBM]

Summary

This is the 3rd of 4 changes to add support for POSIX(OFF) on z/OS.
See D117366 for more background, and D110349 for discussion of an
alternative implementation.

[• Quuxplusone]

libcxx/src/debug.cpp

73

This is great RAII cleanup!
Personally I'm not convinced that it makes sense to lose the tutorial distinction between WLock and RLock, though. Could you keep it as two structs db_read_lock and db_write_lock? Or is there a reason you think that that's a bad idea?
One benefit of db_read_lock guard; is that you can then eliminate the comment // Read lock because it'll be obvious.

Throughout, I think we should stay away from the identifier _ because people (GNU gettext, C++ pattern-matching proposals, ...) like to mess with it. In other codebases, my convention is std::lock_guard lk(m); i.e. using lk as the name of the lock variable. Of course guard also seems like a good name. ;)

270–287

This smells like you need db_guard to be .release()-able, so that you can eliminate all of the #if-stuff and manual unlocking currently here, and just do

guard.release();  // Keep the write lock locked when we return non-null
return p;

on line 294. (Write lock? Read lock? I didn't look closely enough.)

[DanielMcIntosh-IBM]

Address reveiw comments

libcxx/src/debug.cpp

73

Personally I'm not convinced that it makes sense to lose the tutorial distinction between WLock and RLock, though. Could you keep it as two structs db_read_lock and db_write_lock? Or is there a reason you think that that's a bad idea?
One benefit of db_read_lock guard; is that you can then eliminate the comment // Read lock because it'll be obvious.

Having separate structs for read and write locks to me would indicate that there was some need to separate them - e.g. a subtle difference in behaviour somehow, or at the very least the behaviour is likely to diverge in the future in a way that will be hard to implement retroactively if we don't keep them separate now. Neither of these is the case here, and IMO separating them sends the wrong message. It also feels akin to premature optimization - extra work done to solve a non-existent problem.

270–287

I'd considered that, but wasn't sure if it was worth it. I've changed it now though.

[• Quuxplusone]

libcxx/src/debug.cpp

69

73

hard to implement retroactively if we don't keep them separate now

FWIW, my intuition is basically that exact thing you said; the "hard" part is merely that the future programmer will have to figure out which of the mutex locks are "locking for reading" and which are "locking for writing," and then every code-reviewer on their review will have to check their work. We've already done the work (I assume), so I think we should keep it. (And keeping it as code makes more sense than keeping it as comments, even if the code is tantamount to comments anyway.)

using db_read_guard = db_guard;
using db_write_guard = db_guard;

Pay 3 lines of code, to save 15 lines of comments. Worth it IMO.

78

270–287

So worth it. :)

libcxx/src/debug.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "__config"
 #include "__debug"
 #include "functional"
 #include "algorithm"
 #include "string"
 #include "cstdio"
 #include "__hash_table"
 #ifndef _LIBCPP_HAS_NO_THREADS
-#include "mutex"
+#include "__threading_support"
 #if defined(__ELF__) && defined(_LIBCPP_LINK_PTHREAD_LIB)
 #pragma comment(lib, "pthread")
 #endif
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 std::string __libcpp_debug_info::what() const {

 {
     return __get_db();
 }
 
 namespace
 {
 
 #ifndef _LIBCPP_HAS_NO_THREADS
-typedef mutex mutex_type;
-typedef lock_guard<mutex_type> WLock;
-typedef lock_guard<mutex_type> RLock;
+_LIBCPP_SAFE_STATIC static __libcpp_mutex_t __db_mut = _LIBCPP_MUTEX_INITIALIZER;
 
-mutex_type&
-mut()
-{
-    static _LIBCPP_NO_DESTROY mutex_type m;
-    return m;
-}
-#endif // !_LIBCPP_HAS_NO_THREADS
+struct db_guard {
+  bool owns_lock = true;
+  db_guard() { __libcpp_mutex_lock(&__db_mut); }
+  ~db_guard() {
+    if (owns_lock)
+      __libcpp_mutex_unlock(&__db_mut);
+  }
+  void release() { owns_lock = false; }
+};
+#else
+struct db_guard {
+  void release() {};
+};
+#endif
 
 }  // unnamed namespace
 
 __i_node::~__i_node()
 {
     if (__next_)
     {
         __next_->~__i_node();
if (__ibeg_)
         }
         free(__ibeg_);
     }
 }
 
 void*
 __libcpp_db::__find_c_from_i(void* __i) const
 {
-#ifndef _LIBCPP_HAS_NO_THREADS
-    RLock _(mut());
-#endif
+    // Read lock
+    db_guard lk;
     __i_node* i = __find_iterator(__i);
     _LIBCPP_ASSERT(i != nullptr, "iterator not found in debug database.");
     return i->__c_ != nullptr ? i->__c_->__c_ : nullptr;
 }
 
 void
 __libcpp_db::__insert_ic(void* __i, const void* __c)
 {
-#ifndef _LIBCPP_HAS_NO_THREADS
-    WLock _(mut());
-#endif
+    // Write lock
+    db_guard lk;
     if (__cbeg_ == __cend_)
         return;
     size_t hc = hash<const void*>()(__c) % static_cast<size_t>(__cend_ - __cbeg_);
     __c_node* c = __cbeg_[hc];
     if (c == nullptr)
         return;
     while (c->__c_ != __c)
     {
         c = c->__next_;
         if (c == nullptr)
             return;
     }
     __i_node* i = __insert_iterator(__i);
     c->__add(i);
     i->__c_ = c;
 }
 
 void
 __libcpp_db::__insert_c(void* __c, __libcpp_db::_InsertConstruct *__fn)
 {
-#ifndef _LIBCPP_HAS_NO_THREADS
-    WLock _(mut());
-#endif
+    // Write lock
+    db_guard lk;
     if (__csz_ + 1 > static_cast<size_t>(__cend_ - __cbeg_))
     {
         size_t nc = __next_prime(2*static_cast<size_t>(__cend_ - __cbeg_) + 1);
         __c_node** cbeg = static_cast<__c_node**>(calloc(nc, sizeof(__c_node*)));
         if (cbeg == nullptr)
             __throw_bad_alloc();
 
         for (__c_node** p = __cbeg_; p != __cend_; ++p)
__libcpp_db::__insert_c(void* __c, __libcpp_db::_InsertConstruct *__fn)
     __cbeg_[hc] = __fn(buf, __c, p);
 
     ++__csz_;
 }
 
 void
 __libcpp_db::__erase_i(void* __i)
 {
-#ifndef _LIBCPP_HAS_NO_THREADS
-    WLock _(mut());
-#endif
+    // Write lock
+    db_guard lk;
     if (__ibeg_ != __iend_)
     {
         size_t hi = hash<void*>()(__i) % static_cast<size_t>(__iend_ - __ibeg_);
         __i_node* p = __ibeg_[hi];
         if (p != nullptr)
         {
             __i_node* q = nullptr;
             while (p->__i_ != __i)
if (__ibeg_ != __iend_)
             free(p);
         }
     }
 }
 
 void
 __libcpp_db::__invalidate_all(void* __c)
 {
-#ifndef _LIBCPP_HAS_NO_THREADS
-    WLock _(mut());
-#endif
+    // Write lock
+    db_guard lk;
     if (__cend_ != __cbeg_)
     {
         size_t hc = hash<void*>()(__c) % static_cast<size_t>(__cend_ - __cbeg_);
         __c_node* p = __cbeg_[hc];
         if (p == nullptr)
             return;
         while (p->__c_ != __c)
         {
             p = p->__next_;
             if (p == nullptr)
                 return;
         }
         while (p->end_ != p->beg_)
         {
             --p->end_;
             (*p->end_)->__c_ = nullptr;
         }
     }
 }
 
 __c_node*
 __libcpp_db::__find_c_and_lock(void* __c) const
 {
-#ifndef _LIBCPP_HAS_NO_THREADS
-    mut().lock();
-#endif
+    // Write lock - caller is likely to perform a write operation
+    db_guard lk;
     if (__cend_ == __cbeg_)
-    {
-#ifndef _LIBCPP_HAS_NO_THREADS
-        mut().unlock();
-#endif
         return nullptr;
-    }
+
     size_t hc = hash<void*>()(__c) % static_cast<size_t>(__cend_ - __cbeg_);
     __c_node* p = __cbeg_[hc];
     if (p == nullptr)
-    {
-#ifndef _LIBCPP_HAS_NO_THREADS
-        mut().unlock();
-#endif
         return nullptr;
-    }
+
     while (p->__c_ != __c)
     {
         p = p->__next_;
         if (p == nullptr)
-        {
-#ifndef _LIBCPP_HAS_NO_THREADS
-            mut().unlock();
-#endif
             return nullptr;
-        }
-    }
+    lk.release();
     return p;
 }
 
 __c_node*
 __libcpp_db::__find_c(void* __c) const
 {
     size_t hc = hash<void*>()(__c) % static_cast<size_t>(__cend_ - __cbeg_);
     __c_node* p = __cbeg_[hc];
     _LIBCPP_ASSERT(p != nullptr, "debug mode internal logic error __find_c A");
     while (p->__c_ != __c)
     {
         p = p->__next_;
         _LIBCPP_ASSERT(p != nullptr, "debug mode internal logic error __find_c B");
     }
     return p;
 }
 
 void
 __libcpp_db::unlock() const
 {
 #ifndef _LIBCPP_HAS_NO_THREADS
-    mut().unlock();
+    __libcpp_mutex_unlock(&__db_mut);
 #endif
 }
 
 void
 __libcpp_db::__erase_c(void* __c)
 {
-#ifndef _LIBCPP_HAS_NO_THREADS
-    WLock _(mut());
-#endif
+    // Write lock
+    db_guard lk;
     if (__cend_ != __cbeg_)
     {
         size_t hc = hash<void*>()(__c) % static_cast<size_t>(__cend_ - __cbeg_);
         __c_node* p = __cbeg_[hc];
         if (p == nullptr)
             return;
         __c_node* q = nullptr;
         _LIBCPP_ASSERT(p != nullptr, "debug mode internal logic error __erase_c A");
if (__cend_ != __cbeg_)
         free(p);
         --__csz_;
     }
 }
 
 void
 __libcpp_db::__iterator_copy(void* __i, const void* __i0)
 {
-#ifndef _LIBCPP_HAS_NO_THREADS
-    WLock _(mut());
-#endif
+    // Write lock
+    db_guard lk;
     __i_node* i = __find_iterator(__i);
     __i_node* i0 = __find_iterator(__i0);
     __c_node* c0 = i0 != nullptr ? i0->__c_ : nullptr;
     if (i == nullptr && i0 != nullptr)
         i = __insert_iterator(__i);
     __c_node* c = i != nullptr ? i->__c_ : nullptr;
     if (c != c0)
     {
if (c != c0)
             }
         }
     }
 }
 
 bool
 __libcpp_db::__dereferenceable(const void* __i) const
 {
-#ifndef _LIBCPP_HAS_NO_THREADS
-    RLock _(mut());
-#endif
+    // Read lock
+    db_guard lk;
     __i_node* i = __find_iterator(__i);
     return i != nullptr && i->__c_ != nullptr && i->__c_->__dereferenceable(__i);
 }
 
 bool
 __libcpp_db::__decrementable(const void* __i) const
 {
-#ifndef _LIBCPP_HAS_NO_THREADS
-    RLock _(mut());
-#endif
+    // Read lock
+    db_guard lk;
     __i_node* i = __find_iterator(__i);
     return i != nullptr && i->__c_ != nullptr && i->__c_->__decrementable(__i);
 }
 
 bool
 __libcpp_db::__addable(const void* __i, ptrdiff_t __n) const
 {
-#ifndef _LIBCPP_HAS_NO_THREADS
-    RLock _(mut());
-#endif
+    // Read lock
+    db_guard lk;
     __i_node* i = __find_iterator(__i);
     return i != nullptr && i->__c_ != nullptr && i->__c_->__addable(__i, __n);
 }
 
 bool
 __libcpp_db::__subscriptable(const void* __i, ptrdiff_t __n) const
 {
-#ifndef _LIBCPP_HAS_NO_THREADS
-    RLock _(mut());
-#endif
+    // Read lock
+    db_guard lk;
     __i_node* i = __find_iterator(__i);
     return i != nullptr && i->__c_ != nullptr && i->__c_->__subscriptable(__i, __n);
 }
 
 bool
 __libcpp_db::__less_than_comparable(const void* __i, const void* __j) const
 {
-#ifndef _LIBCPP_HAS_NO_THREADS
-    RLock _(mut());
-#endif
+    // Read lock
+    db_guard lk;
     __i_node* i = __find_iterator(__i);
     __i_node* j = __find_iterator(__j);
     __c_node* ci = i != nullptr ? i->__c_ : nullptr;
     __c_node* cj = j != nullptr ? j->__c_ : nullptr;
     return ci == cj;
 }
 
 void
 __libcpp_db::swap(void* c1, void* c2)
 {
-#ifndef _LIBCPP_HAS_NO_THREADS
-    WLock _(mut());
-#endif
+    // Write lock
+    db_guard lk;
     size_t hc = hash<void*>()(c1) % static_cast<size_t>(__cend_ - __cbeg_);
     __c_node* p1 = __cbeg_[hc];
     _LIBCPP_ASSERT(p1 != nullptr, "debug mode internal logic error swap A");
     while (p1->__c_ != c1)
     {
         p1 = p1->__next_;
         _LIBCPP_ASSERT(p1 != nullptr, "debug mode internal logic error swap B");
     }
for (__i_node** p = p1->beg_; p != p1->end_; ++p)
         (*p)->__c_ = p1;
     for (__i_node** p = p2->beg_; p != p2->end_; ++p)
         (*p)->__c_ = p2;
 }
 
 void
 __libcpp_db::__insert_i(void* __i)
 {
-#ifndef _LIBCPP_HAS_NO_THREADS
-    WLock _(mut());
-#endif
+    // Write lock
+    db_guard lk;
     __insert_iterator(__i);
 }
 
 void
 __c_node::__add(__i_node* i)
 {
     if (end_ == cap_)
     {