[Lldb-commits] [lldb] 1cd92e4 - Bug where insn-based unwind plans on arm64 could be wrong
Jason Molenda via lldb-commits
lldb-commits at lists.llvm.org
Tue Apr 14 16:57:56 PDT 2020
Author: Jason Molenda
Date: 2020-04-14T16:57:25-07:00
New Revision: 1cd92e480c12c03ab9a381b29e4e3964892afa01
URL: https://github.com/llvm/llvm-project/commit/1cd92e480c12c03ab9a381b29e4e3964892afa01
DIFF: https://github.com/llvm/llvm-project/commit/1cd92e480c12c03ab9a381b29e4e3964892afa01.diff
LOG: Bug where insn-based unwind plans on arm64 could be wrong
Fix a bug where UnwindAssemblyInstEmulation would confuse which
register is used to compute the Canonical Frame Address after it
had branched over a mid-function epilogue (where the CFA reg changes
from $fp to $sp in the process of epiloguing). Reinstate the
correct CFA register after we forward the unwind rule for branch
targets. The failure mode was that UnwindAssemblyInstEmulation
would think CFA was set in terms of $sp after one of these epilogues,
and if it sees modifications to $sp after the branch target, it would
change the CFA offset in the unwind rule -- even though the CFA is
defined in terms of $fp and the $sp changes are irrelevant to correct
calculation.
<rdar://problem/60300528>
Differential Revision: https://reviews.llvm.org/D78077
Added:
Modified:
lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp
Removed:
################################################################################
diff --git a/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp b/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
index ba7544fb52dd..1bc071c2b161 100644
--- a/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
+++ b/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
@@ -125,18 +125,12 @@ bool UnwindAssemblyInstEmulation::GetNonCallSiteUnwindPlanFromAssembly(
// Add the initial state to the save list with offset 0.
saved_unwind_states.insert({0, {last_row, m_register_values}});
- // cache the pc register number (in whatever register numbering this
- // UnwindPlan uses) for quick reference during instruction parsing.
- RegisterInfo pc_reg_info;
- m_inst_emulator_up->GetRegisterInfo(
- eRegisterKindGeneric, LLDB_REGNUM_GENERIC_PC, pc_reg_info);
-
- // cache the return address register number (in whatever register
+ // cache the stack pointer register number (in whatever register
// numbering this UnwindPlan uses) for quick reference during
// instruction parsing.
- RegisterInfo ra_reg_info;
+ RegisterInfo sp_reg_info;
m_inst_emulator_up->GetRegisterInfo(
- eRegisterKindGeneric, LLDB_REGNUM_GENERIC_RA, ra_reg_info);
+ eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP, sp_reg_info);
// The architecture dependent condition code of the last processed
// instruction.
@@ -167,6 +161,23 @@ bool UnwindAssemblyInstEmulation::GetNonCallSiteUnwindPlanFromAssembly(
*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_inst_emulator_up->GetRegisterInfo(row_kind, row_cfa_regnum,
+ m_cfa_reg_info);
+ // 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_inst_emulator_up->SetInstruction(inst->GetOpcode(),
@@ -197,6 +208,23 @@ bool UnwindAssemblyInstEmulation::GetNonCallSiteUnwindPlanFromAssembly(
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_inst_emulator_up->GetRegisterInfo(row_kind, row_cfa_regnum,
+ m_cfa_reg_info);
+ // 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;
+ }
bool replace_existing =
true; // The last instruction might already
// created a row for this offset and
diff --git a/lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp b/lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp
index 3be15c04a203..6c8bfa1a57f6 100644
--- a/lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp
+++ b/lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp
@@ -678,3 +678,103 @@ TEST_F(TestArm64InstEmulation, TestRegisterDoubleSpills) {
EXPECT_TRUE(regloc.IsSame());
}
}
+
+TEST_F(TestArm64InstEmulation, TestCFARegisterTrackedAcrossJumps) {
+ ArchSpec arch("arm64-apple-ios10");
+ std::unique_ptr<UnwindAssemblyInstEmulation> engine(
+ static_cast<UnwindAssemblyInstEmulation *>(
+ UnwindAssemblyInstEmulation::CreateInstance(arch)));
+ ASSERT_NE(nullptr, engine);
+
+ UnwindPlan::RowSP row_sp;
+ AddressRange sample_range;
+ UnwindPlan unwind_plan(eRegisterKindLLDB);
+ UnwindPlan::Row::RegisterLocation regloc;
+
+ uint8_t data[] = {
+ // prologue
+ 0xf4, 0x4f, 0xbe, 0xa9, // 0: 0xa9be4ff4 stp x20, x19, [sp, #-0x20]!
+ 0xfd, 0x7b, 0x01, 0xa9, // 4: 0xa9017bfd stp x29, x30, [sp, #0x10]
+ 0xfd, 0x43, 0x00, 0x91, // 8: 0x910043fd add x29, sp, #0x10
+ 0xff, 0x43, 0x00, 0xd1, // 12: 0xd10043ff sub sp, sp, #0x10
+ // conditional branch over a mid-function epilogue
+ 0xeb, 0x00, 0x00, 0x54, // 16: 0x540000eb b.lt <+44>
+ // mid-function epilogue
+ 0x1f, 0x20, 0x03, 0xd5, // 20: 0xd503201f nop
+ 0xe0, 0x03, 0x13, 0xaa, // 24: 0xaa1303e0 mov x0, x19
+ 0xbf, 0x43, 0x00, 0xd1, // 28: 0xd10043bf sub sp, x29, #0x10
+ 0xfd, 0x7b, 0x41, 0xa9, // 32: 0xa9417bfd ldp x29, x30, [sp, #0x10]
+ 0xf4, 0x4f, 0xc2, 0xa8, // 36: 0xa8c24ff4 ldp x20, x19, [sp], #0x20
+ 0xc0, 0x03, 0x5f, 0xd6, // 40: 0xd65f03c0 ret
+ // unwind state restored, we're using a frame pointer, let's change the
+ // stack pointer and see no change in how the CFA is computed
+ 0x1f, 0x20, 0x03, 0xd5, // 44: 0xd503201f nop
+ 0xff, 0x43, 0x00, 0xd1, // 48: 0xd10043ff sub sp, sp, #0x10
+ 0x1f, 0x20, 0x03, 0xd5, // 52: 0xd503201f nop
+ // final epilogue
+ 0xe0, 0x03, 0x13, 0xaa, // 56: 0xaa1303e0 mov x0, x19
+ 0xbf, 0x43, 0x00, 0xd1, // 60: 0xd10043bf sub sp, x29, #0x10
+ 0xfd, 0x7b, 0x41, 0xa9, // 64: 0xa9417bfd ldp x29, x30, [sp, #0x10]
+ 0xf4, 0x4f, 0xc2, 0xa8, // 68: 0xa8c24ff4 ldp x20, x19, [sp], #0x20
+ 0xc0, 0x03, 0x5f, 0xd6, // 72: 0xd65f03c0 ret
+
+ 0x1f, 0x20, 0x03, 0xd5, // 52: 0xd503201f nop
+ };
+
+ // UnwindPlan we expect:
+ // row[0]: 0: CFA=sp +0 =>
+ // row[1]: 4: CFA=sp+32 => x19=[CFA-24] x20=[CFA-32]
+ // row[2]: 8: CFA=sp+32 => x19=[CFA-24] x20=[CFA-32] fp=[CFA-16] lr=[CFA-8]
+ // row[3]: 12: CFA=fp+16 => x19=[CFA-24] x20=[CFA-32] fp=[CFA-16] lr=[CFA-8]
+ // row[4]: 32: CFA=sp+32 => x19=[CFA-24] x20=[CFA-32] fp=[CFA-16] lr=[CFA-8]
+ // row[5]: 36: CFA=sp+32 => x19=[CFA-24] x20=[CFA-32] fp= <same> lr= <same>
+ // row[6]: 40: CFA=sp +0 => x19= <same> x20= <same> fp= <same> lr= <same>
+ // row[7]: 44: CFA=fp+16 => x19=[CFA-24] x20=[CFA-32] fp=[CFA-16] lr=[CFA-8]
+ // row[8]: 64: CFA=sp+32 => x19=[CFA-24] x20=[CFA-32] fp=[CFA-16] lr=[CFA-8]
+ // row[9]: 68: CFA=sp+32 => x19=[CFA-24] x20=[CFA-32] fp= <same> lr= <same>
+ // row[10]: 72: CFA=sp +0 => x19= <same> x20= <same> fp= <same> lr= <same>
+
+ // The specific bug we're looking for is this incorrect CFA definition,
+ // where the InstEmulation is using the $sp value mixed in with $fp,
+ // it looks like this:
+ //
+ // row[7]: 44: CFA=fp+16 => x19=[CFA-24] x20=[CFA-32] fp=[CFA-16] lr=[CFA-8]
+ // row[8]: 52: CFA=fp+64 => x19=[CFA-24] x20=[CFA-32] fp=[CFA-16] lr=[CFA-8]
+ // row[9]: 68: CFA=fp+64 => x19=[CFA-24] x20=[CFA-32] fp= <same> lr= <same>
+
+ sample_range = AddressRange(0x1000, sizeof(data));
+
+ EXPECT_TRUE(engine->GetNonCallSiteUnwindPlanFromAssembly(
+ sample_range, data, sizeof(data), unwind_plan));
+
+ // Confirm CFA at mid-func epilogue 'ret' is $sp+0
+ row_sp = unwind_plan.GetRowForFunctionOffset(40);
+ EXPECT_EQ(40ull, row_sp->GetOffset());
+ EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == gpr_sp_arm64);
+ EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+ EXPECT_EQ(0, row_sp->GetCFAValue().GetOffset());
+
+ // After the 'ret', confirm we're back to the correct CFA of $fp+16
+ row_sp = unwind_plan.GetRowForFunctionOffset(44);
+ EXPECT_EQ(44ull, row_sp->GetOffset());
+ EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == gpr_fp_arm64);
+ EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+ EXPECT_EQ(16, row_sp->GetCFAValue().GetOffset());
+
+ // Confirm that we have no additional UnwindPlan rows before the
+ // real epilogue -- we still get the Row at offset 44.
+ row_sp = unwind_plan.GetRowForFunctionOffset(60);
+ EXPECT_EQ(44ull, row_sp->GetOffset());
+ EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == gpr_fp_arm64);
+ EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+ EXPECT_EQ(16, row_sp->GetCFAValue().GetOffset());
+
+ // And in the epilogue, confirm that we start by switching back to
+ // defining the CFA in terms of $sp.
+ row_sp = unwind_plan.GetRowForFunctionOffset(64);
+ EXPECT_EQ(64ull, row_sp->GetOffset());
+ EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == gpr_sp_arm64);
+ EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+ EXPECT_EQ(32, row_sp->GetCFAValue().GetOffset());
+}
+
More information about the lldb-commits
mailing list