[lldb] Slide eh_frame unwind plan if it doesn't begin at function boundary

[labath]

This is mainly useful for discontinuous functions because individual parts of the function will have separate FDE entries, which can begin many megabytes from the start of the function. However, I'm separating it out, because it turns out we already have a test case for the situation where the FDE does not begin exactly at the function boundary.

The test works mostly by accident because the FDE starts only one byte after the beginning of the function so it doesn't really matter whether one looks up the unwind row using the function or fde offset. In this patch, I beef up the test to catch this problem more reliably.

To make this work I've also needed to change a couple of places which that an unwind plan always has a row at offset zero.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

This is mainly useful for discontinuous functions because individual parts of the function will have separate FDE entries, which can begin many megabytes from the start of the function. However, I'm separating it out, because it turns out we already have a test case for the situation where the FDE does not begin exactly at the function boundary.

The test works mostly by accident because the FDE starts only one byte after the beginning of the function so it doesn't really matter whether one looks up the unwind row using the function or fde offset. In this patch, I beef up the test to catch this problem more reliably.

To make this work I've also needed to change a couple of places which that an unwind plan always has a row at offset zero.

---

Full diff: https://github.com/llvm/llvm-project/pull/135333.diff

6 Files Affected:

• (modified) lldb/source/Plugins/UnwindAssembly/x86/UnwindAssembly-x86.cpp (+1-1)
• (modified) lldb/source/Symbol/DWARFCallFrameInfo.cpp (+5-1)
• (modified) lldb/source/Symbol/UnwindPlan.cpp (+1-1)
• (modified) lldb/test/Shell/Unwind/Inputs/eh-frame-small-fde.s (+10-2)
• (modified) lldb/test/Shell/Unwind/eh-frame-small-fde.test (+4-3)
• (modified) lldb/unittests/Symbol/UnwindPlanTest.cpp (+6-1)

