mhorne marked 5 inline comments as done.
mhorne added a comment.
Thanks for the review!
================
Comment at: libunwind/src/Registers.hpp:3585
+
+inline uint64_t Registers_riscv::getRegister(int regNum) const {
+ if (regNum == UNW_REG_IP)
----------------
lenary wrote:
> 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.
The assert is not necessary, since `_LIBUNWIND_ABORT` is called in the case of
an invalid register number.
================
Comment at: libunwind/src/UnwindCursor.hpp:999
+#if defined (_LIBUNWIND_TARGET_RISCV)
+ int stepWithCompactEncoding(Registers_riscv &) {
+ return UNW_EINVAL;
----------------
lenary wrote:
> Nit: This should be formatted like the version for `Registers_sparc` above.
`Registers_sparc` is the outlier in this case, I've formatted it as all the
other architectures use. If you'd like I could fix the SPARC formatting in this
patch.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68362/new/
https://reviews.llvm.org/D68362
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
