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

[Mitchell Horne via Phabricator via libcxx-commits]

mhorne added a comment.

@kamleshbhalui thanks for this, it looks to be shaping up well. I have only a couple of small comments first.



================
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
----------------
kamleshbhalui wrote:
> luismarques wrote:
> > 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.
>  I have generated  values for _LIBUNWIND_CURSOR_SIZE  from UnwindCursor class size for each configration so that checkfit function becomes true.
> i.e.
>   static_assert((check_fit<UnwindCursor<A, R>, unw_cursor_t>::does_fit),
>                 "UnwindCursor<> does not fit in unw_cursor_t");
> I tried to make sense of these values for commenting , but i could not.
> 
> 
I think the purpose is that they want instances of UnwindCursor to be allocated on the stack, and it must fit within unw_cursor_t to do so.

As you have seen the calculation is: _UNWIND_CONTEXT_SIZE + (size of other  UnwindCursor members / sizeof(uint64_t))

For the 64-bit ISA this comes out to _UNWIND_CONTEXT_SIZE + 12, and _UNWIND_CONTEXT_SIZE + 7 for 32-bit.


================
Comment at: libunwind/include/__libunwind_config.h:144
+#    else
+#      define _LIBUNWIND_CONTEXT_SIZE (32 * (__riscv_xlen) / (64))
+#      if __riscv_xlen == 64
----------------
I don't think you need the extra parentheses in this statement.


================
Comment at: libunwind/src/Registers.hpp:3718
+#if defined(__riscv_flen)
+#if __riscv_flen == 64
+#define fp_t double
----------------
Could you please indent the nested `#if`s to improve readability?


================
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:
> 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`?
Yep, I would also like to see this changed to a typedef before going in. Doing this as a define is very unusual.


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

https://reviews.llvm.org/D80690