https://github.com/labath created
https://github.com/llvm/llvm-project/pull/129030
My main motivation was trying to understand how the function and whether the
rows need to be (shared) pointers. I noticed that the function essentially
constructs two copies unwind plans in parallel (the second being the
saved_unwind_states).
If we delay the construction of the unwind plan to the end of the function,
then we never need two copies of a single row (we can just move it into the
final result), so we can just use them as value types. This makes the overall
logic of the function stand out better as it avoids the laborious deep copies
of the Row shared pointer.
I've also noticed that a large portion of the function is devoted to
recomputing certain properties of the unwind state (e.g. the `m_fp_is_cfa`
field). Instead of doing that, this patch just saves/restores them together
with the rest of the state.
>From 95add874e5027b9e117eb0b62326fc7a1b46e432 Mon Sep 17 00:00:00 2001
From: Pavel Labath <pa...@labath.sk>
Date: Thu, 27 Feb 2025 10:58:08 +0100
Subject: [PATCH] [lldb] Clean up UnwindAssemblyInstEmulation
My main motivation was trying to understand how the function and whether
the rows need to be (shared) pointers. I noticed that the function
essentially constructs two copies unwind plans in parallel (the second
being the saved_unwind_states).
If we delay the construction of the unwind plan to the end of the
function, then we never need two copies of a single row (we can just
move it into the final result), so we can just use them as value types.
This makes the overall logic of the function stand out better as it
avoids the laborious deep copies of the Row shared pointer.
I've also noticed that a large portion of the function is devoted to
recomputing certain properties of the unwind state (e.g. the
`m_fp_is_cfa` field). Instead of doing that, this patch just
saves/restores them together with the rest of the state.
---
.../UnwindAssemblyInstEmulation.cpp | 188 ++++++------------
.../UnwindAssemblyInstEmulation.h | 20 +-
2 files changed, 76 insertions(+), 132 deletions(-)
diff --git
a/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
b/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
index fe1c7256e22bc..0a206fbee4678 100644
---
a/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
+++
b/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
@@ -86,10 +86,12 @@ bool
UnwindAssemblyInstEmulation::GetNonCallSiteUnwindPlanFromAssembly(
const bool show_address = true;
const bool show_bytes = true;
const bool show_control_flow_kind = false;
- m_cfa_reg_info = *m_inst_emulator_up->GetRegisterInfo(
+
+ m_state.cfa_reg_info = *m_inst_emulator_up->GetRegisterInfo(
unwind_plan.GetRegisterKind(), unwind_plan.GetInitialCFARegister());
- m_fp_is_cfa = false;
- m_register_values.clear();
+ m_state.fp_is_cfa = false;
+ m_state.register_values.clear();
+
m_pushed_regs.clear();
// Initialize the CFA with a known value. In the 32 bit case it will be
@@ -97,8 +99,8 @@ bool
UnwindAssemblyInstEmulation::GetNonCallSiteUnwindPlanFromAssembly(
// byte size to be safe for any future address sizes
m_initial_sp = (1ull << ((addr_byte_size * 8) - 1));
RegisterValue cfa_reg_value;
- cfa_reg_value.SetUInt(m_initial_sp, m_cfa_reg_info.byte_size);
- SetRegisterValue(m_cfa_reg_info, cfa_reg_value);
+ cfa_reg_value.SetUInt(m_initial_sp, m_state.cfa_reg_info.byte_size);
+ SetRegisterValue(m_state.cfa_reg_info, cfa_reg_value);
const InstructionList &inst_list = disasm_sp->GetInstructionList();
const size_t num_instructions = inst_list.GetSize();
@@ -107,33 +109,25 @@ bool
UnwindAssemblyInstEmulation::GetNonCallSiteUnwindPlanFromAssembly(
Instruction *inst = inst_list.GetInstructionAtIndex(0).get();
const lldb::addr_t base_addr = inst->GetAddress().GetFileAddress();
- // Map for storing the unwind plan row and the value of the registers at a
- // given offset. When we see a forward branch we add a new entry to this
map
- // with the actual unwind plan row and register context for the target
- // address of the branch as the current data have to be valid for the
target
- // address of the branch too if we are in the same function.
- std::map<lldb::addr_t, std::pair<UnwindPlan::RowSP, RegisterValueMap>>
- saved_unwind_states;
+ // Map for storing the unwind state at a given offset. When we see a
forward
+ // branch we add a new entry to this map with the actual unwind plan row
and
+ // register context for the target address of the branch as the current
data
+ // have to be valid for the target address of the branch too if we are in
+ // the same function.
+ std::map<lldb::addr_t, UnwindState> saved_unwind_states;
- // Make a copy of the current instruction Row and save it in m_curr_row so
+ // Make a copy of the current instruction Row and save it in m_state so
// we can add updates as we process the instructions.
- UnwindPlan::RowSP last_row =
- std::make_shared<UnwindPlan::Row>(*unwind_plan.GetLastRow());
- m_curr_row = std::make_shared<UnwindPlan::Row>(*last_row);
+ m_state.row = *unwind_plan.GetLastRow();
// Add the initial state to the save list with offset 0.
- saved_unwind_states.insert({0, {last_row, m_register_values}});
-
- // cache the stack pointer register number (in whatever register numbering
- // this UnwindPlan uses) for quick reference during instruction parsing.
- RegisterInfo sp_reg_info = *m_inst_emulator_up->GetRegisterInfo(
- eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP);
+ auto condition_block_start_state =
+ saved_unwind_states.emplace(0, m_state).first;
// The architecture dependent condition code of the last processed
// instruction.
EmulateInstruction::InstructionCondition last_condition =
EmulateInstruction::UnconditionalCondition;
- lldb::addr_t condition_block_start_offset = 0;
for (size_t idx = 0; idx < num_instructions; ++idx) {
m_curr_row_modified = false;
@@ -151,78 +145,28 @@ bool
UnwindAssemblyInstEmulation::GetNonCallSiteUnwindPlanFromAssembly(
--it; // Move it to the row corresponding to the current offset
// If the offset of m_curr_row don't match with the offset we see in
- // saved_unwind_states then we have to update m_curr_row and
- // m_register_values based on the saved values. It is happening after we
- // processed an epilogue and a return to caller instruction.
- if (it->second.first->GetOffset() != m_curr_row->GetOffset()) {
- UnwindPlan::Row *newrow = new UnwindPlan::Row;
- *newrow = *it->second.first;
- m_curr_row.reset(newrow);
- m_register_values = it->second.second;
- // re-set the CFA register ivars to match the new m_curr_row.
- if (sp_reg_info.name &&
- m_curr_row->GetCFAValue().IsRegisterPlusOffset()) {
- uint32_t row_cfa_regnum =
- m_curr_row->GetCFAValue().GetRegisterNumber();
- lldb::RegisterKind row_kind = m_unwind_plan_ptr->GetRegisterKind();
- // set m_cfa_reg_info to the row's CFA reg.
- m_cfa_reg_info =
- *m_inst_emulator_up->GetRegisterInfo(row_kind, row_cfa_regnum);
- // set m_fp_is_cfa.
- if (sp_reg_info.kinds[row_kind] == row_cfa_regnum)
- m_fp_is_cfa = false;
- else
- m_fp_is_cfa = true;
- }
- }
+ // saved_unwind_states then we have to update current unwind state to
+ // the saved values. It is happening after we processed an epilogue and a
+ // return to caller instruction.
+ if (it->second.row.GetOffset() != m_state.row.GetOffset())
+ m_state = it->second;
m_inst_emulator_up->SetInstruction(inst->GetOpcode(), inst->GetAddress(),
nullptr);
if (last_condition != m_inst_emulator_up->GetInstructionCondition()) {
- if (m_inst_emulator_up->GetInstructionCondition() !=
- EmulateInstruction::UnconditionalCondition &&
- saved_unwind_states.count(current_offset) == 0) {
- // If we don't have a saved row for the current offset then save our
- // current state because we will have to restore it after the
- // conditional block.
- auto new_row = std::make_shared<UnwindPlan::Row>(*m_curr_row.get());
- saved_unwind_states.insert(
- {current_offset, {new_row, m_register_values}});
- }
-
// If the last instruction was conditional with a different condition
- // then the then current condition then restore the condition.
+ // than the current condition then restore the state.
if (last_condition != EmulateInstruction::UnconditionalCondition) {
- const auto &saved_state =
- saved_unwind_states.at(condition_block_start_offset);
- m_curr_row = std::make_shared<UnwindPlan::Row>(*saved_state.first);
- m_curr_row->SetOffset(current_offset);
- m_register_values = saved_state.second;
- // re-set the CFA register ivars to match the new m_curr_row.
- if (sp_reg_info.name &&
- m_curr_row->GetCFAValue().IsRegisterPlusOffset()) {
- uint32_t row_cfa_regnum =
- m_curr_row->GetCFAValue().GetRegisterNumber();
- lldb::RegisterKind row_kind = m_unwind_plan_ptr->GetRegisterKind();
- // set m_cfa_reg_info to the row's CFA reg.
- m_cfa_reg_info =
- *m_inst_emulator_up->GetRegisterInfo(row_kind, row_cfa_regnum);
- // set m_fp_is_cfa.
- if (sp_reg_info.kinds[row_kind] == row_cfa_regnum)
- m_fp_is_cfa = false;
- else
- m_fp_is_cfa = true;
- }
+ m_state = condition_block_start_state->second;
+ m_state.row.SetOffset(current_offset);
// The last instruction might already created a row for this offset
// and we want to overwrite it.
- bool replace_existing = true;
- unwind_plan.InsertRow(std::make_shared<UnwindPlan::Row>(*m_curr_row),
- replace_existing);
+ saved_unwind_states.insert_or_assign(current_offset, m_state);
}
// We are starting a new conditional block at the actual offset
- condition_block_start_offset = current_offset;
+ condition_block_start_state = it;
}
if (log && log->GetVerbose()) {
@@ -245,11 +189,10 @@ bool
UnwindAssemblyInstEmulation::GetNonCallSiteUnwindPlanFromAssembly(
if (m_forward_branch_offset != 0 &&
range.ContainsFileAddress(inst->GetAddress().GetFileAddress() +
m_forward_branch_offset)) {
- auto newrow = std::make_shared<UnwindPlan::Row>(*m_curr_row.get());
- newrow->SetOffset(current_offset + m_forward_branch_offset);
- saved_unwind_states.insert({current_offset + m_forward_branch_offset,
- {newrow, m_register_values}});
- unwind_plan.InsertRow(newrow);
+ if (auto [it, inserted] = saved_unwind_states.emplace(
+ current_offset + m_forward_branch_offset, m_state);
+ inserted)
+ it->second.row.SetOffset(current_offset + m_forward_branch_offset);
}
// Were there any changes to the CFI while evaluating this instruction?
@@ -258,23 +201,21 @@ bool
UnwindAssemblyInstEmulation::GetNonCallSiteUnwindPlanFromAssembly(
// current address
if (saved_unwind_states.count(current_offset +
inst->GetOpcode().GetByteSize()) == 0) {
- m_curr_row->SetOffset(current_offset +
+ m_state.row.SetOffset(current_offset +
inst->GetOpcode().GetByteSize());
- unwind_plan.InsertRow(m_curr_row);
- saved_unwind_states.insert(
- {current_offset + inst->GetOpcode().GetByteSize(),
- {m_curr_row, m_register_values}});
-
- // Allocate a new Row for m_curr_row, copy the current state
- // into it
- UnwindPlan::Row *newrow = new UnwindPlan::Row;
- *newrow = *m_curr_row.get();
- m_curr_row.reset(newrow);
+ saved_unwind_states.emplace(
+ current_offset + inst->GetOpcode().GetByteSize(), m_state);
}
}
}
+ for (auto &[_, state] : saved_unwind_states) {
+ unwind_plan.InsertRow(
+ std::make_shared<UnwindPlan::Row>(std::move(state.row)),
+ /*replace_existing=*/true);
+ }
}
+
if (log && log->GetVerbose()) {
StreamString strm;
lldb::addr_t base_addr = range.GetBaseAddress().GetFileAddress();
@@ -339,14 +280,14 @@ uint64_t
UnwindAssemblyInstEmulation::MakeRegisterKindValuePair(
void UnwindAssemblyInstEmulation::SetRegisterValue(
const RegisterInfo ®_info, const RegisterValue ®_value) {
- m_register_values[MakeRegisterKindValuePair(reg_info)] = reg_value;
+ m_state.register_values[MakeRegisterKindValuePair(reg_info)] = reg_value;
}
bool UnwindAssemblyInstEmulation::GetRegisterValue(const RegisterInfo
®_info,
RegisterValue ®_value) {
const uint64_t reg_id = MakeRegisterKindValuePair(reg_info);
- RegisterValueMap::const_iterator pos = m_register_values.find(reg_id);
- if (pos != m_register_values.end()) {
+ RegisterValueMap::const_iterator pos = m_state.register_values.find(reg_id);
+ if (pos != m_state.register_values.end()) {
reg_value = pos->second;
return true; // We had a real value that comes from an opcode that wrote
// to it...
@@ -444,7 +385,7 @@ size_t UnwindAssemblyInstEmulation::WriteMemory(
generic_regnum != LLDB_REGNUM_GENERIC_SP) {
if (m_pushed_regs.try_emplace(reg_num, addr).second) {
const int32_t offset = addr - m_initial_sp;
- m_curr_row->SetRegisterLocationToAtCFAPlusOffset(reg_num, offset,
+ m_state.row.SetRegisterLocationToAtCFAPlusOffset(reg_num, offset,
/*can_replace=*/true);
m_curr_row_modified = true;
}
@@ -548,13 +489,14 @@ bool UnwindAssemblyInstEmulation::WriteRegister(
// CFA offset
// with the same amount.
lldb::RegisterKind kind = m_unwind_plan_ptr->GetRegisterKind();
- if (m_fp_is_cfa && reg_info->kinds[kind] == m_cfa_reg_info.kinds[kind] &&
+ if (m_state.fp_is_cfa &&
+ reg_info->kinds[kind] == m_state.cfa_reg_info.kinds[kind] &&
context.GetInfoType() ==
EmulateInstruction::eInfoTypeRegisterPlusOffset &&
context.info.RegisterPlusOffset.reg.kinds[kind] ==
- m_cfa_reg_info.kinds[kind]) {
+ m_state.cfa_reg_info.kinds[kind]) {
const int64_t offset = context.info.RegisterPlusOffset.signed_offset;
- m_curr_row->GetCFAValue().IncOffset(-1 * offset);
+ m_state.row.GetCFAValue().IncOffset(-1 * offset);
m_curr_row_modified = true;
}
} break;
@@ -590,25 +532,25 @@ bool UnwindAssemblyInstEmulation::WriteRegister(
case EmulateInstruction::eInfoTypeAddress:
if (auto it = m_pushed_regs.find(reg_num);
it != m_pushed_regs.end() && context.info.address == it->second) {
- m_curr_row->SetRegisterLocationToSame(reg_num,
+ m_state.row.SetRegisterLocationToSame(reg_num,
false /*must_replace*/);
m_curr_row_modified = true;
// FP has been restored to its original value, we are back
// to using SP to calculate the CFA.
- if (m_fp_is_cfa) {
- m_fp_is_cfa = false;
+ if (m_state.fp_is_cfa) {
+ m_state.fp_is_cfa = false;
lldb::RegisterKind sp_reg_kind = eRegisterKindGeneric;
uint32_t sp_reg_num = LLDB_REGNUM_GENERIC_SP;
RegisterInfo sp_reg_info =
*m_inst_emulator_up->GetRegisterInfo(sp_reg_kind, sp_reg_num);
RegisterValue sp_reg_val;
if (GetRegisterValue(sp_reg_info, sp_reg_val)) {
- m_cfa_reg_info = sp_reg_info;
+ m_state.cfa_reg_info = sp_reg_info;
const uint32_t cfa_reg_num =
sp_reg_info.kinds[m_unwind_plan_ptr->GetRegisterKind()];
assert(cfa_reg_num != LLDB_INVALID_REGNUM);
- m_curr_row->GetCFAValue().SetIsRegisterPlusOffset(
+ m_state.row.GetCFAValue().SetIsRegisterPlusOffset(
cfa_reg_num, m_initial_sp - sp_reg_val.GetAsUInt64());
}
}
@@ -620,7 +562,7 @@ bool UnwindAssemblyInstEmulation::WriteRegister(
generic_regnum == LLDB_REGNUM_GENERIC_FLAGS) &&
"eInfoTypeISA used for popping a register other the PC/FLAGS");
if (generic_regnum != LLDB_REGNUM_GENERIC_FLAGS) {
- m_curr_row->SetRegisterLocationToSame(reg_num,
+ m_state.row.SetRegisterLocationToSame(reg_num,
false /*must_replace*/);
m_curr_row_modified = true;
}
@@ -633,26 +575,26 @@ bool UnwindAssemblyInstEmulation::WriteRegister(
} break;
case EmulateInstruction::eContextSetFramePointer:
- if (!m_fp_is_cfa) {
- m_fp_is_cfa = true;
- m_cfa_reg_info = *reg_info;
+ if (!m_state.fp_is_cfa) {
+ m_state.fp_is_cfa = true;
+ m_state.cfa_reg_info = *reg_info;
const uint32_t cfa_reg_num =
reg_info->kinds[m_unwind_plan_ptr->GetRegisterKind()];
assert(cfa_reg_num != LLDB_INVALID_REGNUM);
- m_curr_row->GetCFAValue().SetIsRegisterPlusOffset(
+ m_state.row.GetCFAValue().SetIsRegisterPlusOffset(
cfa_reg_num, m_initial_sp - reg_value.GetAsUInt64());
m_curr_row_modified = true;
}
break;
case EmulateInstruction::eContextRestoreStackPointer:
- if (m_fp_is_cfa) {
- m_fp_is_cfa = false;
- m_cfa_reg_info = *reg_info;
+ if (m_state.fp_is_cfa) {
+ m_state.fp_is_cfa = false;
+ m_state.cfa_reg_info = *reg_info;
const uint32_t cfa_reg_num =
reg_info->kinds[m_unwind_plan_ptr->GetRegisterKind()];
assert(cfa_reg_num != LLDB_INVALID_REGNUM);
- m_curr_row->GetCFAValue().SetIsRegisterPlusOffset(
+ m_state.row.GetCFAValue().SetIsRegisterPlusOffset(
cfa_reg_num, m_initial_sp - reg_value.GetAsUInt64());
m_curr_row_modified = true;
}
@@ -661,9 +603,9 @@ bool UnwindAssemblyInstEmulation::WriteRegister(
case EmulateInstruction::eContextAdjustStackPointer:
// If we have created a frame using the frame pointer, don't follow
// subsequent adjustments to the stack pointer.
- if (!m_fp_is_cfa) {
- m_curr_row->GetCFAValue().SetIsRegisterPlusOffset(
- m_curr_row->GetCFAValue().GetRegisterNumber(),
+ if (!m_state.fp_is_cfa) {
+ m_state.row.GetCFAValue().SetIsRegisterPlusOffset(
+ m_state.row.GetCFAValue().GetRegisterNumber(),
m_initial_sp - reg_value.GetAsUInt64());
m_curr_row_modified = true;
}
diff --git
a/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.h
b/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.h
index d03a8ce4e9241..96a0881eaee9d 100644
---
a/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.h
+++
b/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.h
@@ -63,10 +63,8 @@ class UnwindAssemblyInstEmulation : public
lldb_private::UnwindAssembly {
UnwindAssemblyInstEmulation(const lldb_private::ArchSpec &arch,
lldb_private::EmulateInstruction *inst_emulator)
: UnwindAssembly(arch), m_inst_emulator_up(inst_emulator),
- m_range_ptr(nullptr), m_unwind_plan_ptr(nullptr), m_curr_row(),
- m_initial_sp(0), m_cfa_reg_info(), m_fp_is_cfa(false),
- m_register_values(), m_pushed_regs(), m_curr_row_modified(false),
- m_forward_branch_offset(0) {
+ m_range_ptr(nullptr), m_unwind_plan_ptr(nullptr), m_initial_sp(0),
+ m_curr_row_modified(false), m_forward_branch_offset(0) {
if (m_inst_emulator_up) {
m_inst_emulator_up->SetBaton(this);
m_inst_emulator_up->SetCallbacks(ReadMemory, WriteMemory, ReadRegister,
@@ -124,16 +122,20 @@ class UnwindAssemblyInstEmulation : public
lldb_private::UnwindAssembly {
bool GetRegisterValue(const lldb_private::RegisterInfo ®_info,
lldb_private::RegisterValue ®_value);
+ typedef std::map<uint64_t, lldb_private::RegisterValue> RegisterValueMap;
+ struct UnwindState {
+ lldb_private::UnwindPlan::Row row = {};
+ lldb_private::RegisterInfo cfa_reg_info = {};
+ bool fp_is_cfa = false;
+ RegisterValueMap register_values = {};
+ };
+
std::unique_ptr<lldb_private::EmulateInstruction> m_inst_emulator_up;
lldb_private::AddressRange *m_range_ptr;
lldb_private::UnwindPlan *m_unwind_plan_ptr;
- lldb_private::UnwindPlan::RowSP m_curr_row;
+ UnwindState m_state;
typedef std::map<uint64_t, uint64_t> PushedRegisterToAddrMap;
uint64_t m_initial_sp;
- lldb_private::RegisterInfo m_cfa_reg_info;
- bool m_fp_is_cfa;
- typedef std::map<uint64_t, lldb_private::RegisterValue> RegisterValueMap;
- RegisterValueMap m_register_values;
PushedRegisterToAddrMap m_pushed_regs;
// While processing the instruction stream, we need to communicate some state
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
