[Lldb-commits] [PATCH] D65025: [Symbol] Improve TypeSystemMap mutex safety

[Alex Langford via Phabricator via lldb-commits]

xiaobai created this revision.
xiaobai added reviewers: JDevlieghere, clayborg, davide, labath, jingham.

Relying on m_clear_in_progress is dangerous for modifying m_map. It's
entirely possible for one thread to invoke TypeSystemMap::Clear(), lock
the mutex, and copy the map for finalization while another thread tries
to add a new type system. This could end up in a situation where a type
system isn't finalized because of unsynchronized access.

I propose we remove the `m_clear_in_progress` variable entirely and rely
on the mutex.


https://reviews.llvm.org/D65025

Files:
  include/lldb/Symbol/TypeSystem.h
  source/Symbol/TypeSystem.cpp


Index: source/Symbol/TypeSystem.cpp
===================================================================
--- source/Symbol/TypeSystem.cpp
+++ source/Symbol/TypeSystem.cpp
@@ -155,32 +155,21 @@
 
 #pragma mark TypeSystemMap
 
-TypeSystemMap::TypeSystemMap()
-    : m_mutex(), m_map(), m_clear_in_progress(false) {}
+TypeSystemMap::TypeSystemMap() : m_mutex(), m_map() {}
 
 TypeSystemMap::~TypeSystemMap() {}
 
 void TypeSystemMap::Clear() {
-  collection map;
-  {
-    std::lock_guard<std::mutex> guard(m_mutex);
-    map = m_map;
-    m_clear_in_progress = true;
-  }
+  std::lock_guard<std::mutex> guard(m_mutex);
   std::set<TypeSystem *> visited;
-  for (auto pair : map) {
+  for (auto pair : m_map) {
     TypeSystem *type_system = pair.second.get();
     if (type_system && !visited.count(type_system)) {
       visited.insert(type_system);
       type_system->Finalize();
     }
   }
-  map.clear();
-  {
-    std::lock_guard<std::mutex> guard(m_mutex);
-    m_map.clear();
-    m_clear_in_progress = false;
-  }
+  m_map.clear();
 }
 
 void TypeSystemMap::ForEach(std::function<bool(TypeSystem *)> const &callback) {
@@ -210,7 +199,7 @@
     if (pair.second && pair.second->SupportsLanguage(language)) {
       // Add a new mapping for "language" to point to an already existing
       // TypeSystem that supports this language
-      AddToMap(language, pair.second);
+      m_map[language] = pair.second;
       return pair.second.get();
     }
   }
@@ -221,7 +210,7 @@
   // Cache even if we get a shared pointer that contains null type system back
   lldb::TypeSystemSP type_system_sp =
       TypeSystem::CreateInstance(language, module);
-  AddToMap(language, type_system_sp);
+  m_map[language] = type_system_sp;
   return type_system_sp.get();
 }
 
@@ -238,7 +227,7 @@
       // Add a new mapping for "language" to point to an already existing
       // TypeSystem that supports this language
 
-      AddToMap(language, pair.second);
+      m_map[language] = pair.second;
       return pair.second.get();
     }
   }
@@ -247,16 +236,8 @@
     return nullptr;
 
   // Cache even if we get a shared pointer that contains null type system back
-  lldb::TypeSystemSP type_system_sp;
-  if (!m_clear_in_progress)
-    type_system_sp = TypeSystem::CreateInstance(language, target);
-
-  AddToMap(language, type_system_sp);
+  lldb::TypeSystemSP type_system_sp =
+      TypeSystem::CreateInstance(language, target);
+  m_map[language] = type_system_sp;
   return type_system_sp.get();
 }
-
-void TypeSystemMap::AddToMap(lldb::LanguageType language,
-                             lldb::TypeSystemSP const &type_system_sp) {
-  if (!m_clear_in_progress)
-    m_map[language] = type_system_sp;
-}
Index: include/lldb/Symbol/TypeSystem.h
===================================================================
--- include/lldb/Symbol/TypeSystem.h
+++ include/lldb/Symbol/TypeSystem.h
@@ -498,16 +498,11 @@
                                        Target *target, bool can_create);
 
 protected:
-  // This function does not take the map mutex, and should only be called from
-  // functions that do take the mutex.
-  void AddToMap(lldb::LanguageType language,
-                lldb::TypeSystemSP const &type_system_sp);
 
   typedef std::map<lldb::LanguageType, lldb::TypeSystemSP> collection;
   mutable std::mutex m_mutex; ///< A mutex to keep this object happy in
                               ///multi-threaded environments.
   collection m_map;
-  bool m_clear_in_progress;
 };
 
 } // namespace lldb_private

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