[Symbol] Improve TypeSystemMap mutex safety

[xiaobai]

Summary

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.

[xiaobai]

Actually from the looks of it, I completely misunderstood what's going on here. It looks like AddToMap should only be called by things that hold the mutex, meaning that this change isn't actually necessary. I do think that makes this code kind of frustrating to understand though. Closing.

[grandinj]

Maybe make AddMap take a lock_guard parameter as recommended here

https://stackoverflow.com/questions/28825749/ensuring-that-current-thread-holds-a-lock-on-a-c11-mutex

?

[clayborg]

I fear this will deadlock again now. The original reason for the m_clear_in_progress was:

r260624 | jingham | 2016-02-11 16:03:19 -0800 (Thu, 11 Feb 2016) | 14 lines

When calling TypeSystemMap::Clear, objects being destroyed in the process of
clearing the map ended up calling back into the TypeSystemMap to do lookups.
Not a good idea, and in this case it would cause a deadlock.

You would only see this when replacing the target contents after an exec, and only if you
had stopped before the exec, evaluated an expression, then continued
on to the point where you did the exec.

Fixed this by making sure the TypeSystemMap::Clear tears down the TypeSystems in the map before clearing the map.
I also add an expression before exec to the TestExec.py so that we'll catch this
issue if it crops up again in the future.

rdar://problem/24554920

include/lldb/Symbol/TypeSystem.h

public:
 
   TypeSystem *GetTypeSystemForLanguage(lldb::LanguageType language,
                                        Module *module, bool can_create);
 
   TypeSystem *GetTypeSystemForLanguage(lldb::LanguageType language,
                                        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
 
 #endif // liblldb_TypeSystem_h_

source/Symbol/TypeSystem.cpp

 std::vector<CompilerDecl>
 TypeSystem::DeclContextFindDeclByName(void *opaque_decl_ctx, ConstString name,
                                       bool ignore_imported_decls) {
   return std::vector<CompilerDecl>();
 }
 
 #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::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;
-  }
 }
 
 void TypeSystemMap::ForEach(std::function<bool(TypeSystem *)> const &callback) {
   std::lock_guard<std::mutex> guard(m_mutex);
   // Use a std::set so we only call the callback once for each unique
   // TypeSystem instance
   std::set<TypeSystem *> visited;
   for (auto pair : m_map) {
TypeSystem *TypeSystemMap::GetTypeSystemForLanguage(lldb::LanguageType language,
   collection::iterator pos = m_map.find(language);
   if (pos != m_map.end())
     return pos->second.get();
 
   for (const auto &pair : m_map) {
     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();
     }
   }
 
   if (!can_create)
     return nullptr;
 
   // 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();
 }
 
 TypeSystem *TypeSystemMap::GetTypeSystemForLanguage(lldb::LanguageType language,
                                                     Target *target,
                                                     bool can_create) {
   std::lock_guard<std::mutex> guard(m_mutex);
   collection::iterator pos = m_map.find(language);
   if (pos != m_map.end())
     return pos->second.get();
 
   for (const auto &pair : m_map) {
     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();
     }
   }
 
   if (!can_create)
     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);
-  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;
+  lldb::TypeSystemSP type_system_sp =
+      TypeSystem::CreateInstance(language, target);
+  return type_system_sp.get();
 }