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 <jarin@google.com>.

git-svn-id: https://llvm.org/svn/llvm-project/lldb/trunk@374342 91177308-0d34-0410-b5e6-96231b3b80d8

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<x86AssemblyInspectionEngine> 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<UnwindPlan::Row>();
+
+  // 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;