DWZ 07/11: Protect DWARFDebugInfo::m_compile_units by a new mutex

[jankratochvil]

Summary

DW_TAG_partial_unit is remapped on each of its use, those remapped virtual units are added into DWARFDebugInfo::m_compile_units during multithreaded indexing.

I am not sure what do you prefer about m_compile_units_size_atomic. Overhead of the new mutex will not be measurable in practice I think:
(1) Drop any such atomic_t for its size and always use the mutex.
(2) Use atomic_t to prevent mutex locking in most common cases. But maintain it separately which is error prone during newly coded m_compile_units updates. --- attached patch implements this
(3) Provide m_compile_units class wrapper which transparently updates also its atomic_t.

All DWZ patches are also applied in: git clone -b dwz git://git.jankratochvil.net/lldb

[clayborg]

DWARFDebugInfo::ParseCompileUnitHeadersIfNeeded() seems broken, see inlined comments.

source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp

115–120

So if some code calls DWARFDebugInfo::ParseCompileUnitHeadersIfNeeded() after this loop has added 1 compile unit they will return and be able to proceed? This doesn't make sense.

source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h

80

Why is this named m_dwz_uniq_mutex? Seems to be protected m_compile_units. Should this be named "m_compile_units_mutex"?

[jankratochvil]

Without FileOffset and the remapping to unique DIE offsets for each DW_TAG_partial_unit inclusion by DW_TAG_imported_unit this patch is no longer needed, as discussed in: https://lists.llvm.org/pipermail/lldb-commits/Week-of-Mon-20180409/040324.html

source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h

 #include <map>
 #include <vector>
 
 #include "DWARFUnit.h"
 #include "DWARFDIE.h"
 #include "SymbolFileDWARF.h"
 #include "lldb/Core/STLUtils.h"
 #include "lldb/lldb-private.h"
+#include "llvm/Support/RWMutex.h"
 
 typedef std::multimap<const char *, dw_offset_t, CStringCompareFunctionObject>
     CStringToDIEMap;
 typedef CStringToDIEMap::iterator CStringToDIEMapIter;
 typedef CStringToDIEMap::const_iterator CStringToDIEMapConstIter;
 
 class DWARFDebugInfo {
 public:
protected:
   //----------------------------------------------------------------------
   // Member variables
   //----------------------------------------------------------------------
   SymbolFileDWARF *m_dwarf2Data;
   CompileUnitColl m_compile_units;
   std::unique_ptr<DWARFDebugAranges>
       m_cu_aranges_ap; // A quick address to compile unit table
 
+  std::atomic_size_t m_compile_units_size_atomic { 0 };
+  mutable llvm::sys::RWMutex m_compile_units_mutex;
+
 private:
   // All parsing needs to be done partially any managed by this class as
   // accessors are called.
   void ParseCompileUnitHeadersIfNeeded();
 
   DISALLOW_COPY_AND_ASSIGN(DWARFDebugInfo);
 };
 
 #endif // SymbolFileDWARF_DWARFDebugInfo_h_

