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

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

123–137

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

initial_die_array_size is used uninitialized when calling already_parsed()

131

initial_die_array_size is used uninitialized when calling already_parsed()

source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h

74

This isn't initialized anywhere.

[jankratochvil]

source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp

123

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

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

56 ↗

(On Diff #144576)

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

56 ↗

(On Diff #144576)

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

[clayborg]

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

335 ↗

(On Diff #144589)

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

335 ↗

(On Diff #144589)

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 ↗

(On Diff #149037)

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 ↗

(On Diff #149037)

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/DWARFCompileUnit.h

public:
   lldb_private::LazyBool m_is_optimized = lldb_private::eLazyBoolCalculate;
   dw_addr_t m_addr_base = 0;     // Value of DW_AT_addr_base
   dw_addr_t m_ranges_base = 0;   // Value of DW_AT_ranges_base
   // If this is a dwo compile unit this is the offset of the base compile unit
   // in the main object file
   dw_offset_t m_base_obj_offset = DW_INVALID_OFFSET;
   dw_offset_t m_file_offset;
 
+  std::mutex m_extractdies_mutex;
+  bool m_die_array_finished;
+
 private:
   static uint8_t g_default_addr_size;
 
   DISALLOW_COPY_AND_ASSIGN(DWARFCompileUnitData);
 };
 
 class DWARFCompileUnit : public DWARFCompileUnitDecls {
 public:

source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp

if (debug_info.ValidOffset(*file_offset_ptr)) {
     *file_offset_ptr = m_data->m_file_offset;
   }
 
   return nullptr;
 }
 
 void DWARFCompileUnit::ClearDIEs(bool keep_compile_unit_die) {
   if (m_data->m_die_array.size() > 1) {
+    std::lock_guard<std::mutex> guard(m_data->m_extractdies_mutex);
+
     // std::vectors never get any smaller when resized to a smaller size,
     // or when clear() or erase() are called, the size will report that it
     // is smaller, but the memory allocated remains intact (call capacity()
     // to see this). So we need to create a temporary vector and swap the
     // contents which will cause just the internal pointers to be swapped
     // so that when "tmp_array" goes out of scope, it will destroy the
     // contents.
 

 
 //----------------------------------------------------------------------
 // ParseCompileUnitDIEsIfNeeded
 //
 // Parses a compile unit and indexes its DIEs if it hasn't already been
 // done.
 //----------------------------------------------------------------------
 size_t DWARFCompileUnit::ExtractDIEsIfNeeded(bool cu_die_only) {
-  const size_t initial_die_array_size = m_data->m_die_array.size();
-  if ((cu_die_only && initial_die_array_size > 0) || initial_die_array_size > 1)
-    return 0; // Already parsed
+  size_t initial_die_array_size;
+  auto already_parsed = [cu_die_only, &initial_die_array_size]() -> bool {
+    return (cu_die_only && initial_die_array_size > 0)
+        || initial_die_array_size > 1;
+  };
+  if (already_parsed() && m_data->m_die_array_finished)
+    return 0;
+  std::lock_guard<std::mutex> guard(m_data->m_extractdies_mutex);
+  if (already_parsed()) {
+    lldbassert(m_data->m_die_array_finished);
+    return 0;
+  }
+  m_data->m_die_array_finished = false;
+  std::shared_ptr<void> m_die_array_finished_set(nullptr,
+      [&](void*){ m_data->m_die_array_finished = true; });
 
   static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
   Timer scoped_timer(
       func_cat,
       "%8.8x: DWARFCompileUnit::ExtractDIEsIfNeeded( cu_die_only = %i )",
       m_offset, cu_die_only);
 
   // Set the offset to that of the first DIE and calculate the start of the