[Lldb-commits] [lldb] [lldb] Change Module to have a concrete UnwindTable, update (PR #101130)
Jason Molenda via lldb-commits
lldb-commits at lists.llvm.org
Tue Jul 30 15:25:21 PDT 2024
https://github.com/jasonmolenda updated https://github.com/llvm/llvm-project/pull/101130
>From 55b1ac1fbad89ebc50038738c1e09045e9884be8 Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Mon, 29 Jul 2024 22:37:43 -0700
Subject: [PATCH 1/3] [lldb] Change Module to have a concrete UnwindTable,
update
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
---
lldb/include/lldb/Core/Module.h | 6 +--
lldb/include/lldb/Symbol/UnwindTable.h | 2 +-
lldb/source/Core/Module.cpp | 26 +++++++------
lldb/source/Symbol/UnwindTable.cpp | 51 ++------------------------
4 files changed, 23 insertions(+), 62 deletions(-)
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..0e7d76b1b896c 100644
--- a/lldb/include/lldb/Symbol/UnwindTable.h
+++ b/lldb/include/lldb/Symbol/UnwindTable.h
@@ -64,7 +64,7 @@ class UnwindTable {
private:
void Dump(Stream &s);
- void Initialize();
+ void Initialize(bool force = false);
std::optional<AddressRange> GetAddressRange(const Address &addr,
const SymbolContext &sc);
diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index 9c105b3f0e57a..acc17ecad015b 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.Update();
} 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.Update();
}
}
}
@@ -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.Update();
} 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..3e5927a24659a 100644
--- a/lldb/source/Symbol/UnwindTable.cpp
+++ b/lldb/source/Symbol/UnwindTable.cpp
@@ -37,13 +37,13 @@ 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)
+void UnwindTable::Initialize(bool force) {
+ if (m_initialized && !force)
return;
std::lock_guard<std::mutex> guard(m_mutex);
- if (m_initialized) // check again once we've acquired the lock
+ if (m_initialized && !force) // check again once we've acquired the lock
return;
m_initialized = true;
ObjectFile *object_file = m_module.GetObjectFile();
@@ -84,50 +84,7 @@ void UnwindTable::Initialize() {
}
}
-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;
-
- if (!m_object_file_unwind_up)
- m_object_file_unwind_up = object_file->CreateCallFrameInfo();
-
- SectionList *sl = m_module.GetSectionList();
- if (!sl)
- return;
-
- SectionSP sect = sl->FindSectionByType(eSectionTypeEHFrame, true);
- 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) {
- m_debug_frame_up = std::make_unique<DWARFCallFrameInfo>(
- *object_file, sect, DWARFCallFrameInfo::DWARF);
- }
-
- sect = sl->FindSectionByType(eSectionTypeCompactUnwind, true);
- 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) {
- 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() { Initialize(true /*force*/); }
UnwindTable::~UnwindTable() = default;
>From 6b7e24331ba421ebc33e11ae7ef3413c4ef6573b Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Tue, 30 Jul 2024 01:23:27 -0700
Subject: [PATCH 2/3] Only recalculate the UnwindTable input sources if they
are unset.
---
lldb/source/Symbol/UnwindTable.cpp | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/lldb/source/Symbol/UnwindTable.cpp b/lldb/source/Symbol/UnwindTable.cpp
index 3e5927a24659a..70a9faecc0f8f 100644
--- a/lldb/source/Symbol/UnwindTable.cpp
+++ b/lldb/source/Symbol/UnwindTable.cpp
@@ -50,32 +50,30 @@ void UnwindTable::Initialize(bool force) {
if (!object_file)
return;
- m_object_file_unwind_up = object_file->CreateCallFrameInfo();
+ if (!m_object_file_unwind_up)
+ 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()) {
+ 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 (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 (sect) {
+ if (!m_compact_unwind_up && sect)
m_compact_unwind_up =
std::make_unique<CompactUnwindInfo>(*object_file, sect);
- }
sect = sl->FindSectionByType(eSectionTypeARMexidx, true);
- if (sect) {
+ if (!m_arm_unwind_up && sect) {
SectionSP sect_extab = sl->FindSectionByType(eSectionTypeARMextab, true);
if (sect_extab.get()) {
m_arm_unwind_up =
>From d00b1f15b644cff8f47fd5bfa359cf9c1e0adaaf Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Tue, 30 Jul 2024 15:23:45 -0700
Subject: [PATCH 3/3] Don't non-lazily parse unwind infos when SymbolFile added
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.
---
lldb/include/lldb/Symbol/UnwindTable.h | 8 ++++----
lldb/source/Core/Module.cpp | 6 +++---
lldb/source/Symbol/UnwindTable.cpp | 16 ++++++++++------
3 files changed, 17 insertions(+), 13 deletions(-)
diff --git a/lldb/include/lldb/Symbol/UnwindTable.h b/lldb/include/lldb/Symbol/UnwindTable.h
index 0e7d76b1b896c..bfcf1c3c072d6 100644
--- a/lldb/include/lldb/Symbol/UnwindTable.h
+++ b/lldb/include/lldb/Symbol/UnwindTable.h
@@ -57,14 +57,14 @@ 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);
- void Initialize(bool force = false);
+ void Initialize();
std::optional<AddressRange> GetAddressRange(const Address &addr,
const SymbolContext &sc);
diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index acc17ecad015b..f9d7832254f46 100644
--- a/lldb/source/Core/Module.cpp
+++ b/lldb/source/Core/Module.cpp
@@ -328,7 +328,7 @@ ObjectFile *Module::GetMemoryObjectFile(const lldb::ProcessSP &process_sp,
// we are unable to extract the os/environment from memory.
m_arch.MergeFrom(process_sp->GetTarget().GetArchitecture());
- m_unwind_table.Update();
+ m_unwind_table.ModuleWasUpdated();
} else {
error.SetErrorString("unable to find suitable object file plug-in");
}
@@ -1015,7 +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;
- m_unwind_table.Update();
+ m_unwind_table.ModuleWasUpdated();
}
}
}
@@ -1216,7 +1216,7 @@ ObjectFile *Module::GetObjectFile() {
// those values that overwrite unspecified unknown values.
m_arch.MergeFrom(m_objfile_sp->GetArchitecture());
- m_unwind_table.Update();
+ m_unwind_table.ModuleWasUpdated();
} else {
ReportError("failed to load objfile for {0}\nDebugging will be "
"degraded for this module.",
diff --git a/lldb/source/Symbol/UnwindTable.cpp b/lldb/source/Symbol/UnwindTable.cpp
index 70a9faecc0f8f..4dfbb953d0b8a 100644
--- a/lldb/source/Symbol/UnwindTable.cpp
+++ b/lldb/source/Symbol/UnwindTable.cpp
@@ -36,20 +36,21 @@ 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(bool force) {
- if (m_initialized && !force)
+void UnwindTable::Initialize() {
+ if (m_initialized)
return;
std::lock_guard<std::mutex> guard(m_mutex);
- if (m_initialized && !force) // check again once we've acquired the lock
+ 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_initialized = true;
+
if (!m_object_file_unwind_up)
m_object_file_unwind_up = object_file->CreateCallFrameInfo();
@@ -82,7 +83,10 @@ void UnwindTable::Initialize(bool force) {
}
}
-void UnwindTable::Update() { Initialize(true /*force*/); }
+void UnwindTable::ModuleWasUpdated() {
+ std::lock_guard<std::mutex> guard(m_mutex);
+ m_initialized = false;
+}
UnwindTable::~UnwindTable() = default;
More information about the lldb-commits
mailing list