Replace hand-rolled SharedMutex with C++17 std::shared_mutex (NFCI)

We are getting reports where LLDB is hanging in try_lock().  This
patch makes the code easier to reason about by using standard
primitives, and avoids the pitfall of the copy constructor that the
old code was still allowing. While this may not fix the hangs, it
should certainly make it easier to diagnose.

rdar://112025620

Author: adrian-prantl

lldb/include/lldb/Core/SwiftScratchContextReader.h

@@ -13,64 +13,14 @@
 #ifndef liblldb_SwiftASTContextReader_h_
 #define liblldb_SwiftASTContextReader_h_
 
-#include "llvm/Support/RWMutex.h"
+#include <shared_mutex>
 
 namespace lldb_private {
 
 class TypeSystemSwiftTypeRefForExpressions;
 class ExecutionContext;
 class ExecutionContextRef;
 
-/// This is like llvm::sys::SmartRWMutex<true>, but with a try_lock method.
-///
-/// FIXME: Replace this with a C++14 shared_timed_mutex or a C++17
-/// share_mutex as soon as possible.
-class SharedMutex {
-  llvm::sys::SmartRWMutex<true> m_impl;
-  unsigned m_readers = 0;
-  std::mutex m_reader_mutex;
-public:
-  SharedMutex() = default;
-  SharedMutex(const SharedMutex &) = delete;
-  SharedMutex &operator=(const SharedMutex &) = delete;
-  bool lock_shared() {
-    std::lock_guard<std::mutex> lock(m_reader_mutex);
-    ++m_readers;
-    return m_impl.lock_shared();
-  }
-  bool unlock_shared() {
-    std::lock_guard<std::mutex> lock(m_reader_mutex);
-    assert(m_readers > 0);
-    --m_readers;
-    return m_impl.unlock_shared();
-  }
-  bool try_lock() {
-    std::lock_guard<std::mutex> lock(m_reader_mutex);
-    return m_readers ? false : m_impl.lock();
-  }
-  bool unlock() { return m_impl.unlock(); }
-};
-
-/// RAII acquisition of a reader lock.
-struct ScopedSharedMutexReader {
-  SharedMutex *m_mutex;
-  explicit ScopedSharedMutexReader(SharedMutex *m) : m_mutex(m) {
-    if (m_mutex)
-      m_mutex->lock_shared();
-  }
-
-  ScopedSharedMutexReader(const ScopedSharedMutexReader &copy)
-      : m_mutex(copy.m_mutex) {
-    if (m_mutex)
-      m_mutex->lock_shared();
-  }
-
-  ~ScopedSharedMutexReader() {
-    if (m_mutex)
-      m_mutex->unlock_shared();
-  }
-};
-
 /// A scratch Swift context pointer and its reader lock.
 /// The Swift scratch context may need to be replaced when it gets corrupted,
 /// for example due to incompatible ClangImporter options. This locking
@@ -97,27 +47,25 @@ struct ScopedSharedMutexReader {
 /// or even separate scratch contexts for each lldb::Module. But it
 /// will only do this if no client holds on to a read lock on \b
 /// m_scratch_typesystem_lock.
-class SwiftScratchContextReader : ScopedSharedMutexReader {
-  TypeSystemSwiftTypeRefForExpressions *m_ptr;
+class SwiftScratchContextReader {
+  std::shared_lock<std::shared_mutex> m_lock;
+  TypeSystemSwiftTypeRefForExpressions *m_ts;
 
 public:
-  SwiftScratchContextReader(SharedMutex &mutex,
-                            TypeSystemSwiftTypeRefForExpressions &ctx)
-      : ScopedSharedMutexReader(&mutex), m_ptr(&ctx) {
-    assert(m_ptr && "invalid context");
-  }
-
-  TypeSystemSwiftTypeRefForExpressions *get() {
-    assert(m_ptr && "invalid context");
-    return m_ptr;
-  }
-
+  SwiftScratchContextReader(std::shared_mutex &mutex,
+                            TypeSystemSwiftTypeRefForExpressions &ts);
+  SwiftScratchContextReader(const SwiftScratchContextReader &) = delete;
+  SwiftScratchContextReader(SwiftScratchContextReader &&other) = default;
+  SwiftScratchContextReader &
+  operator=(SwiftScratchContextReader &&other) = default;
+  TypeSystemSwiftTypeRefForExpressions *get() { return m_ts; }
   TypeSystemSwiftTypeRefForExpressions *operator->() { return get(); }
   TypeSystemSwiftTypeRefForExpressions &operator*() { return *get(); }
 };
 
 /// An RAII object that just acquires the reader lock.
-struct SwiftScratchContextLock : ScopedSharedMutexReader {
+struct SwiftScratchContextLock {
+  std::shared_lock<std::shared_mutex> lock;
   SwiftScratchContextLock(const ExecutionContextRef *exe_ctx_ref);
   SwiftScratchContextLock(const ExecutionContext *exe_ctx);
 };

lldb/include/lldb/Target/Target.h

@@ -1257,7 +1257,7 @@ class Target : public std::enable_shared_from_this<Target>,
 
 #ifdef LLDB_ENABLE_SWIFT
   /// Get the lock guarding the scratch typesystem from being re-initialized.
-  SharedMutex &GetSwiftScratchContextLock() {
+  std::shared_mutex &GetSwiftScratchContextLock() {
     return m_scratch_typesystem_lock;
   }
 
@@ -1694,7 +1694,7 @@ class Target : public std::enable_shared_from_this<Target>,
       m_scratch_typesystem_for_module;
 
   /// Guards the scratch typesystem from being re-initialized.
-  SharedMutex m_scratch_typesystem_lock;
+  std::shared_mutex m_scratch_typesystem_lock;
 
   static void ImageSearchPathsChanged(const PathMappingList &path_list,
                                       void *baton);

lldb/source/Core/ValueObjectDynamicValue.cpp

@@ -450,8 +450,8 @@ bool ValueObjectDynamicValue::DynamicValueTypeInfoNeedsUpdate() {
 
 #ifdef LLDB_ENABLE_SWIFT
   auto cached_ctx = m_value.GetCompilerType().GetTypeSystem();
-  llvm::Optional<SwiftScratchContextReader> scratch_ctx =
-      GetSwiftScratchContext();
+  llvm::Optional<SwiftScratchContextReader> scratch_ctx(
+      GetSwiftScratchContext());
 
   if (!scratch_ctx || !cached_ctx)
     return true;

lldb/source/Target/Target.cpp

@@ -2426,12 +2426,12 @@ Target::GetScratchTypeSystemForLanguage(lldb::LanguageType language,
       if (swift_ast_ctx && (swift_ast_ctx->CheckProcessChanged() ||
                             swift_ast_ctx->HasFatalErrors())) {
         // If it is safe to replace the scratch context, do so. If
-        // try_lock() fails, then higher stack frame (or another
+        // try_lock() fails, then a higher stack frame (or another
         // thread) is holding a read lock to the scratch context and
         // replacing it could cause a use-after-free later on.
-        if (GetSwiftScratchContextLock().try_lock()) {
-          auto unlock = llvm::make_scope_exit(
-              [this] { GetSwiftScratchContextLock().unlock(); });
+        auto &lock = GetSwiftScratchContextLock();
+        if (lock.try_lock()) {
+          auto unlock = llvm::make_scope_exit([&lock] { lock.unlock(); });
           if (m_use_scratch_typesystem_per_module)
             DisplayFallbackSwiftContextErrors(swift_ast_ctx);
           else if (StreamSP errs = GetDebugger().GetAsyncErrorStream()) {
@@ -2758,14 +2758,13 @@ llvm::Optional<SwiftScratchContextReader> Target::GetSwiftScratchContext(
       return nullptr;
     }
 
-    if (!GetSwiftScratchContextLock().try_lock()) {
+    auto &lock = GetSwiftScratchContextLock();
+    if (!lock.try_lock()) {
       if (log)
         log->Printf("couldn't acquire scratch context lock\n");
       return nullptr;
     }
-
-    auto unlock = llvm::make_scope_exit(
-        [this] { GetSwiftScratchContextLock().unlock(); });
+    auto unlock = llvm::make_scope_exit([&lock] { lock.unlock(); });
 
     // With the lock held, get the current scratch type system. This ensures the
     // current instance is used even in the unlikely event it was changed during
@@ -2824,7 +2823,7 @@ llvm::Optional<SwiftScratchContextReader> Target::GetSwiftScratchContext(
                                    *swift_scratch_ctx);
 }
 
-static SharedMutex *
+static std::shared_mutex *
 GetSwiftScratchContextMutex(const ExecutionContext *exe_ctx) {
   if (!exe_ctx)
     return nullptr;
@@ -2834,11 +2833,19 @@ GetSwiftScratchContextMutex(const ExecutionContext *exe_ctx) {
   return nullptr;
 }
 
+SwiftScratchContextReader::SwiftScratchContextReader(
+    std::shared_mutex &mutex, TypeSystemSwiftTypeRefForExpressions &ts)
+    : m_lock(mutex), m_ts(&ts) {}
+
 SwiftScratchContextLock::SwiftScratchContextLock(
-    const ExecutionContext *exe_ctx)
-    : ScopedSharedMutexReader(GetSwiftScratchContextMutex(exe_ctx)) {}
+    const ExecutionContext *exe_ctx) {
+  if (auto *mutex = GetSwiftScratchContextMutex(exe_ctx)) {
+    std::shared_lock<std::shared_mutex> tmp(*mutex);
+    lock.swap(tmp);
+  }
+}
 
-static SharedMutex *
+static std::shared_mutex *
 GetSwiftScratchContextMutex(const ExecutionContextRef *exe_ctx_ref) {
   if (!exe_ctx_ref)
     return nullptr;
@@ -2847,8 +2854,12 @@ GetSwiftScratchContextMutex(const ExecutionContextRef *exe_ctx_ref) {
 }
 
 SwiftScratchContextLock::SwiftScratchContextLock(
-    const ExecutionContextRef *exe_ctx_ref)
-    : ScopedSharedMutexReader(GetSwiftScratchContextMutex(exe_ctx_ref)) {}
+    const ExecutionContextRef *exe_ctx_ref) {
+  if (auto *mutex = GetSwiftScratchContextMutex(exe_ctx_ref)) {
+    std::shared_lock<std::shared_mutex> tmp(*mutex);
+    lock.swap(tmp);
+  }
+}
 
 void Target::DisplayFallbackSwiftContextErrors(
     SwiftASTContextForExpressions *swift_ast_ctx) {