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

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

In D112069#3120951 <https://reviews.llvm.org/D112069#3120951>, @DavidSpickett wrote:

> - Set locations for all general purpose registers
> - Set register type to DWARF due to that
> - Add a new test that sets known register values and compares them between signal catch and handler
> - Remove changes to handle abort test (though I could do those as a separate thing later)
>
> One thing remains, the sigcontext can include floating point and SVE
> registers. We'd need to read some memory to determine if it does:
> https://github.com/torvalds/linux/blob/master/arch/arm64/include/uapi/asm/sigcontext.h#L39
>
> Which I can do by passing the target and generating the plan based on
> what's present.
>
> For now let me know if the test case makes sense. Maybe this is ok
> to go in as is and I can follow up with the other registers?

Yes, this is definitely fine. Thanks for taking your time to do that.

Dynamically generating unwind plans based on the contents of memory kinda goes against the concept of unwind plans, which were meant to be static (and cacheable) descriptions of how to unwind from a given point (PC) in a program. I guess the most "pure" solution would be to encode this logic into the dwarf expressions for the relevant registers. I think this should be doable (dwarf expressions being turing-complete and all), but the resulting expressions would probably be horrendous.

Overall, I'm not too worried about not being able to recover non-gpr registers so (unless you really want to work on this) I think that can stay unimplemented. (I'd also be fine with not recovering gpr registers either, though it's obviously better if they are -- the reason this came up was because I was looking at ways to simplify the eRegisterKind business).



================
Comment at: lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp:322
+
+  unwind_plan_sp = std::make_shared<UnwindPlan>(eRegisterKindGeneric);
+  unwind_plan_sp->AppendRow(row);
----------------
Change to `eRegisterKindDWARF` and remove the `SetRegisterKind` call below.


================
Comment at: lldb/test/API/linux/aarch64/unwind_signal/TestUnwindSignal.py:82-83
+
+        # Save the backtrace frames to compare to the handler backtrace later.
+        signal_frames = [make_frame_info(f) for f in thread.get_thread_frames()]
+
----------------
I don't think this is bad, but you could just hardcode the expectations into the python file. Then you could skip this step and continue straight to the final breakpoint.


================
Comment at: lldb/test/API/linux/aarch64/unwind_signal/main.c:23
+      SETREG("24") SETREG("25") SETREG("26") SETREG("27")
+      SETREG("28") SETREG("29") SETREG("30") /* 31 is xzr/sp */
+      /* clang-format on */
----------------
DavidSpickett wrote:
> In my testing the kernel didn't go out of its way to change all the register values but it always changed a few so the test fails consistently if the unwind plan doesn't set the locations.
One could also have a similar block (but with different values) in the signal handler, which would guarantee that all registers are being read from the stack.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112069