jasonmolenda updated this revision to Diff 257557. jasonmolenda added a comment.
Update to address mistake Greg identified; also remove two unused variables that were in this method before my changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78077/new/ https://reviews.llvm.org/D78077 Files: lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp
Index: lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp =================================================================== --- lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp +++ lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp @@ -678,3 +678,103 @@ 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()); +} + Index: lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp =================================================================== --- lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp +++ lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp @@ -125,18 +125,12 @@ // 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 @@ *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 @@ 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
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits