[Lldb-commits] [PATCH] D63667: Support __kernel_rt_sigreturn in frame initialization

Mon Jul 15 15:01:40 PDT 2019

jasonmolenda added a comment.

In D63667#1558027 <https://reviews.llvm.org/D63667#1558027>, @JosephTremoulet wrote:

> I've updated this with code to recognize the 'S' in the eh_frame augmentation and record it in a new bool member of UnwindPlan, `m_plan_is_for_signal_trap`.  I haven't hooked up any consumers of the new bit; as you say, with the current code flow we don't parse the eh_frame info until after we've decided whether the current frame is a trap handler frame or not.
>
> I took a look at `behaves_like_zeroth_frame`.  In my case, we're not getting to the point of consulting it, because before checking it, if we are processing a trap handler frame or if AlwaysRelyOnEHInfo returns true (both of which are true in my case), we use the EH Frame unwind plan if it is valid at the current address, and that valid at current address check is already passing.  I'm somewhat reluctant to fiddle with the behaves_like_zeroth_frame bit speculatively.  Note that with this change, it will get set for the next frame above __kernel_rt_sigreturn (because the current test checks if the next-lower frame is an eTrapHandler frame), which I think is where we want the async processing.

Just just to check, we've got a backtrace like

#0  trap_handler_function_in_my_program
#1 __kernel_rt_sigreturn
#2 function_that_was_interrupted_asynchonrously
#3 main

The eh_frame for #1 has the S bit, and we've hard-coded the name of the function as a known asynchronous signal handler.  When we're choosing an unwind plan for frame #2, or symbolicating the address, we need to treat this as a "behaves like zeroth frame" function - it could be on the first instruction, so backing the pc up by one would put us in the wrong function scope.

> I see that we also do this "subtract 1 from the pc" thing in StackFrame::GetSymbolContext, and I believe this is why even with my change here I'm seeing the wrong function name in backtraces (e.g., in the new test, it shows "abort_caller -> [function that happened to precede __kernel_rt_sigreturn] -> handler" rather than "abort_caller -> kernel_rt_sigreturn -> handler").  I'm not sure how best to address that, since the base StackFrame and RegisterContext classes don't have this notion of "frame type" / "trap handler frame" that the "-LLDB" subclasses have -- seems like we'd need to have GetFrameInfoAtIndex have another out parameter to specify frame type, and UnwindLLDB's implementation could then pull it from the RegisterContextLLDB -- would that be a good change?  I don't actually need that for my current use case, the "wrong" name in the backtrace is fine...

I think Unwind::GetFrameInfoAtIndex can take a new output argument, bool behaves_like_zeroth_frame.  It would be set to true if idx == 0 or if m_frames[idx - 1].IsTrapHandlerFrame() is true.

StackFrame.h would pick up a new ivar m_behaves_like_zeroth_frame and accessor method BehavesLikeZerothFrame().

StackFrameList::GetFramesUpTo will fetch the value via unwinder->GetFrameInfoAtIndex() and pass it as an argument to the StackFrame::StackFrame ctor.

Then in StackFrame::GetSymbolContext we can use the m_behaves_like_zeroth_frame to fix the symbol lookup logic.

I'm not wedded to the behaves_like_zeroth_frame nomenclature.  But I think this approach makes sense.

> Thanks for the notes on the asynchronous cases.  I'd certainly be interested in getting those to work, though I haven't sorted out from where the unwinder can find the above-trap frame's register values (happy for pointers here!).  I think I'm running into what this comment from GetFullUnwindPlanForFrame describes:
> 
>   // If we're in _sigtramp(), unwinding past this frame requires special
>   // knowledge.  On Mac OS X this knowledge is properly encoded in the eh_frame
>   // section, so prefer that if available. On other platforms we may need to
>   // provide a platform-specific UnwindPlan which encodes the details of how to
>   // unwind out of sigtramp.
>    
> 
> I'd like to get the synchronous case support working as a first step regardless.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63667