Protect DWARFCompileUnit::m_die_array by a new mutex

[jankratochvil]

Summary

I find it is unrelated to DW_TAG_partial_unit. For example LLDB Python Reference says:

consider that lldb can run in a multithreaded environment, and another thread might call the "script" command, changing the value out from under you.

Therefore if BuildAddressRangeTable called ExtractDIEsIfNeeded(false), then another thread started processing data from m_die_array and then the first thread called final ClearDIEs() the second thread would crash, wouldn't it?

DW_TAG_partial_unit for DWZ would get solved even by this patch.

This patch requires D46810.

Some notes of mine:

ExtractDIEsIfNeeded ro/rw, 0->1, 1->n, 0->n
AddUnitDIE rw, called with lock by: DWARFUnit::ExtractDIEsIfNeeded
AppendDIEsWithTag ro, called by ParseCompileUnitFunctions,
                      called by Module::ParseAllDebugSymbols after ParseVariablesForContext using ExtractDIEsIfNeeded(false)
ClearDIEs ro/rw, called by BuildAddressRangeTable, SymbolFileDWARF::Index
GetDIE expand+ro, called by MANY, only using m_die_array after: ExtractDIEsIfNeeded(false)
IndexPrivate ro, called by Index, Index called only after: ExtractDIEsIfNeeded(false)
GetUnitDIEPtrOnly expand+ro, only using m_die_array after: ExtractDIEsIfNeeded(true)
                             called by BuildAddressRangeTable, ParseProducerInfo, GetLanguageType, GetIsOptimized
DIEPtr            expand+ro, only using m_die_array after: ExtractDIEsIfNeeded(false)
                             called by BuildAddressRangeTable, GetFunctionAranges

[clayborg]

source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp

123 ↗

