[Lldb-commits] [PATCH] D124957: When picking a row from an UnwindPlan on a mid-stack frame, decr pc to get within bounds of the CALL instruction

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added inline comments.


================
Comment at: lldb/source/Target/RegisterContextUnwind.cpp:642-643
     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(
----------------
clayborg wrote:
> jasonmolenda wrote:
> > clayborg wrote:
> > > 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.
> > 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.
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124957/new/

https://reviews.llvm.org/D124957