lenary added a comment.
Nice! Thanks for adding support for RISC-V. I like the use of the ABI register
names rather than the numeric names.
I have a few queries/nits, below.
================
Comment at: libunwind/include/libunwind.h:835
+// 64-bit RISC-V registers
+enum {
----------------
Please include a link to the RISC-V ELF psABI spec which defines these register
numbers.
================
Comment at: libunwind/src/Registers.hpp:3553
+
+ GPRs _registers;
+ double _floats[32];
----------------
Why is this a special GPR struct type, but the FPRs are not?
================
Comment at: libunwind/src/Registers.hpp:3585
+
+inline uint64_t Registers_riscv::getRegister(int regNum) const {
+ if (regNum == UNW_REG_IP)
----------------
Do you want to include an override in this function for `regNum ==
UNW_RISCV_X0` to always return zero?
The reason I ask is because I worry that
`Registers_riscv::Registers_riscv(const void *registers)` could initialise a
non-zero value into `_registers.__x[0]`, which could lead to really confusing
bugs.
It might also make sense to include `assert(validRegister(regNum));` in both
this function and `setRegister` as you've done with the float registers.
================
Comment at: libunwind/src/UnwindCursor.hpp:999
+#if defined (_LIBUNWIND_TARGET_RISCV)
+ int stepWithCompactEncoding(Registers_riscv &) {
+ return UNW_EINVAL;
----------------
Nit: This should be formatted like the version for `Registers_sparc` above.
================
Comment at: libunwind/src/UnwindCursor.hpp:1071
+#if defined (_LIBUNWIND_TARGET_RISCV)
+ bool compactSaysUseDwarf(Registers_riscv &, uint32_t *) const {
+ return true;
----------------
Nit: This should be formatted like the version for `Registers_sparc` above.
================
Comment at: libunwind/src/UnwindCursor.hpp:1143
+#if defined (_LIBUNWIND_TARGET_RISCV)
+ compact_unwind_encoding_t dwarfEncoding(Registers_riscv &) const {
+ return 0;
----------------
Nit: This should be formatted like the version for `Registers_sparc` above.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68362/new/
https://reviews.llvm.org/D68362
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