diff --git a/lldb/source/Plugins/UnwindAssembly/x86/UnwindAssembly-x86.cpp b/lldb/source/Plugins/UnwindAssembly/x86/UnwindAssembly-x86.cpp
index 5f0a863e936d4..f5279758a147c 100644
--- a/lldb/source/Plugins/UnwindAssembly/x86/UnwindAssembly-x86.cpp
+++ b/lldb/source/Plugins/UnwindAssembly/x86/UnwindAssembly-x86.cpp
@@ -73,7 +73,7 @@ bool UnwindAssembly_x86::AugmentUnwindPlanFromCallSite(
 
   int wordsize = 8;
   ProcessSP process_sp(thread.GetProcess());
-  if (process_sp.get() == nullptr)
+  if (!process_sp || !first_row || !last_row)
     return false;
 
   wordsize = process_sp->GetTarget().GetArchitecture().GetAddressByteSize();
diff --git a/lldb/source/Symbol/DWARFCallFrameInfo.cpp b/lldb/source/Symbol/DWARFCallFrameInfo.cpp
index fb57c61d413aa..a763acb1fdf9e 100644
--- a/lldb/source/Symbol/DWARFCallFrameInfo.cpp
+++ b/lldb/source/Symbol/DWARFCallFrameInfo.cpp
@@ -190,8 +190,12 @@ bool DWARFCallFrameInfo::GetUnwindPlan(const AddressRange &range,
   unwind_plan.SetUnwindPlanForSignalTrap(fde->for_signal_trap ? eLazyBoolYes
                                                               : eLazyBoolNo);
   unwind_plan.SetReturnAddressRegister(fde->return_addr_reg_num);
-  for (UnwindPlan::Row &row : fde->rows)
+  int64_t slide =
+      fde->range.GetBaseAddress().GetFileAddress() - addr.GetFileAddress();
+  for (UnwindPlan::Row &row : fde->rows) {
+    row.SlideOffset(slide);
     unwind_plan.AppendRow(std::move(row));
+  }
 
   return true;
 }
diff --git a/lldb/source/Symbol/UnwindPlan.cpp b/lldb/source/Symbol/UnwindPlan.cpp
index 94c23137e8f12..b1a96b5e26840 100644
--- a/lldb/source/Symbol/UnwindPlan.cpp
+++ b/lldb/source/Symbol/UnwindPlan.cpp
@@ -474,7 +474,7 @@ bool UnwindPlan::PlanValidAtAddress(Address addr) const {
   // If the 0th Row of unwind instructions is missing, or if it doesn't provide
   // a register to use to find the Canonical Frame Address, this is not a valid
   // UnwindPlan.
-  const Row *row0 = GetRowForFunctionOffset(0);
+  const Row *row0 = GetRowAtIndex(0);
   if (!row0 ||
       row0->GetCFAValue().GetValueType() == Row::FAValue::unspecified) {
     Log *log = GetLog(LLDBLog::Unwind);
diff --git a/lldb/test/Shell/Unwind/Inputs/eh-frame-small-fde.s b/lldb/test/Shell/Unwind/Inputs/eh-frame-small-fde.s
index b08436af3f2ea..29decefad5e51 100644
--- a/lldb/test/Shell/Unwind/Inputs/eh-frame-small-fde.s
+++ b/lldb/test/Shell/Unwind/Inputs/eh-frame-small-fde.s
@@ -10,12 +10,20 @@ bar:
 
         .type   foo, @function
 foo:
-        nop # Make the FDE entry start one byte later than the actual function.
+        # Make the FDE entry start 16 bytes later than the actual function. The
+        # size is chosen such that it is larger than the size of the FDE entry.
+        # This allows us to test that we're using the correct offset for
+        # unwinding (we'll stop 21 bytes into the function, but only 5 bytes
+        # into the FDE).
+        .nops 16
         .cfi_startproc
         .cfi_register %rip, %r13
         call    bar
         addl    $1, %eax
-        jmp     *%r13 # Return
+        movq    %r13, %r14
+        .cfi_register %rip, %r14
+        movq    $0, %r13
+        jmp     *%r14 # Return
         .cfi_endproc
         .size   foo, .-foo
 
diff --git a/lldb/test/Shell/Unwind/eh-frame-small-fde.test b/lldb/test/Shell/Unwind/eh-frame-small-fde.test
index 0ece6c2a12a3e..d86d41f73f1c1 100644
--- a/lldb/test/Shell/Unwind/eh-frame-small-fde.test
+++ b/lldb/test/Shell/Unwind/eh-frame-small-fde.test
@@ -14,9 +14,10 @@ process launch
 
 thread backtrace
 # CHECK: frame #0: {{.*}}`bar
-# CHECK: frame #1: {{.*}}`foo + 6
+# CHECK: frame #1: {{.*}}`foo + 21
 # CHECK: frame #2: {{.*}}`main + 20
 
 target modules show-unwind -n foo
-# CHECK: eh_frame UnwindPlan:
-# CHECK: row[0]: 0: CFA=rsp +8 => rip=r13
+# CHECK:      eh_frame UnwindPlan:
+# CHECK:      row[0]:   16: CFA=rsp +8 => rip=r13
+# CHECK-NEXT: row[1]:   27: CFA=rsp +8 => rip=r14
diff --git a/lldb/unittests/Symbol/UnwindPlanTest.cpp b/lldb/unittests/Symbol/UnwindPlanTest.cpp
index 08aa5b2dd84bb..2d7ef43f83c19 100644
--- a/lldb/unittests/Symbol/UnwindPlanTest.cpp
+++ b/lldb/unittests/Symbol/UnwindPlanTest.cpp
@@ -62,15 +62,20 @@ TEST(UnwindPlan, PlanValidAtAddress) {
   UnwindPlan::Row row2 = make_simple_row(10, 47);
 
   UnwindPlan plan(eRegisterKindGeneric);
+  // When valid address range is not set, plans are valid as long as they have a
+  // row that sets the CFA.
   EXPECT_FALSE(plan.PlanValidAtAddress(Address(0)));
+  EXPECT_FALSE(plan.PlanValidAtAddress(Address(10)));
 
   plan.InsertRow(row2);
-  EXPECT_FALSE(plan.PlanValidAtAddress(Address(0)));
+  EXPECT_TRUE(plan.PlanValidAtAddress(Address(0)));
+  EXPECT_TRUE(plan.PlanValidAtAddress(Address(10)));
 
   plan.InsertRow(row1);
   EXPECT_TRUE(plan.PlanValidAtAddress(Address(0)));
   EXPECT_TRUE(plan.PlanValidAtAddress(Address(10)));
 
+  // With an address range, they're only valid within that range.
   plan.SetPlanValidAddressRanges({AddressRange(0, 5), AddressRange(15, 5)});
   EXPECT_TRUE(plan.PlanValidAtAddress(Address(0)));
   EXPECT_FALSE(plan.PlanValidAtAddress(Address(5)));

[labath]

This actually changes code back to the state before I started messing with it (#127661). In that patch, I changed it to use GetRowForFunctionOffset because I thought it better captures the original intent of the code ("check the value of CFA at function entry").

I still sort of think that, but now I also think that check is too strict (just because we don't have a CFA at offset zero, it doesn't mean we won't have it at some other offset). So this goes back to checking the first row (whatever it's offset), but I'm also open to other options (checking that any row contains the CFA, or maybe removing the check entirely -- just checking that a row exists)

[jasonmolenda]

OK looks reasonable.