[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I'm sorry for terse response. I wanted to ensure this gets more attention, but I wasn't able to look at this straight away. Let me try to elaborate.

We generally try to avoid platform-specific code in the core libraries. The unwind code is one of the biggest offenders here, but I'd still want to avoid making it worse, particularly if there is a fairly simple way to avoid that. And I think there might be one. We already involve the Platform class in the unwinding logic (see Platform::GetTrapHandlerSymbolNames). Since it already knows about their names, it doesn't seem far-fetched to have it provide a way to unwind out of them (if necessary). Creating a new Platform entry point to provide the unwind plan (or somehow refactoring the GetTrapHandlerSymbolNames so that it can also provide an unwind plan) would enable us to put this code into PlatformLinux, which (although not ideal) seems much better than smacking it into the middle of the Symbol library. It also seems consistent with the intent in one of the comments in the code you modify.



================
Comment at: lldb/source/Symbol/AArch64UnwindInfo.cpp:58
+  unwind_plan_sp->SetSourcedFromCompiler(eLazyBoolYes);
+  unwind_plan_sp->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
+  unwind_plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolYes);
----------------
DavidSpickett wrote:
> If this means "at all instructions within that function" this could be yes, but no one seems to read this field anyway.
The way I read it is "all locations within the range of this unwind plan" as given by UnwindPlan::GetAddressRange. compiler-generated unwind plans might cover the entire function, but only be valid (correct) at call sites.


================
Comment at: lldb/source/Target/RegisterContextUnwind.cpp:900
   // 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.
----------------
this comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112069