[Lldb-commits] [PATCH] D64993: Fix PC adjustment in StackFrame::GetSymbolContext

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda accepted this revision.
jasonmolenda added a comment.

This looks good, this is in line with what we discussed, thanks for taking it on!  Sorry for the delay at looking this over, it has been a little crazy this week.



================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextLLDB.cpp:1760
+void RegisterContextLLDB::PropagateTrapHandlerFlag(
+    lldb::UnwindPlanSP unwind_plan) {
+  if (unwind_plan->GetUnwindPlanForSignalTrap() != eLazyBoolYes) {
----------------
clayborg wrote:
> JosephTremoulet wrote:
> > I'm a bit ambivalent about adding this part -- the downside is that it's not concretely helping today, because if an eh_frame record has the 'S' flag in its augmentation (which is what `unwind_Plan->GetUnwindPlanForSignalTrap()` reports), we'll have already decremented pc and generated unwind plans based on the prior function when initializing the register context.  But the upside is that this connects the dots between the otherwise-unused bit on the unwind plan and the frame type, and will be in place should we subsequently add code to try the second function's unwind plan as a fallback.
> I will let Jason comment on this one.
Yeah, this was my impression of the S augmentation flag in the eh_frame too -- we can't really use it in lldb today without forcing a scan of eh_frame entries the first time we unwind a function from that Module, and that would be unfortunate.  But I like to see the flag being parsed and recorded; at some point in the future we may find a good way to use it.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextLLDB.cpp:1744
+    // frame may in fact be the first instruction of a signal return
+    // trampoline, rather than the instruction after a call.
+    UnwindLogMsg("Resetting current offset and re-doing symbol lookup; "
----------------
This is more a personal preference thing, but I would explicitly call out that we're addressing a problem on a platform where a trap handler is "added" to the stack by the kernel, but it hasn't actually been called and executed the way a normal function is, the PC is sitting on the first instruction of the function, so backing the pc up by 1 points us to the wrong function.

The kernel is presenting us with a stack frame that doesn't follow the normal ABI and execution models, and we're hacking around that.  It's perfectly OK to do that, but it's a specific enough thing we're addressing here, I think it'll be helpful to Future Us to name it explicitly.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextLLDB.h:125
+  /// update frame type and symbol context if so.
+  void PropagateTrapHandlerFlag(lldb::UnwindPlanSP unwind_plan);
+
----------------
This is very much a personal preference, but I might call this something like UpdateTrapHandlerFlagFromUnwindPlan.  I'm fine with this name if you find it clearer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64993