Page MenuHomePhabricator

[libunwind][RISCV] Track PC separately from RA
ClosedPublic

Authored by Amanieu on Apr 27 2020, 7:55 AM.

Details

Summary

This allows unwinding to work across signal handler frames where the IP of the previous frame is not the same as the current value of the RA register. This is particularly useful for acquiring backtraces from signal handlers.

I kept the size of the context structure the same to avoid ABI breakage; the PC is stored in the previously unused slot for register 0.

Diff Detail

Event Timeline

Amanieu created this revision.Apr 27 2020, 7:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2020, 7:55 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript

Hmm, is there a specification for unwinding on RISC-V? Is this the same or different from what other unwinders (e.g. libgcc) do?

Amanieu updated this revision to Diff 260346.Apr 27 2020, 8:39 AM

The RISC-V unwinding spec is pretty bare: https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#dwarf

I had a look at libgcc, it tracks the PC separately from the other dwarf registers. The value modified by _Unwind_SetIP is separate from any of the values controlled by _Unwind_SetGR.

The way I am currently getting backtraces from a signal handler is to use unw_set_reg to initialize all the register from my sigcontext, and then using unw_set_reg(UNW_REG_IP) to set the PC. This only works if the PC is tracked separately from the return address register. This works fine on other targets (ARM, ARM64) where this is already the case.

The RISC-V unwinding spec is pretty bare: https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#dwarf

I had a look at libgcc, it tracks the PC separately from the other dwarf registers. The value modified by _Unwind_SetIP is separate from any of the values controlled by _Unwind_SetGR.

The way I am currently getting backtraces from a signal handler is to use unw_set_reg to initialize all the register from my sigcontext, and then using unw_set_reg(UNW_REG_IP) to set the PC. This only works if the PC is tracked separately from the return address register. This works fine on other targets (ARM, ARM64) where this is already the case.

Looking at this, it seems RISC-V is currently an exceptional case for tracking its pc in a GPR slot, and this change would bring RISC-V much closer to how other targets are defined (like ARM and AArch64, as you point out). I am still a little wary of doing so, as I'm not sure what libraries we need to maintain compatibility with.

Do you have a link to how libgcc implements this for risc-v? I couldn't find code when searching the repositories (either mainline gcc or riscv-gcc on github), but maybe I'm missing something.

GCC's implementation is in libgcc/unwind-dw2.c. The relevant code is here. The PC is tracked separately in the ra field, while the registers are in the reg array.

mhorne accepted this revision.May 4 2020, 4:18 PM

This looks fine to me based on your description of the issue and how the libgcc implementation achieves this. Someone else will need to sign off on this however.

I'll note that this implementation was upstreamed from FreeBSD, and at the moment we still allow occasional ABI breakage RISC-V where it is needed. I'm not aware of who else (if anyone) might be a consumer of this unwind implementation.

lenary added a subscriber: arichardson.

Tagging @arichardson for a review from the FreeBSD perspective.

Thanks for the link to the GCC implementation, that's helpful.

Tagging @arichardson for a review from the FreeBSD perspective.

Thanks for the link to the GCC implementation, that's helpful.

This change seems fine to me.

For our CHERI-RISCV version of libunwind I was considering using the zero register slot for a new register that we add (DDC). However, we already have to increase the size of the cursor to store our larger capability registers so we can just add another field instead.
Tagging @bsdjhb and @jrtc27 in case they have further comments.

lenary accepted this revision.May 28 2020, 11:31 AM

From the RISC-V side, I'm happy with this (having looked at how gcc's libunwind works). Any comments @bsdjhb and @jrtc27?

We have to wait for a review from #libunwind, I'm not sure to ensure this happens. They will also say whether to heed the clang-format suggested changes.

I think this is fine.

compnerd accepted this revision.Jun 9 2020, 9:09 AM

The context is not guaranteed to be stable across implementations, so this seems like a good way to avoid the ABI break.

This revision is now accepted and ready to land.Jun 9 2020, 9:09 AM

@Amanieu given you've now had a few commits approved successfully, do request LLVM committer access and land this yourself. There's details as to how to do so here: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

This revision was automatically updated to reflect the committed changes.