[Lldb-commits] [PATCH] D52543: [DWARFSymbolFile] Adds the module lock to all virtual methods from SymbolFile

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

It is the SymbolVendor's job to do the locking. And in many cases it already is. I stopped with inlined comments after a few comments as it would apply to this entire patch.

It would be great if we can replace all of the locations where you added lock guards with lldbasserts to assert that the lock is already taken. Then we might be able to catch places where the locks are not being respected. But in general the symbol vendor should take the lock, use N number of SymbolFile instances to complete the request, and then release the lock. Makes things easier on the lldb_private::SymbolFile subclasses this way.



================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:266-267
 TypeList *SymbolFileDWARF::GetTypeList() {
+  std::lock_guard<std::recursive_mutex> guard(
+      GetObjectFile()->GetModule()->GetMutex());
   SymbolFileDWARFDebugMap *debug_map_symfile = GetDebugMapSymfile();
----------------
This shouldn't require locking as it is just handing out a type list ivar from one place or another.


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:277-278
                                TypeSet &type_set) {
+  std::lock_guard<std::recursive_mutex> guard(
+      GetObjectFile()->GetModule()->GetMutex());
   if (die) {
----------------
This is not a SymbolFile override. It shouldn't require the locking.


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:442-443
 TypeSystem *SymbolFileDWARF::GetTypeSystemForLanguage(LanguageType language) {
+  std::lock_guard<std::recursive_mutex> guard(
+      GetObjectFile()->GetModule()->GetMutex());
   SymbolFileDWARFDebugMap *debug_map_symfile = GetDebugMapSymfile();
----------------
This shouldn't require locking as it is just handing out a type system from one place or another.


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:857-858
 uint32_t SymbolFileDWARF::GetNumCompileUnits() {
+  std::lock_guard<std::recursive_mutex> guard(
+      GetObjectFile()->GetModule()->GetMutex());
   DWARFDebugInfo *info = DebugInfo();
----------------
SymbolVendor::GetNumCompileUnits() already takes the module lock for us. All other internal calls to this should be protected as well


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:866-867
 CompUnitSP SymbolFileDWARF::ParseCompileUnitAtIndex(uint32_t cu_idx) {
+  std::lock_guard<std::recursive_mutex> guard(
+      GetObjectFile()->GetModule()->GetMutex());
   CompUnitSP cu_sp;
----------------
SymbolVendor already takes the module lock for us


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:880-881
                                                     const DWARFDIE &die) {
+  std::lock_guard<std::recursive_mutex> guard(
+      GetObjectFile()->GetModule()->GetMutex());
   if (die.IsValid()) {
----------------
SymbolVendor requests that lead to this call should take the lock for us...


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52543