[libunwind][RISCV] Track PC separately from RA

[Amanieu]

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.

[compnerd]

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]

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.

[Amanieu]

Ping

[lenary]

In D78931#2005664, @Amanieu wrote:

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.

[Amanieu]

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]

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]

Tagging @arichardson for a review from the FreeBSD perspective.

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

[arichardson]

In D78931#2019950, @lenary wrote:

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.

[Amanieu]

Ping.

[lenary]

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.

[bsdjhb]

I think this is fine.

[compnerd]

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

[lenary]

@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

libunwind/src/Registers.hpp

public:
   void        setVectorRegister(int num, v128 value);
   static const char *getRegisterName(int num);
   void        jumpto();
   static int  lastDwarfRegNum() { return _LIBUNWIND_HIGHEST_DWARF_REGISTER_RISCV; }
   static int  getArch() { return REGISTERS_RISCV; }
 
   uint64_t  getSP() const         { return _registers[2]; }
   void      setSP(uint64_t value) { _registers[2] = value; }
-  uint64_t  getIP() const         { return _registers[1]; }
-  void      setIP(uint64_t value) { _registers[1] = value; }
+  uint64_t  getIP() const         { return _registers[0]; }
+  void      setIP(uint64_t value) { _registers[0] = value; }
 
 private:
-
+  // _registers[0] holds the pc
   uint64_t _registers[32];
   double   _floats[32];
 };
 
 inline Registers_riscv::Registers_riscv(const void *registers) {
   static_assert((check_fit<Registers_riscv, unw_context_t>::does_fit),
                 "riscv registers do not fit into unw_context_t");
   memcpy(&_registers, registers, sizeof(_registers));
if (regNum < 0)
     return false;
   if (regNum > UNW_RISCV_F31)
     return false;
   return true;
 }
 
 inline uint64_t Registers_riscv::getRegister(int regNum) const {
   if (regNum == UNW_REG_IP)
-    return _registers[1];
+    return _registers[0];
   if (regNum == UNW_REG_SP)
     return _registers[2];
   if (regNum == UNW_RISCV_X0)
     return 0;
   if ((regNum > 0) && (regNum < 32))
     return _registers[regNum];
   _LIBUNWIND_ABORT("unsupported riscv register");
 }
 
 inline void Registers_riscv::setRegister(int regNum, uint64_t value) {
   if (regNum == UNW_REG_IP)
-    _registers[1] = value;
+    _registers[0] = value;
   else if (regNum == UNW_REG_SP)
     _registers[2] = value;
   else if (regNum == UNW_RISCV_X0)
     /* x0 is hardwired to zero */
     return;
   else if ((regNum > 0) && (regNum < 32))
     _registers[regNum] = value;
   else

libunwind/src/UnwindRegistersRestore.S

#if defined(__riscv_flen) && __riscv_flen == 64
   fld    f27, (8 * 32 + 8 * 27)(a0)
   fld    f28, (8 * 32 + 8 * 28)(a0)
   fld    f29, (8 * 32 + 8 * 29)(a0)
   fld    f30, (8 * 32 + 8 * 30)(a0)
   fld    f31, (8 * 32 + 8 * 31)(a0)
 #endif
 
   // x0 is zero
-  ld    x1, (8 * 1)(a0)
+  ld    x1, (8 * 0)(a0) // restore pc into ra
   ld    x2, (8 * 2)(a0)
   ld    x3, (8 * 3)(a0)
   ld    x4, (8 * 4)(a0)
   ld    x5, (8 * 5)(a0)
   ld    x6, (8 * 6)(a0)
   ld    x7, (8 * 7)(a0)
   ld    x8, (8 * 8)(a0)
   ld    x9, (8 * 9)(a0)

libunwind/src/UnwindRegistersSave.S

 
 #
 # extern int __unw_getcontext(unw_context_t* thread_state)
 #
 # On entry:
 #  thread_state pointer is in a0
 #
 DEFINE_LIBUNWIND_FUNCTION(__unw_getcontext)
-  // x0 is zero
+  sd    x1, (8 * 0)(a0) // store ra as pc
   sd    x1, (8 * 1)(a0)
   sd    x2, (8 * 2)(a0)
   sd    x3, (8 * 3)(a0)
   sd    x4, (8 * 4)(a0)
   sd    x5, (8 * 5)(a0)
   sd    x6, (8 * 6)(a0)
   sd    x7, (8 * 7)(a0)
   sd    x8, (8 * 8)(a0)