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

56

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

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

[clayborg]

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

334

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

334

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

150

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

150

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:
   void *m_user_data = nullptr;
   // The compile unit debug information entry item
   DWARFDebugInfoEntry::collection m_die_array;
   // 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;
   uint32_t m_producer_version_major = 0;
   uint32_t m_producer_version_minor = 0;
   uint32_t m_producer_version_update = 0;
   lldb::LanguageType m_language_type = lldb::eLanguageTypeUnknown;
   bool m_is_dwarf64 = false;
   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;
+  llvm::sys::RWMutex m_extractdies_mutex;
 
   static void
   IndexPrivate(DWARFUnit *dwarf_cu, const lldb::LanguageType cu_language,
                const DWARFFormValue::FixedFormSizes &fixed_form_sizes,
                const dw_offset_t cu_offset, NameToDIE &func_basenames,
                NameToDIE &func_fullnames, NameToDIE &func_methods,
                NameToDIE &func_selectors, NameToDIE &objc_class_selectors,
                NameToDIE &globals, NameToDIE &types, NameToDIE &namespaces);

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

 
 //----------------------------------------------------------------------
 // ParseCompileUnitDIEsIfNeeded
 //
 // Parses a compile unit and indexes its DIEs if it hasn't already been
 // done.
 //----------------------------------------------------------------------
 size_t DWARFUnit::ExtractDIEsIfNeeded(bool cu_die_only) {
-  const size_t initial_die_array_size = 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, this]() -> bool {
+    initial_die_array_size = m_die_array.size();
+    return (cu_die_only && initial_die_array_size > 0)
+        || initial_die_array_size > 1;
+  };
+  {
+    llvm::sys::ScopedReader lock(m_extractdies_mutex);
+    if (already_parsed())
+      return 0;
+  }
+  llvm::sys::ScopedWriter lock(m_extractdies_mutex);
+  if (already_parsed())
+    return 0;
 
   static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
   Timer scoped_timer(
       func_cat, "%8.8x: DWARFUnit::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
   // next compilation unit header.
void DWARFUnit::SetAddrBase(dw_addr_t addr_base,
                             dw_offset_t base_obj_offset) {
   m_addr_base = addr_base;
   m_ranges_base = ranges_base;
   m_base_obj_offset = base_obj_offset;
 }
 
 void DWARFUnit::ClearDIEs(bool keep_compile_unit_die) {
   if (m_die_array.size() > 1) {
+    llvm::sys::ScopedWriter lock(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.
 
     // Save at least the compile unit DIE
     DWARFDebugInfoEntry::collection tmp_array;
     m_die_array.swap(tmp_array);
     if (keep_compile_unit_die)
       m_die_array.push_back(tmp_array.front());
   }
 
   if (m_dwo_symbol_file)
     m_dwo_symbol_file->GetCompileUnit()->ClearDIEs(keep_compile_unit_die);
 }
 
 void DWARFUnit::BuildAddressRangeTable(SymbolFileDWARF *dwarf,