[Lldb-commits] [PATCH] D35083: [TypeSystem] Guard the global `ASTSourceMap` with a mutex

[Sean Callanan via Phabricator via lldb-commits]

spyffe created this revision.

`s_source_map` in `ClangExternalASTSourceCommon.cpp` is unguarded and therefore 
can break in multithreaded conditions.  This can cause crashes in particular if 
multiple targets are being set up at once.

This patch wraps `s_source_map` in a function that ensures exclusivity, and 
makes every user of it use that function instead.


Repository:
  rL LLVM

https://reviews.llvm.org/D35083

Files:
  source/Symbol/ClangExternalASTSourceCommon.cpp


Index: source/Symbol/ClangExternalASTSourceCommon.cpp
===================================================================
--- source/Symbol/ClangExternalASTSourceCommon.cpp
+++ source/Symbol/ClangExternalASTSourceCommon.cpp
@@ -10,41 +10,55 @@
 #include "lldb/Symbol/ClangExternalASTSourceCommon.h"
 #include "lldb/Utility/Stream.h"
 
+#include <mutex>
+
 using namespace lldb_private;
 
 uint64_t g_TotalSizeOfMetadata = 0;
 
 typedef llvm::DenseMap<clang::ExternalASTSource *,
                        ClangExternalASTSourceCommon *>
     ASTSourceMap;
 
-static ASTSourceMap &GetSourceMap() {
+template <typename FnType>
+static decltype(std::declval<FnType>()(std::declval<ASTSourceMap&>()))
+WithExclusiveSourceMap(FnType fn) {
   // Intentionally leaked to avoid problems with global destructors.
   static ASTSourceMap *s_source_map = new ASTSourceMap;
-  return *s_source_map;
+  static std::mutex s_source_map_mutex;
+  
+  {
+    std::lock_guard<std::mutex> source_map_locker(s_source_map_mutex);
+    return fn(*s_source_map);
+  }
 }
 
 ClangExternalASTSourceCommon *
 ClangExternalASTSourceCommon::Lookup(clang::ExternalASTSource *source) {
-  ASTSourceMap &source_map = GetSourceMap();
-
-  ASTSourceMap::iterator iter = source_map.find(source);
-
-  if (iter != source_map.end()) {
-    return iter->second;
-  } else {
-    return nullptr;
-  }
+  return WithExclusiveSourceMap(
+      [source](ASTSourceMap &source_map) -> ClangExternalASTSourceCommon * {
+    ASTSourceMap::iterator iter = source_map.find(source);
+    
+    if (iter != source_map.end()) {
+      return iter->second;
+    } else {
+      return nullptr;
+    }
+  });
 }
 
 ClangExternalASTSourceCommon::ClangExternalASTSourceCommon()
     : clang::ExternalASTSource() {
+  WithExclusiveSourceMap([this](ASTSourceMap &source_map) {
+    source_map[this] = this;
+  });
   g_TotalSizeOfMetadata += m_metadata.size();
-  GetSourceMap()[this] = this;
 }
 
 ClangExternalASTSourceCommon::~ClangExternalASTSourceCommon() {
-  GetSourceMap().erase(this);
+  WithExclusiveSourceMap([this](ASTSourceMap &source_map) {
+    source_map.erase(this);
+  });
   g_TotalSizeOfMetadata -= m_metadata.size();
 }
 



Index: source/Symbol/ClangExternalASTSourceCommon.cpp
===================================================================
--- source/Symbol/ClangExternalASTSourceCommon.cpp
+++ source/Symbol/ClangExternalASTSourceCommon.cpp
@@ -10,41 +10,55 @@
 #include "lldb/Symbol/ClangExternalASTSourceCommon.h"
 #include "lldb/Utility/Stream.h"
 
+#include <mutex>
+
 using namespace lldb_private;
 
 uint64_t g_TotalSizeOfMetadata = 0;
 
 typedef llvm::DenseMap<clang::ExternalASTSource *,
                        ClangExternalASTSourceCommon *>
     ASTSourceMap;
 
-static ASTSourceMap &GetSourceMap() {
+template <typename FnType>
+static decltype(std::declval<FnType>()(std::declval<ASTSourceMap&>()))
+WithExclusiveSourceMap(FnType fn) {
   // Intentionally leaked to avoid problems with global destructors.
   static ASTSourceMap *s_source_map = new ASTSourceMap;
-  return *s_source_map;
+  static std::mutex s_source_map_mutex;
+  
+  {
+    std::lock_guard<std::mutex> source_map_locker(s_source_map_mutex);
+    return fn(*s_source_map);
+  }
 }
 
 ClangExternalASTSourceCommon *
 ClangExternalASTSourceCommon::Lookup(clang::ExternalASTSource *source) {
-  ASTSourceMap &source_map = GetSourceMap();
-
-  ASTSourceMap::iterator iter = source_map.find(source);
-
-  if (iter != source_map.end()) {
-    return iter->second;
-  } else {
-    return nullptr;
-  }
+  return WithExclusiveSourceMap(
+      [source](ASTSourceMap &source_map) -> ClangExternalASTSourceCommon * {
+    ASTSourceMap::iterator iter = source_map.find(source);
+    
+    if (iter != source_map.end()) {
+      return iter->second;
+    } else {
+      return nullptr;
+    }
+  });
 }
 
 ClangExternalASTSourceCommon::ClangExternalASTSourceCommon()
     : clang::ExternalASTSource() {
+  WithExclusiveSourceMap([this](ASTSourceMap &source_map) {
+    source_map[this] = this;
+  });
   g_TotalSizeOfMetadata += m_metadata.size();
-  GetSourceMap()[this] = this;
 }
 
 ClangExternalASTSourceCommon::~ClangExternalASTSourceCommon() {
-  GetSourceMap().erase(this);
+  WithExclusiveSourceMap([this](ASTSourceMap &source_map) {
+    source_map.erase(this);
+  });
   g_TotalSizeOfMetadata -= m_metadata.size();
 }
 

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits