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

jasonmolenda · 2024-07-30T05:46:29Z

Currently a Module has a std::optional 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

Currently a Module has a std::optional<UnwindTable> 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://132514736

unset.

jimingham · 2024-07-30T20:04:26Z

lldb/include/lldb/Symbol/UnwindTable.h

@@ -64,7 +64,7 @@ class UnwindTable {
 private:
  void Dump(Stream &s);

-  void Initialize();
+  void Initialize(bool force = false);


This needs a comment explaining what you are forcing.

Yeah I'm not thrilled with the terminology I used. But I had Initialize which assumed no unwind sources had been discovered, and Update called after a SymbolFileVendor is added to the Module which looks for any un-set unwind sources and checks to see if they can now be found. That duplication didn't thrill me.

jimingham · 2024-07-30T20:50:57Z

So the strategy here is the UnwindTable class calls its Initialize() any time a client wants to use an UnwindTable so that it can be lazily filled. Then Module calls UnwindTable::Update() any time something gets added to the module that might change the unwind table.

That seems pretty clear.

It is a little odd that the laziness of the UnwindTable itself gets defeated by Update, which forces the Initialize even if the unwind table isn't currently being requested.

Not sure if that matters? If it does, Update could just tell the UnwindTable that next time it gets asked a question it has to reread the unwind information. Maybe it could even just set m_initialized back to false and then let the lazy initialization redo the work when requested?

jasonmolenda · 2024-07-30T20:55:39Z

It is a little odd that the laziness of the UnwindTable itself gets defeated by Update, which forces the Initialize even if the unwind table isn't currently being requested.

I could add an ivar m_module_has_updated which is set when a new ObjectFile/SymbolFileVendor is added to the Module. And all the getter methods in UnwindTable call Initialize() which currently checks that it has been initialized or not, and add that in.

Not sure if that matters? If it does, Update could just tell the UnwindTable that next time it gets asked a question it has to reread the unwind information. Maybe it could even just set m_initialized back to false and then let the lazy initialization redo the work when requested?

Hah, I didn't read your full comment. Yes, same idea, and just as good. I'll do that.

Rename UnwindTable::Update to UnwindTable::ModuleWasUpdated. Remove `force` parameter to `Initialize` method. Have `ModuleWasUpdated` unset m_initialized, so the next time the UnwindTable is asked for a source of information, we will then parse any newly available sources of unwind information.

llvmbot · 2024-07-30T22:25:53Z

@llvm/pr-subscribers-lldb

Author: Jason Molenda (jasonmolenda)

Changes

Currently a Module has a std::optional<UnwindTable> 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

Full diff: https://github.com/llvm/llvm-project/pull/101130.diff

4 Files Affected:

(modified) lldb/include/lldb/Core/Module.h (+3-3)
(modified) lldb/include/lldb/Symbol/UnwindTable.h (+3-3)
(modified) lldb/source/Core/Module.cpp (+15-11)
(modified) lldb/source/Symbol/UnwindTable.cpp (+10-51)

diff --git a/lldb/include/lldb/Core/Module.h b/lldb/include/lldb/Core/Module.h
index 0188057247a68..5589c1c9a350d 100644
--- 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
index 26826e5d1b497..bfcf1c3c072d6 100644
--- 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);
diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index 9c105b3f0e57a..f9d7832254f46 100644
--- 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
index 11bedf3d6052e..4dfbb953d0b8a 100644
--- a/lldb/source/Symbol/UnwindTable.cpp
+++ b/lldb/source/Symbol/UnwindTable.cpp
@@ -36,7 +36,6 @@ UnwindTable::UnwindTable(Module &module)
 
 // 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)
     return;
@@ -45,55 +44,13 @@ void UnwindTable::Initialize() {
 
   if (m_initialized) // check again once we've acquired the lock
     return;
-  m_initialized = true;
-  ObjectFile *object_file = m_module.GetObjectFile();
-  if (!object_file)
-    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_initialized = 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_initialized = false;
+}
+
 UnwindTable::~UnwindTable() = default;
 
 std::optional<AddressRange>

jasonmolenda · 2024-07-31T00:09:44Z

Thanks for the suggestion @jimingham , I pushed an update to implement that idea.

JDevlieghere

This makes sense to me. LGTM if Jim is happy.

jimingham

LGTM I like that you also changed "Update" to "ModuleWasUpdated", that's more accurate. m_initialized isn't quite the right name, it means "needs work" not "initialized" which sounds like it should happen once.
If you can think of a better name for that off-hand, that would be a little clearer, but I can't think of a good one, and it's not crucial.

in UnwindTable class, to make it clearer what it is tracking.

) Currently a Module has a std::optional<UnwindTable> 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 (cherry picked from commit 7ad073a)

…s-has-an-unwindtable [lldb] Change Module to have a concrete UnwindTable, update (llvm#101130)

…-unwindtable-from-module [lldb] Change Module to have a concrete UnwindTable, update (llvm#101130)

jasonmolenda requested review from adrian-prantl and jimingham July 30, 2024 05:46

jasonmolenda requested a review from JDevlieghere as a code owner July 30, 2024 05:46

Only recalculate the UnwindTable input sources if they are

6b7e243

unset.

jimingham reviewed Jul 30, 2024

View reviewed changes

llvmbot added the lldb label Jul 30, 2024

JDevlieghere approved these changes Jul 31, 2024

View reviewed changes

jimingham approved these changes Jul 31, 2024

View reviewed changes

rename m_initialized to m_scanned_all_unwind_sources

a9d949d

in UnwindTable class, to make it clearer what it is tracking.

jasonmolenda merged commit 7ad073a into llvm:main Aug 2, 2024
4 of 5 checks passed

jasonmolenda deleted the change-module-to-have-concrete-unwindtable branch August 2, 2024 00:43

jasonmolenda added a commit to swiftlang/llvm-project that referenced this pull request Aug 27, 2024

Merge pull request #9186 from jasonmolenda/cp/r128876433-module-alway…

dd86335

…s-has-an-unwindtable [lldb] Change Module to have a concrete UnwindTable, update (llvm#101130)

JDevlieghere added a commit to swiftlang/llvm-project that referenced this pull request Aug 27, 2024

Merge pull request #9121 from jasonmolenda/cp/r128876433-dont-release…

6f66b44

…-unwindtable-from-module [lldb] Change Module to have a concrete UnwindTable, update (llvm#101130)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

jasonmolenda commented Jul 30, 2024

Uh oh!

jimingham Jul 30, 2024

Uh oh!

jasonmolenda Jul 30, 2024

Uh oh!

jimingham commented Jul 30, 2024

Uh oh!

jasonmolenda commented Jul 30, 2024

Uh oh!

llvmbot commented Jul 30, 2024

Uh oh!

jasonmolenda commented Jul 31, 2024

Uh oh!

JDevlieghere left a comment

Uh oh!

jimingham left a comment

Uh oh!

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

jasonmolenda commented Jul 30, 2024

Uh oh!

jimingham Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

jasonmolenda Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

jimingham commented Jul 30, 2024

Uh oh!

jasonmolenda commented Jul 30, 2024

Uh oh!

llvmbot commented Jul 30, 2024

Uh oh!

jasonmolenda commented Jul 31, 2024

Uh oh!

JDevlieghere left a comment

Choose a reason for hiding this comment

Uh oh!

jimingham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!