Differential D78931
[libunwind][RISCV] Track PC separately from RA Authored by Amanieu on Apr 27 2020, 7:55 AM.
Details
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 TimelineComment Actions Hmm, is there a specification for unwinding on RISC-V? Is this the same or different from what other unwinders (e.g. libgcc) do? Comment Actions 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. Comment Actions 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. Comment Actions 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. Comment Actions 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. Comment Actions Tagging @arichardson for a review from the FreeBSD perspective. Thanks for the link to the GCC implementation, that's helpful. Comment Actions 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. Comment Actions The context is not guaranteed to be stable across implementations, so this seems like a good way to avoid the ABI break. Comment Actions @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 |
clang-format: please reformat the code
- uint64_t getIP() const { return _registers[0]; } - void setIP(uint64_t value) { _registers[0] = value; } + uint64_t getIP() const { return _registers[0]; } + void setIP(uint64_t value) { _registers[0] = value; }