[lldb] Remove UnwindPlan::Row shared_ptrs #132370
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The surrounding code doesn't use them anymore. This removes the internal usages. This patch makes the Rows actual values. An alternative would be to make them unique_ptrs. That would make vector resizes faster at the cost of more pointer chasing and heap fragmentation. I don't know which one is better so I picked the simpler option.
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesThe surrounding code doesn't use them anymore. This removes the internal usages. This patch makes the Rows actual values. An alternative would be to make them unique_ptrs. That would make vector resizes faster at the cost of more pointer chasing and heap fragmentation. I don't know which one is better so I picked the simpler option. Full diff: https://github.com/llvm/llvm-project/pull/132370.diff 2 Files Affected:
diff --git a/lldb/include/lldb/Symbol/UnwindPlan.h b/lldb/include/lldb/Symbol/UnwindPlan.h
index db9aade93b6ba..ddc54a9fdc8ae 100644
--- a/lldb/include/lldb/Symbol/UnwindPlan.h
+++ b/lldb/include/lldb/Symbol/UnwindPlan.h
@@ -428,8 +428,6 @@ class UnwindPlan {
bool m_unspecified_registers_are_undefined = false;
}; // class Row
- typedef std::shared_ptr<Row> RowSP;
-
UnwindPlan(lldb::RegisterKind reg_kind)
: m_register_kind(reg_kind), m_return_addr_register(LLDB_INVALID_REGNUM),
m_plan_is_sourced_from_compiler(eLazyBoolCalculate),
@@ -437,25 +435,10 @@ class UnwindPlan {
m_plan_is_for_signal_trap(eLazyBoolCalculate) {}
// Performs a deep copy of the plan, including all the rows (expensive).
- UnwindPlan(const UnwindPlan &rhs)
- : m_plan_valid_ranges(rhs.m_plan_valid_ranges),
- m_register_kind(rhs.m_register_kind),
- m_return_addr_register(rhs.m_return_addr_register),
- m_source_name(rhs.m_source_name),
- m_plan_is_sourced_from_compiler(rhs.m_plan_is_sourced_from_compiler),
- m_plan_is_valid_at_all_instruction_locations(
- rhs.m_plan_is_valid_at_all_instruction_locations),
- m_plan_is_for_signal_trap(rhs.m_plan_is_for_signal_trap),
- m_lsda_address(rhs.m_lsda_address),
- m_personality_func_addr(rhs.m_personality_func_addr) {
- m_row_list.reserve(rhs.m_row_list.size());
- for (const RowSP &row_sp : rhs.m_row_list)
- m_row_list.emplace_back(new Row(*row_sp));
- }
+ UnwindPlan(const UnwindPlan &rhs) = default;
+ UnwindPlan &operator=(const UnwindPlan &rhs) = default;
+
UnwindPlan(UnwindPlan &&rhs) = default;
- UnwindPlan &operator=(const UnwindPlan &rhs) {
- return *this = UnwindPlan(rhs); // NB: moving from a temporary (deep) copy
- }
UnwindPlan &operator=(UnwindPlan &&) = default;
~UnwindPlan() = default;
@@ -486,7 +469,7 @@ class UnwindPlan {
uint32_t GetInitialCFARegister() const {
if (m_row_list.empty())
return LLDB_INVALID_REGNUM;
- return m_row_list.front()->GetCFAValue().GetRegisterNumber();
+ return m_row_list.front().GetCFAValue().GetRegisterNumber();
}
// This UnwindPlan may not be valid at every address of the function span.
@@ -569,7 +552,7 @@ class UnwindPlan {
}
private:
- std::vector<RowSP> m_row_list;
+ std::vector<Row> m_row_list;
std::vector<AddressRange> m_plan_valid_ranges;
lldb::RegisterKind m_register_kind; // The RegisterKind these register numbers
// are in terms of - will need to be
diff --git a/lldb/source/Symbol/UnwindPlan.cpp b/lldb/source/Symbol/UnwindPlan.cpp
index 48089cbdecd97..92daf29630ab6 100644
--- a/lldb/source/Symbol/UnwindPlan.cpp
+++ b/lldb/source/Symbol/UnwindPlan.cpp
@@ -391,29 +391,29 @@ bool UnwindPlan::Row::operator==(const UnwindPlan::Row &rhs) const {
}
void UnwindPlan::AppendRow(Row row) {
- if (m_row_list.empty() || m_row_list.back()->GetOffset() != row.GetOffset())
- m_row_list.push_back(std::make_shared<Row>(std::move(row)));
+ if (m_row_list.empty() || m_row_list.back().GetOffset() != row.GetOffset())
+ m_row_list.push_back(std::move(row));
else
- *m_row_list.back() = std::move(row);
+ m_row_list.back() = std::move(row);
}
struct RowLess {
- bool operator()(addr_t a, const UnwindPlan::RowSP &b) const {
- return a < b->GetOffset();
+ bool operator()(addr_t a, const UnwindPlan::Row &b) const {
+ return a < b.GetOffset();
}
- bool operator()(const UnwindPlan::RowSP &a, addr_t b) const {
- return a->GetOffset() < b;
+ bool operator()(const UnwindPlan::Row &a, addr_t b) const {
+ return a.GetOffset() < b;
}
};
void UnwindPlan::InsertRow(Row row, bool replace_existing) {
auto it = llvm::lower_bound(m_row_list, row.GetOffset(), RowLess());
- if (it == m_row_list.end() || it->get()->GetOffset() > row.GetOffset())
- m_row_list.insert(it, std::make_shared<Row>(std::move(row)));
+ if (it == m_row_list.end() || it->GetOffset() > row.GetOffset())
+ m_row_list.insert(it, std::move(row));
else {
- assert(it->get()->GetOffset() == row.GetOffset());
+ assert(it->GetOffset() == row.GetOffset());
if (replace_existing)
- **it = std::move(row);
+ *it = std::move(row);
}
}
@@ -424,7 +424,7 @@ const UnwindPlan::Row *UnwindPlan::GetRowForFunctionOffset(int offset) const {
return nullptr;
// upper_bound returns the row strictly greater than our desired offset, which
// means that the row before it is a match.
- return std::prev(it)->get();
+ return &*std::prev(it);
}
bool UnwindPlan::IsValidRowIndex(uint32_t idx) const {
@@ -433,7 +433,7 @@ bool UnwindPlan::IsValidRowIndex(uint32_t idx) const {
const UnwindPlan::Row *UnwindPlan::GetRowAtIndex(uint32_t idx) const {
if (idx < m_row_list.size())
- return m_row_list[idx].get();
+ return &m_row_list[idx];
LLDB_LOG(GetLog(LLDBLog::Unwind),
"error: UnwindPlan::GetRowAtIndex(idx = {0}) invalid index "
"(number rows is {1})",
@@ -447,7 +447,7 @@ const UnwindPlan::Row *UnwindPlan::GetLastRow() const {
"UnwindPlan::GetLastRow() when rows are empty");
return nullptr;
}
- return m_row_list.back().get();
+ return &m_row_list.back();
}
bool UnwindPlan::PlanValidAtAddress(Address addr) {
@@ -566,9 +566,9 @@ void UnwindPlan::Dump(Stream &s, Thread *thread, lldb::addr_t base_addr) const {
range.Dump(&s, target_sp.get(), Address::DumpStyleSectionNameOffset);
s.EOL();
}
- for (const auto &[index, row_sp] : llvm::enumerate(m_row_list)) {
+ for (const auto &[index, row] : llvm::enumerate(m_row_list)) {
s.Format("row[{0}]: ", index);
- row_sp->Dump(s, this, thread, base_addr);
+ row.Dump(s, this, thread, base_addr);
s << "\n";
}
}
|
bulbazord
approved these changes
Mar 25, 2025
clayborg
approved these changes
Mar 25, 2025
How would you achieve that? By returning copies of the Row? That seems wasteful.. I don't think there's much risk in persisting row. There are many places that construct unwind plans, but there's basically just one user of it --
They are unstable, but only during construction. What I'm actually thinking is to guarantee that UnwindPlans are immutable after construction. That's very important since they are cached, accessed from multiple threads and whatnot, and it would also guarantee pointer stability.
They aren't, because I removed all the uses earlier :D |
JDevlieghere
approved these changes
Mar 26, 2025
|
Still double checking but i think this may have broken a couple of macOS x86_64 tests: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/lldb-cmake/10909/ |
rupprecht
added a commit
to rupprecht/llvm-project
that referenced
this pull request
Mar 27, 2025
This reverts commit d7cea2b. It causes crashes in API tests.
rupprecht
added a commit
that referenced
this pull request
Mar 27, 2025
|
Saw the same thing running tests internally; reverted. |
|
Thanks for reverting this while I was gone. It looks like there's still some pointer persistence. I'll take a look at that a bit later... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The surrounding code doesn't use them anymore. This removes the internal usages.
This patch makes the Rows actual values. An alternative would be to make them unique_ptrs. That would make vector resizes faster at the cost of more pointer chasing and heap fragmentation. I don't know which one is better so I picked the simpler option.