When picking a row from an UnwindPlan on a mid-stack frame, decr pc to get within bounds of the CALL instruction

[jasonmolenda]

Summary

This is addressing a bit of a big problem we're hitting with the way optimized code is getting generated for kernel binaries today; we have a central function that has a function call with one set of unwind rules, but the return address is a different block with really different unwind rules (maybe it's a noreturn call, I didn't read through the source tbh).

It's the usual problem with this kind of thing - we need to decrement the return pc before looking up the scope for unwind rules. The patch is straightforward, but I haven't come up with any way to test it short of "have a kernel binary and corefile" and I can't use either of those (no small part of it being that they're enormous). Not thrilled about this, but I haven't come up with a synthetic repo case for it.

I don't know who would be a good reviewer given how little I can provide here - I'll add Pavel and Greg, but don't feel obligated to give an opinion if you don't have time to look at the code. I would let this simmer for longer trying to come up with some kind of test case, but it happens to impact a code sequence that is hit by a lot of internal people here, so I need to get going with a fix.

[jasonmolenda]

Ah, I can't share the binary/corefile that led me to this issue but I can show what it looks like. I have a function with a noreturn call, it's a crashing scenario where (as always) this is the most important bit to get right, to show the crash,

    0xfffffff02058d2c4 <+3404>: mov    x1, x19
    0xfffffff02058d2c8 <+3408>: bl     -0xfdf3b49dc              ; panic_with_thread_kernel_state 
->  0xfffffff02058d2cc <+3412>: mov    x0, x19
    0xfffffff02058d2d0 <+3416>: mov    x1, x24

and the unwind plan rules for these instructions look like

row[115]: 3396: CFA=fp+16 => x5= <same> x8= <same> x19=[CFA-24] x20=[CFA-32] x21=[CFA-40] x22=[CFA-48] x23=[CFA-56] x24=[CFA-64] x25=[CFA-72] x26=[CFA-80] x27=[CFA-88] x28=[CFA-96] fp=[CFA-16] lr=[CFA-8] 
row[116]: 3412: CFA=fp+16 => x19= <same> x20= <same> x21= <same> x22= <same> x23= <same> x24= <same> x25= <same> x26= <same> x27= <same> x28= <same> fp= <same> lr= <same> 
row[117]: 3428: CFA=fp+16 => x8=[CFA-128] x19=[CFA-24] x20=[CFA-32] x21=[CFA-40] x22=[CFA-48] x23=[CFA-56] x24=[CFA-64] x25=[CFA-72] x26=[CFA-80] x27=[CFA-88] x28=[CFA-96] fp=[CFA-16] lr=[CFA-8]

We're under the BL call here, and if we use

row[115]: 3396: CFA=fp+16 => x5= <same> x8= <same> x19=[CFA-24] x20=[CFA-32] x21=[CFA-40] x22=[CFA-48] x23=[CFA-56] x24=[CFA-64] x25=[CFA-72] x26=[CFA-80] x27=[CFA-88] x28=[CFA-96] fp=[CFA-16] lr=[CFA-8]

we'll walk the stack correctly. But instead we're using the return address which has an unwind rule

row[116]: 3412: CFA=fp+16 => x19= <same> x20= <same> x21= <same> x22= <same> x23= <same> x24= <same> x25= <same> x26= <same> x27= <same> x28= <same> fp= <same> lr= <same>

