[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
----------------
kito-cheng wrote:
> kamleshbhalui wrote:
> > luismarques wrote:
> > > This doesn't seem correct.
> > OK so it appears that in principle we could have 4 combinations 
> > 
> > ILP32 + "f" : xlen = 32, flen = 32
> > ILP32 + "fd" : xlen = 32, flen = 64
> > LP64 + "f" : xlen = 64, flen = 32
> > LP64 + "fd" : xlen = 64, flen = 64
> > 
> > So instead of having fixed size area for each kind of register thought of having packed size.
> > please point me where it can cause problem?
> > 
> > 
> I am not family with libunwind and don't know what's _LIBUNWIND_CURSOR_SIZE , but I am curious about does here different value for xlen=32/flen=and xlen=64/flen=0?
> 
> And I think add an #else + #error to catch any unsupported value would be better, since the flen could be 128 (Q), although LLVM didn't implement yet, but in case we implemented in future we'll know we need fix here.
You are right that `_LIBUNWIND_CONTEXT_SIZE` is correctly defined.
Can you please add comments explaining the values for `_LIBUNWIND_CURSOR_SIZE`?
Personally, sometimes I also like to define constants as e.g. (32 + 32) instead of just 64, to make the structure behind the final value clear, so that may also be an option.


================
Comment at: libunwind/src/Registers.hpp:3764-3770
+#if __riscv_xlen == 64
   static_assert(sizeof(_registers) == 0x100,
                 "expected float registers to be at offset 256");
+#elif __riscv_xlen == 32
+static_assert(sizeof(_registers) == 0x80,
+                "expected float registers to be at offset 128");
+#endif
----------------
Since you're going for a generic approach, and you already define things like `RISCV_FOFFSET`, instead of using #if #elses here I guess you could check that `sizeof(_registers) == RISCV_FOFFSET*8`, or something like that. If you keep this approach then it's probably best to to add an #else branch to error out for other conditions (e.g. riscv128).


================
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
----------------
luismarques wrote:
> 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.
I see you changed `uintX_t` to be #defined as `unsigned long`. That was not the point of my previous comment. Is there an advantage in introducing these `X` types, instead of using existing types like, say, `size_t`?


================
Comment at: libunwind/src/UnwindRegistersRestore.S:1074
 
-#elif defined(__riscv) && __riscv_xlen == 64
+#elif defined(__riscv)
 
----------------
kamleshbhalui wrote:
> luismarques wrote:
> > 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.
> Since It is  already handled  at __libunwind_config.h:130
> do we really need it at other places?
I think you're right, everything here is generic so we don't need to error out here on unhandled cases, only in the code that sets the preprocessor definitions this code uses.


================
Comment at: libunwind/src/assembly.h:39
+#endif
+#if defined(__riscv_xlen)
+#if __riscv_xlen == 64
----------------
Isn't `__riscv_xlen` always defined when `__riscv` is defined? What's the point of this?



================
Comment at: libunwind/src/assembly.h:40-46
+#if __riscv_xlen == 64
+#define ILOAD ld
+#define ISTORE sd
+#elif __riscv_xlen == 32
+#define ILOAD lw
+#define ISTORE sw
+#endif
----------------
Better to put an #else branch here, to #error out on other cases (e.g. rv128).


================
Comment at: libunwind/src/assembly.h:56
+#define FSTORE fsw
+#endif
+#endif
----------------
Ditto #else, to #error out on other cases (e.g. quad-precision FP).


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

https://reviews.llvm.org/D80690