From 7449342b6caf25ed0251eb1325af2321f76a32bc Mon Sep 17 00:00:00 2001 From: Pavel Labath Date: Thu, 10 Oct 2019 13:23:09 +0000 Subject: Fix the unwinding plan augmentation from x86 assembly Unwind plan augmentation should compute the plan row at offset x from the instruction before offset x, but currently we compute it from the instruction at offset x. Note that this behavior is a regression introduced when moving the x86 assembly inspection engine to its own file (https://github.com/llvm/llvm-project/commit/1c9858b298d79ce82c45a2954096718b39550109#diff-375a2be066db6f34bb9a71442c9b71fcL913); the original version handled this properly by copying the previous instruction out before advancing the instruction pointer. The relevant bug with more info is here: https://bugs.llvm.org/show_bug.cgi?id=43561 Differential Revision: https://reviews.llvm.org/D68454 Patch by Jaroslav Sevcik . git-svn-id: https://llvm.org/svn/llvm-project/lldb/trunk@374342 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../x86/x86AssemblyInspectionEngine.cpp | 31 ++++---- .../x86/Testx86AssemblyInspectionEngine.cpp | 91 ++++++++++++++++++++++ 2 files changed, 106 insertions(+), 16 deletions(-) diff --git a/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp b/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp index 946d99cc0..bf6f60a2d 100644 --- a/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp +++ b/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp @@ -1371,7 +1371,6 @@ bool x86AssemblyInspectionEngine::AugmentUnwindPlanFromCallSite( int row_id = 1; bool unwind_plan_updated = false; UnwindPlan::RowSP row(new UnwindPlan::Row(*first_row)); - m_cur_insn = data + offset; // 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 @@ -1381,19 +1380,17 @@ bool x86AssemblyInspectionEngine::AugmentUnwindPlanFromCallSite( while (offset < size) { m_cur_insn = data + offset; int insn_len; - if (!instruction_length(m_cur_insn, insn_len, size - offset) - || insn_len == 0 - || insn_len > kMaxInstructionByteSize) { + if (!instruction_length(m_cur_insn, insn_len, size - offset) || + insn_len == 0 || insn_len > kMaxInstructionByteSize) { // An unrecognized/junk instruction. break; } // Advance offsets. offset += insn_len; - m_cur_insn = data + offset; // offset is pointing beyond the bounds of the function; stop looping. - if (offset >= size) + if (offset >= size) continue; if (reinstate_unwind_state) { @@ -1547,16 +1544,18 @@ bool x86AssemblyInspectionEngine::AugmentUnwindPlanFromCallSite( // [0x5d] pop %rbp/%ebp // => [0xc3] ret if (pop_rbp_pattern_p() || leave_pattern_p()) { - offset += 1; - row->SetOffset(offset); - row->GetCFAValue().SetIsRegisterPlusOffset( - first_row->GetCFAValue().GetRegisterNumber(), m_wordsize); - - UnwindPlan::RowSP new_row(new UnwindPlan::Row(*row)); - unwind_plan.InsertRow(new_row); - unwind_plan_updated = true; - reinstate_unwind_state = true; - continue; + m_cur_insn++; + if (ret_pattern_p()) { + row->SetOffset(offset); + row->GetCFAValue().SetIsRegisterPlusOffset( + first_row->GetCFAValue().GetRegisterNumber(), m_wordsize); + + UnwindPlan::RowSP new_row(new UnwindPlan::Row(*row)); + unwind_plan.InsertRow(new_row); + unwind_plan_updated = true; + reinstate_unwind_state = true; + continue; + } } } else { // CFA register is not sp or fp. diff --git a/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp b/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp index f8308c304..083a8175d 100644 --- a/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp +++ b/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp @@ -2199,6 +2199,97 @@ TEST_F(Testx86AssemblyInspectionEngine, TestSpillRegToStackViaMOVi386) { EXPECT_EQ(-40, regloc.GetOffset()); } +TEST_F(Testx86AssemblyInspectionEngine, TestSpArithx86_64Augmented) { + UnwindPlan::Row::RegisterLocation regloc; + UnwindPlan::RowSP row_sp; + AddressRange sample_range; + UnwindPlan unwind_plan(eRegisterKindLLDB); + std::unique_ptr engine64 = Getx86_64Inspector(); + + uint8_t data[] = { + 0x55, // pushq %rbp + 0x48, 0x89, 0xe5, // movq %rsp, %rbp + + // x86AssemblyInspectionEngine::AugmentUnwindPlanFromCallSite + // has a bug where it can't augment a function that is just + // prologue+epilogue - it needs at least one other instruction + // in between. + + 0x90, // nop + 0x48, 0x81, 0xec, 0x88, 0, 0, 0, // subq $0x88, %rsp + 0x90, // nop + 0x48, 0x81, 0xc4, 0x88, 0, 0, 0, // addq $0x88, %rsp + + 0x5d, // popq %rbp + 0xc3 // retq + }; + + sample_range = AddressRange(0x1000, sizeof(data)); + + unwind_plan.SetSourceName("unit testing hand-created unwind plan"); + unwind_plan.SetPlanValidAddressRange(sample_range); + unwind_plan.SetRegisterKind(eRegisterKindLLDB); + + row_sp = std::make_shared(); + + // Describe offset 0 + row_sp->SetOffset(0); + row_sp->GetCFAValue().SetIsRegisterPlusOffset(k_rsp, 8); + + regloc.SetAtCFAPlusOffset(-8); + row_sp->SetRegisterInfo(k_rip, regloc); + + unwind_plan.AppendRow(row_sp); + + // Allocate a new Row, populate it with the existing Row contents. + UnwindPlan::Row *new_row = new UnwindPlan::Row; + *new_row = *row_sp.get(); + row_sp.reset(new_row); + + // Describe offset 1 + row_sp->SetOffset(1); + row_sp->GetCFAValue().SetIsRegisterPlusOffset(k_rsp, 16); + regloc.SetAtCFAPlusOffset(-16); + row_sp->SetRegisterInfo(k_rbp, regloc); + unwind_plan.AppendRow(row_sp); + + // Allocate a new Row, populate it with the existing Row contents. + new_row = new UnwindPlan::Row; + *new_row = *row_sp.get(); + row_sp.reset(new_row); + + // Describe offset 4 + row_sp->SetOffset(4); + row_sp->GetCFAValue().SetIsRegisterPlusOffset(k_rsp, 16); + unwind_plan.AppendRow(row_sp); + + RegisterContextSP reg_ctx_sp; + EXPECT_TRUE(engine64->AugmentUnwindPlanFromCallSite( + data, sizeof(data), sample_range, unwind_plan, reg_ctx_sp)); + + // Before we touch the stack pointer, we should still refer to the + // row from after the prologue. + row_sp = unwind_plan.GetRowForFunctionOffset(5); + EXPECT_EQ(4ull, row_sp->GetOffset()); + + // Check the first stack pointer update. + row_sp = unwind_plan.GetRowForFunctionOffset(12); + EXPECT_EQ(12ull, row_sp->GetOffset()); + EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_rsp); + EXPECT_EQ(152, row_sp->GetCFAValue().GetOffset()); + + // After the nop, we should still refer to the same row. + row_sp = unwind_plan.GetRowForFunctionOffset(13); + EXPECT_EQ(12ull, row_sp->GetOffset()); + + // Check that the second stack pointer update is reflected in the + // unwind plan. + row_sp = unwind_plan.GetRowForFunctionOffset(20); + EXPECT_EQ(20ull, row_sp->GetOffset()); + EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_rsp); + EXPECT_EQ(16, row_sp->GetCFAValue().GetOffset()); +} + TEST_F(Testx86AssemblyInspectionEngine, TestSimplex86_64Augmented) { UnwindPlan::Row::RegisterLocation regloc; UnwindPlan::RowSP row_sp; -- cgit v1.2.3