(which is honestly real suspect in its own right and I'll be looking at what codepath lldb was following when it came up with that, it's a complex function and I don't have an opinion at first blush)

But the problem is obvious when you see this - the unwind state after a noreturn could be any old thing, and we can't use it reliably.

[clayborg]

Inline comments point out where can can get rid of the second parameter to IsUnwindPlanValidForCurrentPC() as no one uses it anymore.

lldb/source/Target/RegisterContextUnwind.cpp

519–522

This can easily just be:

m_behaves_like_zeroth_frame = !decr_pc_and_recompute_addr_range;

642–643

remove "valid_offset" variable and also remove it from the second argument to IsUnwindPlanValidForCurrentPC as it isn't used anywhere now and is just dead code.

[jasonmolenda]

lldb/source/Target/RegisterContextUnwind.cpp

642–643

hm, this merits consideration more widely; I looked at all callers to IsUnwindPlanValidForCurrentPC and none of them use the valid_offset that it provides; they are all merely checking that the offset is covered by the UnwindPlan's byte size. I suspect none of these are actually necessary; we picked the unwind plan based on symbol name so you'd need a "symbol" that has a large byte size, but an UnwindPlan that was sourced from some input that limited the byte size range covered. idk, it might be possible tho, especially in a stripped binary.

I'll change all of the callers to not use the returned valid_offset for now.

[clayborg]

lldb/source/Target/RegisterContextUnwind.cpp

642–643

If it is actually being used somewhere, then that is fine. My search of the code showed 3 locations, and this location was the only one that was using it, but now it isn't. Are there other locations I missed?

[jasonmolenda]

Update to address Greg's feedback.

[jasonmolenda]

lldb/source/Target/RegisterContextUnwind.cpp

642–643

I wrote that poorly. I meant to say that no one was using the valid_offset returned value, so I would change the method to not return it. I uploaded an updated patch showing what I mean here.

[jasonmolenda]

Accidentally included an unrelated change in the last update.

[clayborg]

lldb/source/Target/RegisterContextUnwind.cpp

90

Don't we want to subtract 1 here? Otherwise this function might return true incorrectly if we actually need to subtract at least 1 from the PC

[jasonmolenda]

lldb/source/Target/RegisterContextUnwind.cpp

90

honestly this method is a bit suspect, when we look at it today. It's first "is $pc within ByteSize", then "ok is pc-1 within ByteSize", with a bonus check if somehow we have a negative offset. Maybe it became suspect with revision over time. I don't think it's wrong, but it's not written with much clarity and I should look into how it's changed over time to see if there was a cleaner vision of its goals originally...

lldb/include/lldb/Target/RegisterContextUnwind.h

private:
 
   lldb::UnwindPlanSP GetFullUnwindPlanForFrame();
 
   void UnwindLogMsg(const char *fmt, ...) __attribute__((format(printf, 2, 3)));
 
   void UnwindLogMsgVerbose(const char *fmt, ...)
       __attribute__((format(printf, 2, 3)));
 
-  bool IsUnwindPlanValidForCurrentPC(lldb::UnwindPlanSP unwind_plan_sp,
-                                     int &valid_pc_offset);
+  bool IsUnwindPlanValidForCurrentPC(lldb::UnwindPlanSP unwind_plan_sp);
 
   lldb::addr_t GetReturnAddressHint(int32_t plan_offset);
 
   lldb_private::Thread &m_thread;
 
   ///
   // The following tell us how to retrieve the CALLER's register values (ie the
   // "previous" frame, aka the frame above)

lldb/source/Target/RegisterContextUnwind.cpp

RegisterContextUnwind::RegisterContextUnwind(Thread &thread,
   // not have been executed yet
   if (IsFrameZero() || next_frame->m_frame_type == eTrapHandlerFrame ||
       next_frame->m_frame_type == eDebuggerFrame) {
     m_all_registers_available = true;
   }
 }
 
 bool RegisterContextUnwind::IsUnwindPlanValidForCurrentPC(
-    lldb::UnwindPlanSP unwind_plan_sp, int &valid_pc_offset) {
+    lldb::UnwindPlanSP unwind_plan_sp) {
   if (!unwind_plan_sp)
     return false;
 
   // check if m_current_pc is valid
   if (unwind_plan_sp->PlanValidAtAddress(m_current_pc)) {
     // yes - current offset can be used as is
-    valid_pc_offset = m_current_offset;
     return true;
   }
 
   // if m_current_offset <= 0, we've got nothing else to try
   if (m_current_offset <= 0)
     return false;
 
   // check pc - 1 to see if it's valid
   Address pc_minus_one(m_current_pc);
   pc_minus_one.SetOffset(m_current_pc.GetOffset() - 1);
   if (unwind_plan_sp->PlanValidAtAddress(pc_minus_one)) {
-    // *valid_pc_offset = m_current_offset - 1;
-    valid_pc_offset = m_current_pc.GetOffset() - 1;
     return true;
   }
 
   return false;
 }
 
 // Initialize a RegisterContextUnwind which is the first frame of a stack -- the
 // zeroth frame or currently executing frame.
void RegisterContextUnwind::InitializeNonZerothFrame() {
   } else if (GetNextFrame()->m_frame_type == eTrapHandlerFrame ||
              GetNextFrame()->m_frame_type == eDebuggerFrame) {
     // Don't decrement if we're "above" an asynchronous event like
     // sigtramp.
     decr_pc_and_recompute_addr_range = false;
   } else if (!addr_range.GetBaseAddress().IsValid() ||
              addr_range.GetBaseAddress().GetSection() != m_current_pc.GetSection() ||
              addr_range.GetBaseAddress().GetOffset() != m_current_pc.GetOffset()) {
-    // If our "current" pc isn't the start of a function, no need
-    // to decrement and recompute.
+    // If our "current" pc isn't the start of a function, decrement the pc
+    // if we're up the stack.
+    if (m_behaves_like_zeroth_frame)
       decr_pc_and_recompute_addr_range = false;
+    else
+      decr_pc_and_recompute_addr_range = true;
   } else if (IsTrapHandlerSymbol(process, m_sym_ctx)) {
     // Signal dispatch may set the return address of the handler it calls to
     // point to the first byte of a return trampoline (like __kernel_rt_sigreturn),
     // so do not decrement and recompute if the symbol we already found is a trap
     // handler.
     decr_pc_and_recompute_addr_range = false;
   } else if (m_behaves_like_zeroth_frame) {
     decr_pc_and_recompute_addr_range = false;
   } else {
     // Decrement to find the function containing the call.
     decr_pc_and_recompute_addr_range = true;
if (active_row.get() && log) {
       active_row->Dump(active_row_strm, m_fast_unwind_plan_sp.get(), &m_thread,
                        m_start_pc.GetLoadAddress(exe_ctx.GetTargetPtr()));
       UnwindLogMsg("Using fast unwind plan '%s'",
                    m_fast_unwind_plan_sp->GetSourceName().AsCString());
       UnwindLogMsg("active row: %s", active_row_strm.GetData());
     }
   } else {
     m_full_unwind_plan_sp = GetFullUnwindPlanForFrame();
-    int valid_offset = -1;
-    if (IsUnwindPlanValidForCurrentPC(m_full_unwind_plan_sp, valid_offset)) {
-      active_row = m_full_unwind_plan_sp->GetRowForFunctionOffset(valid_offset);
+    if (IsUnwindPlanValidForCurrentPC(m_full_unwind_plan_sp)) {
+      active_row = m_full_unwind_plan_sp->GetRowForFunctionOffset(
+          m_current_offset_backed_up_one);
       row_register_kind = m_full_unwind_plan_sp->GetRegisterKind();
       PropagateTrapHandlerFlagFromUnwindPlan(m_full_unwind_plan_sp);
       if (active_row.get() && log) {
         StreamString active_row_strm;
         active_row->Dump(active_row_strm, m_full_unwind_plan_sp.get(),
                          &m_thread,
                          m_start_pc.GetLoadAddress(exe_ctx.GetTargetPtr()));
         UnwindLogMsg("Using full unwind plan '%s'",
                      m_full_unwind_plan_sp->GetSourceName().AsCString());
         UnwindLogMsg("active row: %s", active_row_strm.GetData());
UnwindPlanSP RegisterContextUnwind::GetFullUnwindPlanForFrame() {
   }
 
   // Typically this is unwind info from an eh_frame section intended for
   // exception handling; only valid at call sites
   if (process) {
     unwind_plan_sp = func_unwinders_sp->GetUnwindPlanAtCallSite(
         process->GetTarget(), m_thread);
   }
-  int valid_offset = -1;
-  if (IsUnwindPlanValidForCurrentPC(unwind_plan_sp, valid_offset)) {
+  if (IsUnwindPlanValidForCurrentPC(unwind_plan_sp)) {
     UnwindLogMsgVerbose("frame uses %s for full UnwindPlan because this "
                         "is the call-site unwind plan",
                         unwind_plan_sp->GetSourceName().GetCString());
     return unwind_plan_sp;
   }
 
   // We'd prefer to use an UnwindPlan intended for call sites when we're at a
   // call site but if we've struck out on that, fall back to using the non-
if (call_site_unwind_plan &&
         call_site_unwind_plan->GetSourceName() !=
             unwind_plan_sp->GetSourceName()) {
       m_fallback_unwind_plan_sp = call_site_unwind_plan;
     } else {
       m_fallback_unwind_plan_sp = arch_default_unwind_plan_sp;
     }
   }
 
-  if (IsUnwindPlanValidForCurrentPC(unwind_plan_sp, valid_offset)) {
+  if (IsUnwindPlanValidForCurrentPC(unwind_plan_sp)) {
     UnwindLogMsgVerbose("frame uses %s for full UnwindPlan because we "
                         "failed to find a call-site unwind plan that would work",
                         unwind_plan_sp->GetSourceName().GetCString());
     return unwind_plan_sp;
   }
 
   // If nothing else, use the architectural default UnwindPlan and hope that
   // does the job.
if (!m_full_unwind_plan_sp) {
       got_new_full_unwindplan = true;
     }
 
     if (m_full_unwind_plan_sp) {
       RegisterNumber pc_regnum(m_thread, eRegisterKindGeneric,
                                LLDB_REGNUM_GENERIC_PC);
 
       UnwindPlan::RowSP active_row =
-          m_full_unwind_plan_sp->GetRowForFunctionOffset(m_current_offset);
+          m_full_unwind_plan_sp->GetRowForFunctionOffset(
+              m_current_offset_backed_up_one);
       unwindplan_registerkind = m_full_unwind_plan_sp->GetRegisterKind();
 
       if (got_new_full_unwindplan && active_row.get() && log) {
         StreamString active_row_strm;
         ExecutionContext exe_ctx(m_thread.shared_from_this());
         active_row->Dump(active_row_strm, m_full_unwind_plan_sp.get(),
                          &m_thread,
                          m_start_pc.GetLoadAddress(exe_ctx.GetTargetPtr()));
bool RegisterContextUnwind::TryFallbackUnwindPlan() {
   addr_t old_cfa = m_cfa;
   addr_t old_afa = m_afa;
 
   m_registers.clear();
 
   m_full_unwind_plan_sp = m_fallback_unwind_plan_sp;
 
   UnwindPlan::RowSP active_row =
-      m_fallback_unwind_plan_sp->GetRowForFunctionOffset(m_current_offset);
+      m_fallback_unwind_plan_sp->GetRowForFunctionOffset(
+          m_current_offset_backed_up_one);
 
   if (active_row &&
       active_row->GetCFAValue().GetValueType() !=
           UnwindPlan::Row::FAValue::unspecified) {
     addr_t new_cfa;
     if (!ReadFrameAddress(m_fallback_unwind_plan_sp->GetRegisterKind(),
                             active_row->GetCFAValue(), new_cfa) ||
         new_cfa == 0 || new_cfa == 1 || new_cfa == LLDB_INVALID_ADDRESS) {