[Lldb-commits] [PATCH] D52543: [DWARFSymbolFile] Add the module lock where necessary and assert that we own it.

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

See inlined comments



================
Comment at: include/lldb/Symbol/SymbolFile.h:98
 
+  virtual std::recursive_mutex &GetModuleMutex() const;
+
----------------
Might add a comment here stating something like "Symbols file subclasses should override this to return the Module that owns the TypeSystem that this symbol file modifies type information in".


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2022-2025
+  if (m_debug_map_module_wp.expired())
+    return GetObjectFile()->GetModule()->GetMutex();
+  lldb::ModuleSP module_sp(m_debug_map_module_wp.lock());
+  return module_sp->GetMutex();
----------------
Possible race condition and crasher here. Best coded as:

```
  lldb::ModuleSP module_sp(m_debug_map_module_wp.lock());
  if (module_sp)
    return module_sp->GetMutex();
  return GetObjectFile()->GetModule()->GetMutex();
```

Otherwise some other thread could clear "m_debug_map_module_wp" after "m_debug_map_module_wp.expired()" is called, but before "m_debug_map_module_wp.lock()" is called and we would crash.


================
Comment at: source/Symbol/SymbolFile.cpp:160-168
+void SymbolFile::AssertModuleLock() {
+  // We assert that we have to module lock by trying to acquire the lock from a
+  // different thread. Note that we must abort if the result is true to
+  // guarantee correctness.
+  lldbassert(std::async(std::launch::async,
+                        [this] { return this->GetModuleMutex().try_lock(); })
+                     .get() == false &&
----------------
We should make this entire thing and all uses of it into a macro so we can enable it for Debug builds, but disable it for Release builds and have no extra code in release builds. We really shouldn't be starting threads up all the time for every function in SymbolFile subclasses for Release builds. It is ok for Debug builds or maybe manually enable this when we run into issues, but this is too expensive to leave in for all builds.


https://reviews.llvm.org/D52543