source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp

 //----------------------------------------------------------------------
 DWARFDebugInfo::DWARFDebugInfo()
     : m_dwarf2Data(NULL), m_compile_units(), m_cu_aranges_ap() {}
 
 //----------------------------------------------------------------------
 // SetDwarfData
 //----------------------------------------------------------------------
 void DWARFDebugInfo::SetDwarfData(SymbolFileDWARF *dwarf2Data) {
+  llvm::sys::ScopedWriter lock(m_compile_units_mutex);
+  m_compile_units_size_atomic = 0;
   m_dwarf2Data = dwarf2Data;
   m_compile_units.clear();
 }
 
 DWARFDebugAranges &DWARFDebugInfo::GetCompileUnitAranges() {
   if (m_cu_aranges_ap.get() == NULL && m_dwarf2Data) {
     Log *log(LogChannelDWARF::GetLogIfAll(DWARF_LOG_DEBUG_ARANGES));
 
if (m_cu_aranges_ap.get() == NULL && m_dwarf2Data) {
 
     const bool minimize = true;
     m_cu_aranges_ap->Sort(minimize);
   }
   return *m_cu_aranges_ap.get();
 }
 
 void DWARFDebugInfo::ParseCompileUnitHeadersIfNeeded() {
-  if (m_compile_units.empty()) {
-    if (m_dwarf2Data != NULL) {
-      lldb::offset_t file_offset = 0;
-      DWARFUnitSP cu_sp;
-      while ((cu_sp = DWARFCompileUnit::Extract(m_dwarf2Data, &file_offset))) {
-        m_compile_units.push_back(cu_sp);
+  if (m_compile_units_size_atomic)
+    return;
+  if (m_dwarf2Data == NULL)
+    return;
+  llvm::sys::ScopedWriter lock(m_compile_units_mutex);
+  if (!m_compile_units.empty())
+    return;
 
-        file_offset = cu_sp->GetNextCompileUnitFileOffset();
-      }
-    }
-  }
+  m_compile_units_size_atomic = m_compile_units.size();
 }
 
 size_t DWARFDebugInfo::GetNumCompileUnits() {
   ParseCompileUnitHeadersIfNeeded();
-  return m_compile_units.size();
+  return m_compile_units_size_atomic;
 }
 
 DWARFUnit *DWARFDebugInfo::GetCompileUnitAtIndex(uint32_t idx) {
   DWARFUnit *cu = NULL;
-  if (idx < GetNumCompileUnits())
+  if (idx < GetNumCompileUnits()) {
+    llvm::sys::ScopedReader lock(m_compile_units_mutex);
     cu = m_compile_units[idx].get();
+  }
   return cu;
 }
 
 bool DWARFDebugInfo::ContainsCompileUnit(const DWARFUnit *cu) const {
+  llvm::sys::ScopedReader lock(m_compile_units_mutex);
   // Not a verify efficient function, but it is handy for use in assertions
   // to make sure that a compile unit comes from a debug information file.
   CompileUnitColl::const_iterator end_pos = m_compile_units.end();
   CompileUnitColl::const_iterator pos;
 
   for (pos = m_compile_units.begin(); pos != end_pos; ++pos) {
     if (pos->get() == cu)
       return true;
   }
   return false;
 }
 
 bool DWARFDebugInfo::OffsetLessThanCompileUnitOffset(
     dw_offset_t offset, const DWARFUnitSP &cu_sp) {
   return offset < cu_sp->GetOffset();
 }
 
 DWARFUnit *DWARFDebugInfo::GetCompileUnit(dw_offset_t cu_offset,
                                                  uint32_t *idx_ptr) {
   DWARFUnitSP cu_sp;
   uint32_t cu_idx = DW_INVALID_INDEX;
   if (cu_offset != DW_INVALID_OFFSET) {
     ParseCompileUnitHeadersIfNeeded();
+    llvm::sys::ScopedReader lock(m_compile_units_mutex);
 
     // Watch out for single compile unit executable as they are pretty common
     const size_t num_cus = m_compile_units.size();
     if (num_cus == 1) {
       if (m_compile_units[0]->GetOffset() == cu_offset) {
         cu_sp = m_compile_units[0];
         cu_idx = 0;
       }
if (die_ref.cu_offset == DW_INVALID_OFFSET)
     return GetCompileUnitContainingDIEOffset(die_ref.die_offset);
   else
     return GetCompileUnit(die_ref.cu_offset);
 }
 
 DWARFUnit *
 DWARFDebugInfo::GetCompileUnitContainingDIEOffset(dw_offset_t die_offset) {
   ParseCompileUnitHeadersIfNeeded();
+  llvm::sys::ScopedReader lock(m_compile_units_mutex);
 
   DWARFUnitSP cu_sp;
 
   // Watch out for single compile unit executable as they are pretty common
   const size_t num_cus = m_compile_units.size();
   if (num_cus == 1) {
     if (m_compile_units[0]->ContainsDIEOffset(die_offset))
       return m_compile_units[0].get();
void DWARFDebugInfo::Dump(Stream *s, const uint32_t die_offset,
                           const uint32_t recurse_depth) {
   DumpInfo dumpInfo(s, die_offset, recurse_depth);
 
   s->PutCString("Dumping .debug_info section from internal representation\n");
 
   CompileUnitColl::const_iterator pos;
   uint32_t curr_depth = 0;
   ParseCompileUnitHeadersIfNeeded();
+  llvm::sys::ScopedReader lock(m_compile_units_mutex);
   for (pos = m_compile_units.begin(); pos != m_compile_units.end(); ++pos) {
     DWARFUnit *cu = pos->get();
     DumpCallback(m_dwarf2Data, cu, NULL, 0, curr_depth, &dumpInfo);
 
     const DWARFDIE die = cu->DIE();
     if (die)
       die.Dump(s, recurse_depth);
   }
 }