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

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_lock<std::shared_mutex> &&lock,
+                            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()) {
+          std::lock_guard<std::shared_mutex> unlock(lock, std::adopt_lock);
           if (m_use_scratch_typesystem_per_module)
             DisplayFallbackSwiftContextErrors(swift_ast_ctx);
           else if (StreamSP errs = GetDebugger().GetAsyncErrorStream()) {
@@ -2716,66 +2716,77 @@ llvm::Optional<SwiftScratchContextReader> Target::GetSwiftScratchContext(
       }
     }
   }
-
-  auto get_or_create_fallback_context =
-      [&]() -> TypeSystemSwiftTypeRefForExpressions * {
-    if (!lldb_module || !m_use_scratch_typesystem_per_module)
-      return nullptr;
 
-    ModuleLanguage idx = {lldb_module, lldb::eLanguageTypeSwift};
-    auto cached = m_scratch_typesystem_for_module.find(idx);
-    if (cached != m_scratch_typesystem_for_module.end()) {
-      auto *cached_ts =
-          llvm::cast<TypeSystemSwiftTypeRefForExpressions>(cached->second.get());
-      if (!cached_ts)
-        return nullptr;
+  auto get_cached_module_ts =
+      [&](Module *lldb_module) -> TypeSystemSwiftTypeRefForExpressions * {
+    ModuleLanguage key = {lldb_module, lldb::eLanguageTypeSwift};
+    auto cached = m_scratch_typesystem_for_module.find(key);
+    if (cached != m_scratch_typesystem_for_module.end())
+      return llvm::cast<TypeSystemSwiftTypeRefForExpressions>(cached->second.get());
+    return nullptr;
+  };
+
+  auto maybe_create_fallback_context = [&]() {
+    ModuleLanguage key = {lldb_module, lldb::eLanguageTypeSwift};
+    if (auto *cached_ts = get_cached_module_ts(lldb_module)) {
       auto *cached_ast_ctx =
           llvm::dyn_cast_or_null<SwiftASTContextForExpressions>(
               cached_ts->GetSwiftASTContext());
       if (cached_ast_ctx && cached_ast_ctx->HasFatalErrors() &&
           !m_cant_make_scratch_type_system.count(lldb::eLanguageTypeSwift)) {
         DisplayFallbackSwiftContextErrors(cached_ast_ctx);
         // Try again.
-        m_scratch_typesystem_for_module.erase(cached);
-        return nullptr;
+        // FIXME: Shouldn't this continue rather than return?
+        auto &lock = GetSwiftScratchContextLock();
+        if (!lock.try_lock()) {
+          if (log)
+            log->Printf("module scratch context has errors but couldn't "
+                        "acquire scratch context lock\n");
+          return;
+        }
+        std::lock_guard<std::shared_mutex> unlock(lock, std::adopt_lock);
+        m_scratch_typesystem_for_module.erase(key);
+        if (log)
+          log->Printf("erased module-wide scratch context with errors\n");
+        return;
       }
       if (log)
         log->Printf("returned cached module-wide scratch context\n");
-      return cached_ts;
+      return;
     }
 
     if (!create_on_demand) {
       if (log)
         log->Printf("not allowed to create a new context\n");
-      return nullptr;
+      return;
     }
 
-    // Call for its side effects of establishing the Swift scratch type system.
+    // Call for its side effects of establishing the Swift scratch type
+    // system.
     auto type_system_or_err =
         GetScratchTypeSystemForLanguage(eLanguageTypeSwift, false);
     if (!type_system_or_err) {
       llvm::consumeError(type_system_or_err.takeError());
-      return nullptr;
+      return;
     }
 
-    if (!GetSwiftScratchContextLock().try_lock()) {
+    auto &lock = GetSwiftScratchContextLock();
+    if (!lock.try_lock()) {
       if (log)
         log->Printf("couldn't acquire scratch context lock\n");
-      return nullptr;
+      return;
     }
+    std::lock_guard<std::shared_mutex> unlock(lock, std::adopt_lock);
 
-    auto unlock = llvm::make_scope_exit(
-        [this] { GetSwiftScratchContextLock().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
-    // the brief window between the call to `GetScratchTypeSystemForLanguage`
-    // and taking the lock.
+    // 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 the brief window between the call to
+    // `GetScratchTypeSystemForLanguage` and taking the lock.
     type_system_or_err = m_scratch_type_system_map.GetTypeSystemForLanguage(
         eLanguageTypeSwift, this, false);
     if (!type_system_or_err) {
       llvm::consumeError(type_system_or_err.takeError());
-      return nullptr;
+      return;
     }
 
     if (auto *global_scratch_ctx =
@@ -2789,42 +2800,55 @@ llvm::Optional<SwiftScratchContextReader> Target::GetSwiftScratchContext(
     auto typesystem_sp = std::make_shared<TypeSystemSwiftTypeRefForExpressions>(
         lldb::eLanguageTypeSwift, *this, *lldb_module);
     typesystem_sp->GetSwiftASTContext();
-    m_scratch_typesystem_for_module.insert({idx, typesystem_sp});
+    m_scratch_typesystem_for_module.insert({key, typesystem_sp});
     if (log)
       log->Printf("created module-wide scratch context\n");
-    return typesystem_sp.get();
+    return;
   };
 
-  auto *swift_scratch_ctx = get_or_create_fallback_context();
-  if (!swift_scratch_ctx) {
-    if (log)
-      log->Printf("returned project-wide scratch context\n");
+  llvm::Optional<SwiftScratchContextReader> reader;
+  if (lldb_module && m_use_scratch_typesystem_per_module) {
+    maybe_create_fallback_context();
+    std::shared_lock<std::shared_mutex> lock(GetSwiftScratchContextLock());
+    if (auto *cached_ts = get_cached_module_ts(lldb_module)) {
+      reader = SwiftScratchContextReader(std::move(lock), *cached_ts);
+      if (log)
+        log->Printf("returned module-wide scratch context\n");
+    }
+  }
+  // FIXME: Don't return the project-wide context after requesting the
+  // module-wide one.
+  if (!reader) {
+    std::shared_lock<std::shared_mutex> lock(GetSwiftScratchContextLock());
     auto type_system_or_err =
         GetScratchTypeSystemForLanguage(eLanguageTypeSwift, create_on_demand);
-    if (type_system_or_err)
-      swift_scratch_ctx =
-          llvm::cast_or_null<TypeSystemSwiftTypeRefForExpressions>(
-              type_system_or_err->get());
-    else
+    if (type_system_or_err) {
+      if (auto *ts = llvm::cast_or_null<TypeSystemSwiftTypeRefForExpressions>(
+              type_system_or_err->get())) {
+        reader = SwiftScratchContextReader(std::move(lock), *ts);
+        if (log)
+          log->Printf("returned project-wide scratch context\n");
+      }
+    } else
       llvm::consumeError(type_system_or_err.takeError());
   }
 
-  StackFrameSP frame_sp = exe_scope.CalculateStackFrame();
-  if (frame_sp && frame_sp.get() && swift_scratch_ctx) {
-    SymbolContext sc =
-        frame_sp->GetSymbolContext(lldb::eSymbolContextEverything);
-    Status status = swift_scratch_ctx->PerformCompileUnitImports(sc);
-    if (status.Fail())
-      Debugger::ReportError(status.AsCString(), GetDebugger().GetID());
+  if (reader) {
+    // Perform compile unit imports.
+    assert(reader->get());
+    StackFrameSP frame_sp = exe_scope.CalculateStackFrame();
+    if (frame_sp && frame_sp.get()) {
+      SymbolContext sc =
+          frame_sp->GetSymbolContext(lldb::eSymbolContextEverything);
+      Status status = reader->get()->PerformCompileUnitImports(sc);
+      if (status.Fail())
+        Debugger::ReportError(status.AsCString(), GetDebugger().GetID());
+    }
   }
-
-  if (!swift_scratch_ctx)
-    return llvm::None;
-  return SwiftScratchContextReader(GetSwiftScratchContextLock(),
-                                   *swift_scratch_ctx);
+  return reader;
 }
 
-static SharedMutex *
+static std::shared_mutex *
 GetSwiftScratchContextMutex(const ExecutionContext *exe_ctx) {
   if (!exe_ctx)
     return nullptr;
@@ -2834,11 +2858,20 @@ GetSwiftScratchContextMutex(const ExecutionContext *exe_ctx) {
   return nullptr;
 }
 
+SwiftScratchContextReader::SwiftScratchContextReader(
+    std::shared_lock<std::shared_mutex> &&lock,
+    TypeSystemSwiftTypeRefForExpressions &ts)
+    : m_lock(std::move(lock)), 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 +2880,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) {