Fix the unwinding plan augmentation from x86 assembly

[jarin]

Summary

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

[jarin]

Pavel, could you possibly take a look? It looks like Jason is busy with something else...

[jasonmolenda]

Hi Jaroslav, I apologize for taking so long to look at this; I've been heads-down on a project the past few weeks an my email inbox is a disaster right now.

This patch looks good, thanks for doing this. The fragile thing is that we have two methods doing very similar things -- GetNonCallSiteUnwindPlanFromAssembly() and AugmentUnwindPlanFromCallSite() -- and we've got some divergence between the two that make it harder to spot bugs like this. I can't remember the history well, but I have a feeling that AugmentUnwindPlanFromCallSite() is basically an older version of how GetNonCallSiteUnwindPlanFromAssembly() was written.

[jarin]

Thank you for the review! Could you also possibly commit the change for me?

[labath]

I can do *that*: r374342

lldb/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp

if (cfa_reg != m_lldb_sp_regnum ||
     return false;
 
   UnwindPlan::RowSP original_last_row = unwind_plan.GetRowForFunctionOffset(-1);
 
   size_t offset = 0;
   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
   // aren't common with clang/gcc on x86 but it is possible.
   bool reinstate_unwind_state = false;
 
   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) {
       UnwindPlan::RowSP new_row(new UnwindPlan::Row());
       *new_row = *original_last_row;
       new_row->SetOffset(offset);
       unwind_plan.AppendRow(new_row);
       row = std::make_shared<UnwindPlan::Row>();
if (cfa_reg == m_lldb_sp_regnum) {
       }
     } else if (cfa_reg == m_lldb_fp_regnum) {
       // CFA register is fp.
 
       // The only case we care about is epilogue:
       //     [0x5d] pop %rbp/%ebp
       //  => [0xc3] ret
       if (pop_rbp_pattern_p() || leave_pattern_p()) {
-        offset += 1;
+        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.
 
       // This must be hand-written assembly.
       // Just trust eh_frame and assume we have finished.
       break;
     }
   }

lldb/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp

TEST_F(Testx86AssemblyInspectionEngine, TestSpillRegToStackViaMOVi386) {
   EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
   EXPECT_EQ(-344, regloc.GetOffset());
 
   EXPECT_TRUE(row_sp->GetRegisterInfo(k_esi, regloc));
   EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
   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;
   AddressRange sample_range;
   UnwindPlan unwind_plan(eRegisterKindLLDB);
   std::unique_ptr<x86AssemblyInspectionEngine> engine64 = Getx86_64Inspector();
 
   uint8_t data[] = {