Skip to content

Reapply "[lldb] Remove UnwindPlan::Row shared_ptrs (https://github.com/llvm/llvm-project/pull/132370)" #134821

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 9, 2025
Merged

Reapply "[lldb] Remove UnwindPlan::Row shared_ptrs (https://github.com/llvm/llvm-project/pull/132370)" #134821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1de7588
WIP fix use-after-free
labath Apr 2, 2025
461706e
Reapply "[lldb] Remove UnwindPlan::Row shared_ptrs (#132370)"
labath Apr 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 5 additions & 22 deletions lldb/include/lldb/Symbol/UnwindPlan.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,34 +428,17 @@ 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),
m_plan_is_valid_at_all_instruction_locations(eLazyBoolCalculate),
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;
Expand Down Expand Up @@ -487,7 +470,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.
Expand Down Expand Up @@ -570,7 +553,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
Expand Down
18 changes: 9 additions & 9 deletions lldb/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1338,25 +1338,25 @@ bool x86AssemblyInspectionEngine::AugmentUnwindPlanFromCallSite(
if (unwind_plan.GetRowCount() < 2)
return false;

const UnwindPlan::Row *first_row = unwind_plan.GetRowAtIndex(0);
if (first_row->GetOffset() != 0)
UnwindPlan::Row first_row = *unwind_plan.GetRowAtIndex(0);
if (first_row.GetOffset() != 0)
return false;
uint32_t cfa_reg = first_row->GetCFAValue().GetRegisterNumber();
uint32_t cfa_reg = first_row.GetCFAValue().GetRegisterNumber();
if (unwind_plan.GetRegisterKind() != eRegisterKindLLDB) {
cfa_reg = reg_ctx->ConvertRegisterKindToRegisterNumber(
unwind_plan.GetRegisterKind(),
first_row->GetCFAValue().GetRegisterNumber());
first_row.GetCFAValue().GetRegisterNumber());
}
if (cfa_reg != m_lldb_sp_regnum ||
first_row->GetCFAValue().GetOffset() != m_wordsize)
first_row.GetCFAValue().GetOffset() != m_wordsize)
return false;

const UnwindPlan::Row *original_last_row = unwind_plan.GetLastRow();
UnwindPlan::Row original_last_row = *unwind_plan.GetLastRow();

size_t offset = 0;
int row_id = 1;
bool unwind_plan_updated = false;
UnwindPlan::Row row = *first_row;
UnwindPlan::Row row = first_row;

// After a mid-function epilogue we will need to re-insert the original
// unwind rules so unwinds work for the remainder of the function. These
Expand All @@ -1380,7 +1380,7 @@ bool x86AssemblyInspectionEngine::AugmentUnwindPlanFromCallSite(
continue;

if (reinstate_unwind_state) {
row = *original_last_row;
row = original_last_row;
row.SetOffset(offset);
unwind_plan.AppendRow(row);
reinstate_unwind_state = false;
Expand Down Expand Up @@ -1521,7 +1521,7 @@ bool x86AssemblyInspectionEngine::AugmentUnwindPlanFromCallSite(
if (ret_pattern_p()) {
row.SetOffset(offset);
row.GetCFAValue().SetIsRegisterPlusOffset(
first_row->GetCFAValue().GetRegisterNumber(), m_wordsize);
first_row.GetCFAValue().GetRegisterNumber(), m_wordsize);

unwind_plan.InsertRow(row);
unwind_plan_updated = true;
Expand Down
32 changes: 16 additions & 16 deletions lldb/source/Symbol/UnwindPlan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand All @@ -425,7 +425,7 @@ UnwindPlan::GetRowForFunctionOffset(std::optional<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 {
Expand All @@ -434,7 +434,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})",
Expand All @@ -448,7 +448,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) const {
Expand Down Expand Up @@ -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] : 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";
}
}
Expand Down