[lldb] Change Module to have a concrete UnwindTable, update (llvm#101130)

jasonmolenda · web-flow · commit 7ad073a45b41 · 2024-08-01T17:43:25.000-07:00
Currently a Module has a std::optional&lt;UnwindTable&gt; which is created
when the UnwindTable is requested from outside the Module. The idea is
to delay its creation until the Module has an ObjectFile initialized,
which will have been done by the time we're doing an unwind.

However, Module::GetUnwindTable wasn't doing any locking, so it was
possible for two threads to ask for the UnwindTable for the first time,
one would be created and returned while another thread would create one,
destroy the first in the process of emplacing it. It was an uncommon
crash, but it was possible.

Grabbing the Module's mutex would be one way to address it, but when
loading ELF binaries, we start creating the SymbolTable on one thread
(ObjectFileELF) grabbing the Module's mutex, and then spin up worker
threads to parse the individual DWARF compilation units, which then try
to also get the UnwindTable and deadlock if they try to get the Module's
mutex.

This changes Module to have a concrete UnwindTable as an ivar, and when
it adds an ObjectFile or SymbolFileVendor, it will call the Update
method on it, which will re-evaluate which sections exist in the
ObjectFile/SymbolFile. UnwindTable used to have an Initialize method
which set all the sections, and an Update method which would set some of
them if they weren't set. I unified these with the Initialize method
taking a `force` option to re-initialize the section pointers even if
they had been done already before.

This is addressing a rare crash report we've received, and also a
failure Adrian spotted on the -fsanitize=address CI bot last week, it's
still uncommon with ASAN but it can happen with the standard testsuite.

rdar://128876433
diff --git a/lldb/include/lldb/Core/Module.h b/lldb/include/lldb/Core/Module.h
@@ -1021,9 +1021,9 @@ class Module : public std::enable_shared_from_this<Module>,
   lldb::ObjectFileSP m_objfile_sp; ///< A shared pointer to the object file
                                    /// parser for this module as it may or may
                                    /// not be shared with the SymbolFile
-  std::optional<UnwindTable> m_unwind_table; ///< Table of FuncUnwinders
-                                             /// objects created for this
-                                             /// Module's functions
+  UnwindTable m_unwind_table;      ///< Table of FuncUnwinders
+                                   /// objects created for this
+                                   /// Module's functions
   lldb::SymbolVendorUP
       m_symfile_up; ///< A pointer to the symbol vendor for this module.
   std::vector<lldb::SymbolVendorUP>
diff --git a/lldb/include/lldb/Symbol/UnwindTable.h b/lldb/include/lldb/Symbol/UnwindTable.h
@@ -57,9 +57,9 @@ class UnwindTable {
 
   ArchSpec GetArchitecture();
 
-  /// Called after a SymbolFile has been added to a Module to add any new
-  /// unwind sections that may now be available.
-  void Update();
+  /// Called after an ObjectFile/SymbolFile has been added to a Module to add
+  /// any new unwind sections that may now be available.
+  void ModuleWasUpdated();
 
 private:
   void Dump(Stream &s);
@@ -75,7 +75,11 @@ class UnwindTable {
   Module &m_module;
   collection m_unwinds;
 
-  bool m_initialized; // delay some initialization until ObjectFile is set up
+  bool m_scanned_all_unwind_sources; // true when we have looked at the
+                                     // ObjectFile and SymbolFile for all
+                                     // sources of unwind information; false if
+                                     // we haven't done that yet, or one of the
+                                     // files has been updated in the Module.
   std::mutex m_mutex;
 
   std::unique_ptr<CallFrameInfo> m_object_file_unwind_up;
diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
@@ -131,7 +131,8 @@ Module *Module::GetAllocatedModuleAtIndex(size_t idx) {
 }
 
 Module::Module(const ModuleSpec &module_spec)
-    : m_file_has_changed(false), m_first_file_changed_log(false) {
+    : m_unwind_table(*this), m_file_has_changed(false),
+      m_first_file_changed_log(false) {
   // Scope for locker below...
   {
     std::lock_guard<std::recursive_mutex> guard(
@@ -238,7 +239,8 @@ Module::Module(const FileSpec &file_spec, const ArchSpec &arch,
     : m_mod_time(FileSystem::Instance().GetModificationTime(file_spec)),
       m_arch(arch), m_file(file_spec), m_object_name(object_name),
       m_object_offset(object_offset), m_object_mod_time(object_mod_time),
-      m_file_has_changed(false), m_first_file_changed_log(false) {
+      m_unwind_table(*this), m_file_has_changed(false),
+      m_first_file_changed_log(false) {
   // Scope for locker below...
   {
     std::lock_guard<std::recursive_mutex> guard(
@@ -254,7 +256,9 @@ Module::Module(const FileSpec &file_spec, const ArchSpec &arch,
               m_object_name.AsCString(""), m_object_name.IsEmpty() ? "" : ")");
 }
 
-Module::Module() : m_file_has_changed(false), m_first_file_changed_log(false) {
+Module::Module()
+    : m_unwind_table(*this), m_file_has_changed(false),
+      m_first_file_changed_log(false) {
   std::lock_guard<std::recursive_mutex> guard(
       GetAllocationModuleCollectionMutex());
   GetModuleCollection().push_back(this);
@@ -323,6 +327,8 @@ ObjectFile *Module::GetMemoryObjectFile(const lldb::ProcessSP &process_sp,
           // Augment the arch with the target's information in case
           // we are unable to extract the os/environment from memory.
           m_arch.MergeFrom(process_sp->GetTarget().GetArchitecture());
+
+          m_unwind_table.ModuleWasUpdated();
         } else {
           error.SetErrorString("unable to find suitable object file plug-in");
         }
@@ -1009,8 +1015,7 @@ SymbolFile *Module::GetSymbolFile(bool can_create, Stream *feedback_strm) {
         m_symfile_up.reset(
             SymbolVendor::FindPlugin(shared_from_this(), feedback_strm));
         m_did_load_symfile = true;
-        if (m_unwind_table)
-          m_unwind_table->Update();
+        m_unwind_table.ModuleWasUpdated();
       }
     }
   }
@@ -1210,6 +1215,8 @@ ObjectFile *Module::GetObjectFile() {
           // more specific than the generic COFF architecture, only merge in
           // those values that overwrite unspecified unknown values.
           m_arch.MergeFrom(m_objfile_sp->GetArchitecture());
+
+          m_unwind_table.ModuleWasUpdated();
         } else {
           ReportError("failed to load objfile for {0}\nDebugging will be "
                       "degraded for this module.",
@@ -1240,12 +1247,9 @@ void Module::SectionFileAddressesChanged() {
 }
 
 UnwindTable &Module::GetUnwindTable() {
-  if (!m_unwind_table) {
-    if (!m_symfile_spec)
-      SymbolLocator::DownloadSymbolFileAsync(GetUUID());
-    m_unwind_table.emplace(*this);
-  }
-  return *m_unwind_table;
+  if (!m_symfile_spec)
+    SymbolLocator::DownloadSymbolFileAsync(GetUUID());
+  return m_unwind_table;
 }
 
 SectionList *Module::GetUnifiedSectionList() {
diff --git a/lldb/source/Symbol/UnwindTable.cpp b/lldb/source/Symbol/UnwindTable.cpp
@@ -30,70 +30,27 @@ using namespace lldb;
 using namespace lldb_private;
 
 UnwindTable::UnwindTable(Module &module)
-    : m_module(module), m_unwinds(), m_initialized(false), m_mutex(),
-      m_object_file_unwind_up(), m_eh_frame_up(), m_compact_unwind_up(),
-      m_arm_unwind_up() {}
+    : m_module(module), m_unwinds(), m_scanned_all_unwind_sources(false),
+      m_mutex(), m_object_file_unwind_up(), m_eh_frame_up(),
+      m_compact_unwind_up(), m_arm_unwind_up() {}
 
 // We can't do some of this initialization when the ObjectFile is running its
 // ctor; delay doing it until needed for something.
-
 void UnwindTable::Initialize() {
-  if (m_initialized)
+  if (m_scanned_all_unwind_sources)
     return;
 
   std::lock_guard<std::mutex> guard(m_mutex);
 
-  if (m_initialized) // check again once we've acquired the lock
-    return;
-  m_initialized = true;
-  ObjectFile *object_file = m_module.GetObjectFile();
-  if (!object_file)
+  if (m_scanned_all_unwind_sources) // check again once we've acquired the lock
     return;
 
-  m_object_file_unwind_up = object_file->CreateCallFrameInfo();
-
-  SectionList *sl = m_module.GetSectionList();
-  if (!sl)
-    return;
-
-  SectionSP sect = sl->FindSectionByType(eSectionTypeEHFrame, true);
-  if (sect.get()) {
-    m_eh_frame_up = std::make_unique<DWARFCallFrameInfo>(
-        *object_file, sect, DWARFCallFrameInfo::EH);
-  }
-
-  sect = sl->FindSectionByType(eSectionTypeDWARFDebugFrame, true);
-  if (sect) {
-    m_debug_frame_up = std::make_unique<DWARFCallFrameInfo>(
-        *object_file, sect, DWARFCallFrameInfo::DWARF);
-  }
-
-  sect = sl->FindSectionByType(eSectionTypeCompactUnwind, true);
-  if (sect) {
-    m_compact_unwind_up =
-        std::make_unique<CompactUnwindInfo>(*object_file, sect);
-  }
-
-  sect = sl->FindSectionByType(eSectionTypeARMexidx, true);
-  if (sect) {
-    SectionSP sect_extab = sl->FindSectionByType(eSectionTypeARMextab, true);
-    if (sect_extab.get()) {
-      m_arm_unwind_up =
-          std::make_unique<ArmUnwindInfo>(*object_file, sect, sect_extab);
-    }
-  }
-}
-
-void UnwindTable::Update() {
-  if (!m_initialized)
-    return Initialize();
-
-  std::lock_guard<std::mutex> guard(m_mutex);
-
   ObjectFile *object_file = m_module.GetObjectFile();
   if (!object_file)
     return;
 
+  m_scanned_all_unwind_sources = true;
+
   if (!m_object_file_unwind_up)
     m_object_file_unwind_up = object_file->CreateCallFrameInfo();
 
@@ -102,22 +59,19 @@ void UnwindTable::Update() {
     return;
 
   SectionSP sect = sl->FindSectionByType(eSectionTypeEHFrame, true);
-  if (!m_eh_frame_up && sect) {
+  if (!m_eh_frame_up && sect)
     m_eh_frame_up = std::make_unique<DWARFCallFrameInfo>(
         *object_file, sect, DWARFCallFrameInfo::EH);
-  }
 
   sect = sl->FindSectionByType(eSectionTypeDWARFDebugFrame, true);
-  if (!m_debug_frame_up && sect) {
+  if (!m_debug_frame_up && sect)
     m_debug_frame_up = std::make_unique<DWARFCallFrameInfo>(
         *object_file, sect, DWARFCallFrameInfo::DWARF);
-  }
 
   sect = sl->FindSectionByType(eSectionTypeCompactUnwind, true);
-  if (!m_compact_unwind_up && sect) {
+  if (!m_compact_unwind_up && sect)
     m_compact_unwind_up =
         std::make_unique<CompactUnwindInfo>(*object_file, sect);
-  }
 
   sect = sl->FindSectionByType(eSectionTypeARMexidx, true);
   if (!m_arm_unwind_up && sect) {
@@ -129,6 +83,11 @@ void UnwindTable::Update() {
   }
 }
 
+void UnwindTable::ModuleWasUpdated() {
+  std::lock_guard<std::mutex> guard(m_mutex);
+  m_scanned_all_unwind_sources = false;
+}
+
 UnwindTable::~UnwindTable() = default;
 
 std::optional<AddressRange>
