[lldb] Remove UnwindPlan::Row shared_ptrs, lite #134081
Conversation
Replace with std::unique_ptr. Previous attempt at replacing them with values failed because x86AssemblyInspectionEngine was storing pointers to the rows while modifying the plan. Changing that is relatively easy, but I'm not sure if that's an improvement.
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesReplace with std::unique_ptr. Previous attempt at replacing them with values failed because x86AssemblyInspectionEngine was storing pointers to the rows while modifying the plan. Changing that is relatively easy, but I'm not sure if that's an improvement. Full diff: https://github.com/llvm/llvm-project/pull/134081.diff 2 Files Affected:
diff --git a/lldb/include/lldb/Symbol/UnwindPlan.h b/lldb/include/lldb/Symbol/UnwindPlan.h
index 9adda27b8f928..a0032d6e777c7 100644
--- a/lldb/include/lldb/Symbol/UnwindPlan.h
+++ b/lldb/include/lldb/Symbol/UnwindPlan.h
@@ -428,7 +428,7 @@ class UnwindPlan {
bool m_unspecified_registers_are_undefined = false;
}; // class Row
- typedef std::shared_ptr<Row> RowSP;
+ typedef std::unique_ptr<Row> RowUP;
UnwindPlan(lldb::RegisterKind reg_kind)
: m_register_kind(reg_kind), m_return_addr_register(LLDB_INVALID_REGNUM),
@@ -449,8 +449,8 @@ class UnwindPlan {
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));
+ for (const RowUP &row_up : rhs.m_row_list)
+ m_row_list.emplace_back(new Row(*row_up));
}
UnwindPlan(UnwindPlan &&rhs) = default;
UnwindPlan &operator=(const UnwindPlan &rhs) {
@@ -570,7 +570,7 @@ class UnwindPlan {
}
private:
- std::vector<RowSP> m_row_list;
+ std::vector<RowUP> 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 f2846eb927bf8..6cf96e66a7771 100644
--- a/lldb/source/Symbol/UnwindPlan.cpp
+++ b/lldb/source/Symbol/UnwindPlan.cpp
@@ -392,16 +392,16 @@ 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)));
+ m_row_list.push_back(std::make_unique<Row>(std::move(row)));
else
*m_row_list.back() = std::move(row);
}
struct RowLess {
- bool operator()(addr_t a, const UnwindPlan::RowSP &b) const {
+ bool operator()(addr_t a, const UnwindPlan::RowUP &b) const {
return a < b->GetOffset();
}
- bool operator()(const UnwindPlan::RowSP &a, addr_t b) const {
+ bool operator()(const UnwindPlan::RowUP &a, addr_t b) const {
return a->GetOffset() < b;
}
};
@@ -409,7 +409,7 @@ struct RowLess {
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)));
+ m_row_list.insert(it, std::make_unique<Row>(std::move(row)));
else {
assert(it->get()->GetOffset() == row.GetOffset());
if (replace_existing)
@@ -567,9 +567,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_up] : llvm::enumerate(m_row_list)) {
s.Format("row[{0}]: ", index);
- row_sp->Dump(s, this, thread, base_addr);
+ row_up->Dump(s, this, thread, base_addr);
s << "\n";
}
}
|
|
FTR, this is what it takes to fix the use after free problem. I didn't do that because it strictly increases the number of copies, and the use pattern is reasonable, but I'd be totally fine with doing that instead (I don't expect anyone will notice the extra copies). |
|
LGTM. I'm not sure if it's feasible, but perhaps x86AssemblyInspectionEngine could store an Index to the Row instead? Then you wouldn't need pointers or worry about their stability. UnwindPlan would just need to hand back some kind of row pointer/reference on demand and those should not be stored. |
Yeah, this definitely feels less satisfying. It also feels a bit path-dependent, as I don't think we'd fret over this if Rows were values from the beginning. So, I tried to make a small benchmark, and according to the results, copying a Row object with three register values (which is one more than what I'd expect for a typical first/last row of an unwind plan) takes about 50 nanoseconds. Copying a row with 100 registers (which is like more that most architectures have) takes ~three microseconds. With that in mind, maybe we do want to go ahead with d55a1ba ?
That doesn't quite work here because the code in question is inserting new rows into the middle of the unwind plan, so the index changes as well (for the last row, at least). I mean, I suppose you could try and keep the index up to date, but that's going to be very fragile. |
|
I see a 👍, so here's the value-semantics version of this. Please approve that PR if you prefer that approach. |
|
Abandoning in favor of #134821 |
Replace with std::unique_ptr. Previous attempt at replacing them with values failed because x86AssemblyInspectionEngine was storing pointers to the rows while modifying the plan. Changing that is relatively easy, but I'm not sure if that's an improvement.