[libcxx-commits] [PATCH] D80690: [RISCV] Support libunwind for riscv32

[Luís Marques via Phabricator via libcxx-commits]

luismarques added inline comments.


================
Comment at: libunwind/include/__libunwind_config.h:132
 #    define _LIBUNWIND_TARGET_RISCV 1
-#    define _LIBUNWIND_CONTEXT_SIZE 64
+#    define _LIBUNWIND_CONTEXT_SIZE (32 * (__riscv_xlen + __riscv_flen) / 64)
 #    define _LIBUNWIND_CURSOR_SIZE 76
----------------
This doesn't seem correct.


================
Comment at: libunwind/src/Registers.hpp:3717-3721
+#if __riscv_xlen == 64
+#define uintX_t uint64_t
+#elif __riscv_xlen == 32
+#define uintX_t uint32_t
+#endif
----------------
If we go the generic route, why not just use `unsigned long` (or `size_t`, etc.) instead of these custom types? Also, these uncapitalized `*_t` names are generally typedefs, not preprocessor defines.


================
Comment at: libunwind/src/Registers.hpp:3962-3963
 #else
-  (void)regNum;
-  (void)value;
   _LIBUNWIND_ABORT("libunwind not built with float support");
----------------
In the #else "branch" those variables aren't used, so presumably you still need those lines to avoid warnings about unused variables.


================
Comment at: libunwind/src/UnwindRegistersRestore.S:1074
 
-#elif defined(__riscv) && __riscv_xlen == 64
+#elif defined(__riscv)
 
----------------
It might be a good idea to use instead `__riscv_xlen == 32 || __riscv_xlen == 64`, as that will correctly detect the case that riscv128 isn't handled.


================
Comment at: libunwind/src/UnwindRegistersRestore.S:1085-1116
+  fload    f0, (RISCV_FOFFSET + RISCV_FSIZE * 0)(a0)
+  fload    f1, (RISCV_FOFFSET + RISCV_FSIZE * 1)(a0)
+  fload    f2, (RISCV_FOFFSET + RISCV_FSIZE * 2)(a0)
+  fload    f3, (RISCV_FOFFSET + RISCV_FSIZE * 3)(a0)
+  fload    f4, (RISCV_FOFFSET + RISCV_FSIZE * 4)(a0)
+  fload    f5, (RISCV_FOFFSET + RISCV_FSIZE * 5)(a0)
+  fload    f6, (RISCV_FOFFSET + RISCV_FSIZE * 6)(a0)
----------------
If we are going to use a preprocessed instruction name (which I'm not sure we should) then at least the name should be ALL CAPS to make it obvious that it's a preprocessor definition, not a regular instruction.


================
Comment at: libunwind/src/UnwindRegistersSave.S:1024
 
-#elif defined(__riscv) && __riscv_xlen == 64
+#elif defined(__riscv)
 
----------------
ditto `32 || 64`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80690/new/

https://reviews.llvm.org/D80690