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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