(On Diff #124296)

This isn't initialized and it is captured and used in already_parsed below?

123–137 ↗

(On Diff #124296)

I really can't tell what any of the above changed code is doing besides adding the mutex. Seems like you are trying to emulate a condition variable to halt other threads trying to get access to the DIEs. But m_die_array_finished isn't initialized in the constructor or inlined in the header file and initial_die_array_size isn't initialized so I can't really tell what this is supposed to actually do.

128 ↗

(On Diff #124296)

initial_die_array_size is used uninitialized when calling already_parsed()

131 ↗

(On Diff #124296)

initial_die_array_size is used uninitialized when calling already_parsed()

source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h

74 ↗

(On Diff #124296)

This isn't initialized anywhere.

[jankratochvil]

source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp

123 ↗

(On Diff #124296)

I am sorry but the missing initialization was just due to a wrong patch splitting. Next patch in the series did initialize it. Anyway it has been reworked now (after dropping whole ExtractDIEsIfNeededKind for its AllDiesButCuDieOnlyForPartialUnits).

123–137 ↗

(On Diff #124296)

Condition variable is not required here I think. m_die_array_finished is initialized before any time it gets read.

[jankratochvil]

It is now reworked without FileOffset and the remapping to unique DIE offsets for each DW_TAG_partial_unit inclusion by DW_TAG_imported_unit, as discussed in: https://lists.llvm.org/pipermail/lldb-commits/Week-of-Mon-20180409/040324.html

[clayborg]

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

96

Remove the "m_" prefix on this local variable as that indicates it is a member variable.

Might be better to use CleanUp utility from "lldb/Utility/CleanUp.h" instead of a shared pointer to void? Took me a bit to figure out what this was doing. If this is a very accepted C++ way to run a function that is scoped that is fine, but it seems a little unclear what this was doing initially.

[jankratochvil]

Removed the m_die_array_size_atomic_set m_ prefix, used CleanUp.

[jankratochvil]

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

96

Done, done. I have googled the shared_ptr cleanup for this purpose at StackOverflow. Updated, thanks.

[clayborg]

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

348–350

You can't really play with the m_die_array_size_atomic without locking the mutex, which really means m_die_array_size_atomic is not very useful. Seems like there is still a race condition:

• thread 1 calls this function and executes "m_die_array.swap(tmp_array);" but not " m_die_array_size_atomic = 0;"
• thread 2 calls DWARFUnit::ExtractDIEsIfNeeded() and calls "already_parsed()" which returns true because m_die_array_size_atomic is still some valid value
• thread 1 clears the DIE array and sets m_die_array_size_atomic to zero
• thread 2 has no DIEs when it tries to use them

[clayborg]

Much better.

[jankratochvil]

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

348–350

I see I did screw up even that, sorry for that. I believe there still could be an atomic_t speedup but given you are fine with the performance of a mutex I have implemented it that way.

[labath]

lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

317 ↗

(On Diff #144641)

You're accessing m_die_array without holding a lock.

[clayborg]

Good catch

[jankratochvil]

Revert "Protect DWARFCompileUnit::m_die_array by a new mutex"
Pavel Labath found this patch is incomplete and racy. I think there needs to be some more mutexes even before considering DW_TAG_partial_unit.
This reverts commit 331229 which was: https://reviews.llvm.org/D40470
GIT commit cc242f2a5e10a8e6f8e11a268d376a76324c1d23

[clayborg]

I think we should expose the llvm::sys::RWMutex and lets clients like BuildAddressRangeTable and SymbolFileDWARF::Index use it and get rid of m_die_array_usecount all together.

source/Plugins/SymbolFile/DWARF/DWARFUnit.h

181

Why don't we expose the llvm::sys::RWMutex for BuildAddressRangeTable and SymbolFileDWARF::Index? Then they can just grab the read lock?

[jankratochvil]

source/Plugins/SymbolFile/DWARF/DWARFUnit.h

181

Wouldn't you prefer an RAII lock (DWARFUnit::ScopedExtractDIEs) instead of the mutex exposure?

There are two mutexes (m_die_array_mutex and m_die_array_scoped_mutex), I tried it using a single mutex (as you IMO suggest) but then one needs to downgrade exclusive->shared lock there which requires a retry label there so I did not like the single lock (mutex2.patch).

[jankratochvil]

RAII lock DWARFUnit::ScopedExtractDIEs with mutexes m_die_array_mutex, m_die_array_scoped_mutex and m_first_die_mutex.

[jankratochvil]

Provided variants with a new RAII lock guard vs. unchanged caller bool clear_dies variable. And also 1 vs. 2 mutexes for m_die_array. Please choose one, I do not mind any.

• mutexes2raii.patch - RAII lock guard, 2 mutexes - preferred by me, posted here
• mutexes2var.patch - caller variable, 2 mutexes
• mutexes1raii.patch - RAII lock guard, 1 mutex (lock retrying)
• mutexes1var.patch - caller variable, 1 mutex (lock retrying) - IMO this was suggested by you

[jankratochvil]

FYI I have reordered m_cu->m_die_array_scoped_mutex.unlock_shared(); in DWARFUnit::ScopedExtractDIEs::~ScopedExtractDIEs() during commit (it was written this new way in some other of the 4 offered variants).

[jankratochvil]

Thanks for rL333996 fixups.

source/Plugins/SymbolFile/DWARF/DWARFUnit.h

 //===-- DWARFUnit.h ---------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef SymbolFileDWARF_DWARFUnit_h_
 #define SymbolFileDWARF_DWARFUnit_h_
 
 #include "DWARFDIE.h"
 #include "DWARFDebugInfoEntry.h"
 #include "lldb/lldb-enumerations.h"
+#include "llvm/Support/RWMutex.h"
 
 class DWARFUnit;
 class DWARFCompileUnit;
 class NameToDIE;
 class SymbolFileDWARF;
 class SymbolFileDWARFDwo;
 
 typedef std::shared_ptr<DWARFUnit> DWARFUnitSP;
protected:
   DWARFUnit(SymbolFileDWARF *dwarf);
 
   SymbolFileDWARF *m_dwarf = nullptr;
   std::unique_ptr<SymbolFileDWARFDwo> m_dwo_symbol_file;
   const DWARFAbbreviationDeclarationSet *m_abbrevs = nullptr;
   void *m_user_data = nullptr;
   // The compile unit debug information entry item
   DWARFDebugInfoEntry::collection m_die_array;
+  mutable llvm::sys::RWMutex m_die_array_mutex;
+  // It gets increased by ExtractDIEsIfNeeded() and decreased by ClearDIEs().
+  // BuildAddressRangeTable and SymbolFileDWARF::Index can populate m_die_array
+  // only temporarily while other functions calling only
+  // ExtractDIEsIfNeeded() will keep m_die_array populated forever.
+  uint32_t m_die_array_usecount = 0;
   // GetUnitDIEPtrOnly() needs to return pointer to the first DIE.
   // But the first element of m_die_array after ExtractUnitDIEIfNeeded()
   // would possibly move in memory after later ExtractDIEsIfNeeded().
   DWARFDebugInfoEntry m_first_die;
+  llvm::sys::RWMutex m_first_die_mutex;
   // A table similar to the .debug_aranges table, but this one points to the
   // exact DW_TAG_subprogram DIEs
   std::unique_ptr<DWARFDebugAranges> m_func_aranges_ap;
   dw_addr_t m_base_addr = 0;
   dw_offset_t m_length = 0;
   uint16_t m_version = 0;
   uint8_t m_addr_size = 0;
   DWARFProducer m_producer = eProducerInvalid;

 
 private:
   void ParseProducerInfo();
 
   // Get the DWARF unit DWARF debug informration entry. Parse the single DIE
   // if needed.
   const DWARFDebugInfoEntry *GetUnitDIEPtrOnly() {
     ExtractUnitDIEIfNeeded();
+    // m_first_die_mutex is not required as m_first_die is never cleared.
     if (!m_first_die)
       return NULL;
     return &m_first_die;
   }
 
   // Get all DWARF debug informration entries. Parse all DIEs if needed.
   const DWARFDebugInfoEntry *DIEPtr() {
     ExtractDIEsIfNeeded();
+    // m_die_array_mutex is not required as ExtractDIEsIfNeeded() has not
+    // been matched by ClearDIEs().
     if (m_die_array.empty())
       return NULL;
     return &m_die_array[0];
   }
 
   void AddUnitDIE(const DWARFDebugInfoEntry &cu_die);
   void ExtractDIEsEndCheck(lldb::offset_t offset) const;
 
   DISALLOW_COPY_AND_ASSIGN(DWARFUnit);
 };
 
 #endif // SymbolFileDWARF_DWARFUnit_h_

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

 DWARFUnit::DWARFUnit(SymbolFileDWARF *dwarf) : m_dwarf(dwarf) {}
 
 DWARFUnit::~DWARFUnit() {}
 
 //----------------------------------------------------------------------
 // Parses first DIE of a compile unit.
 //----------------------------------------------------------------------
 void DWARFUnit::ExtractUnitDIEIfNeeded() {
+  {
+    llvm::sys::ScopedReader lock(m_first_die_mutex);
+    if (m_first_die)
+      return; // Already parsed
+  }
+  llvm::sys::ScopedWriter lock(m_first_die_mutex);
   if (m_first_die)
     return; // Already parsed
 
   static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
   Timer scoped_timer(
       func_cat, "%8.8x: DWARFUnit::ExtractUnitDIEIfNeeded()", m_offset);
 
   // Set the offset to that of the first DIE and calculate the start of the
void DWARFUnit::ExtractUnitDIEIfNeeded() {
 
   ExtractDIEsEndCheck(offset);
 }
 
 //----------------------------------------------------------------------
 // Parses a compile unit and indexes its DIEs if it hasn't already been done.
 //----------------------------------------------------------------------
 bool DWARFUnit::ExtractDIEsIfNeeded() {
+  {
+    llvm::sys::ScopedReader lock(m_die_array_mutex);
+    if (!m_die_array.empty())
+      return 0; // Already parsed
+  }
+  llvm::sys::ScopedWriter lock(m_die_array_mutex);
   if (!m_die_array.empty())
     return 0; // Already parsed
+  // Protect against use count overflow.
+  if (m_die_array_usecount
+      < std::numeric_limits<decltype(m_die_array_usecount)>::max())
+    ++m_die_array_usecount;
+  llvm::sys::ScopedWriter first_die_lock(m_first_die_mutex);
 
   static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
   Timer scoped_timer(
       func_cat, "%8.8x: DWARFUnit::ExtractDIEsIfNeeded()", m_offset);
 
   // Set the offset to that of the first DIE and calculate the start of the
   // next compilation unit header.
   lldb::offset_t offset = GetFirstDIEOffset();
   lldb::offset_t next_cu_offset = GetNextCompileUnitOffset();
 
   DWARFDebugInfoEntry die;
   // Keep a flat array of the DIE for binary lookup by DIE offset
   Log *log(
       LogChannelDWARF::GetLogIfAny(DWARF_LOG_DEBUG_INFO | DWARF_LOG_LOOKUPS));
if (log && log->GetVerbose()) {
     if (m_die_array.empty())
       strm.Printf("error: no DIE for compile unit");
     else
       m_die_array[0].Dump(m_dwarf, this, strm, UINT32_MAX);
     log->PutString(strm.GetString());
   }
 }
 
+// m_die_array_mutex must be already held as read/write.
 void DWARFUnit::AddUnitDIE(const DWARFDebugInfoEntry &cu_die) {
   uint64_t base_addr = cu_die.GetAttributeValueAsAddress(
       m_dwarf, this, DW_AT_low_pc, LLDB_INVALID_ADDRESS);
   if (base_addr == LLDB_INVALID_ADDRESS)
     base_addr = cu_die.GetAttributeValueAsAddress(
         m_dwarf, this, DW_AT_entry_pc, 0);
   SetBaseAddress(base_addr);
 
DWARFDIE DWARFUnit::LookupAddress(const dw_addr_t address) {
   }
   return DWARFDIE();
 }
 
 size_t DWARFUnit::AppendDIEsWithTag(const dw_tag_t tag,
             DWARFDIECollection &dies,
             uint32_t depth) const {
   size_t old_size = dies.Size();
+  {
+    llvm::sys::ScopedReader lock(m_die_array_mutex);
     DWARFDebugInfoEntry::const_iterator pos;
     DWARFDebugInfoEntry::const_iterator end = m_die_array.end();
     for (pos = m_die_array.begin(); pos != end; ++pos) {
       if (pos->Tag() == tag)
         dies.Append(DWARFDIE(this, &(*pos)));
     }
+  }
 
   // Return the number of DIEs added to the collection
   return dies.Size() - old_size;
 }
 
 
 lldb::user_id_t DWARFUnit::GetID() const {
   dw_offset_t local_id =

 
 void DWARFUnit::SetAddrBase(dw_addr_t addr_base,
                             dw_addr_t ranges_base,
                             dw_offset_t base_obj_offset) {
   m_addr_base = addr_base;
   m_ranges_base = ranges_base;
   m_base_obj_offset = base_obj_offset;
 }
 
+// It may be called only after: ExtractDIEsIfNeeded() == true
 void DWARFUnit::ClearDIEs() {
+  {
+    llvm::sys::ScopedWriter lock(m_die_array_mutex);
+    lldbassert(!m_die_array.empty());
+    lldbassert(m_die_array_usecount > 0);
+    if (--m_die_array_usecount > 0)
+      return;
+
     m_die_array.clear();
     m_die_array.shrink_to_fit();
+  }
 
   if (m_dwo_symbol_file)
     m_dwo_symbol_file->GetCompileUnit()->ClearDIEs();
 }
 
 void DWARFUnit::BuildAddressRangeTable(SymbolFileDWARF *dwarf,
                                        DWARFDebugAranges *debug_aranges) {
   // This function is usually called if there in no .debug_aranges section in

 DWARFDIE
 DWARFUnit::GetDIE(dw_offset_t die_offset) {
   if (die_offset != DW_INVALID_OFFSET) {
     if (GetDwoSymbolFile())
       return GetDwoSymbolFile()->GetCompileUnit()->GetDIE(die_offset);
 
     if (ContainsDIEOffset(die_offset)) {
       ExtractDIEsIfNeeded();
+      // m_die_array_mutex is not required as ExtractDIEsIfNeeded() has
+      // not been matched by ClearDIEs().
       DWARFDebugInfoEntry::const_iterator end = m_die_array.cend();
       DWARFDebugInfoEntry::const_iterator pos =
           lower_bound(m_die_array.cbegin(), end, die_offset, CompareDIEOffset);
       if (pos != end) {
         if (die_offset == (*pos).GetOffset())
           return DWARFDIE(this, &(*pos));
       }
     } else {