[RISCV] Support libunwind for riscv32

[kamleshbhalui]

Summary

This patch adds support in libunwind for rv32 hard flaot and soft-float for both rv32 and rv64.
Tried to generalise as much possible with lesser code duplication.

[luismarques]

libunwind/include/__libunwind_config.h

129–130

This doesn't seem correct.

libunwind/src/Registers.hpp

3717–3721

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.

3962–3963

In the #else "branch" those variables aren't used, so presumably you still need those lines to avoid warnings about unused variables.

libunwind/src/UnwindRegistersRestore.S

1074

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.

1084–1115

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.

libunwind/src/UnwindRegistersSave.S

1024

ditto 32 || 64.

[kamleshbhalui]

libunwind/include/__libunwind_config.h

129–130

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?

[kamleshbhalui]

libunwind/src/UnwindRegistersRestore.S

1084–1115

This is good idea.
thanks. will incorporate this.

[kamleshbhalui]

libunwind/src/UnwindRegistersRestore.S

1074

Since It is already handled at __libunwind_config.h:130
do we really need it at other places?

[kamleshbhalui]

addressed @luismarques comments.

[kamleshbhalui]

defined _LIBUNWIND_CURSOR_SIZE
for all these below variants
ILP32 + "f" : xlen = 32, flen = 32 ===> 39
ILP32 + "fd" : xlen = 32, flen = 64 ===>55
LP64 + "f" : xlen = 64, flen = 32 ====>60
LP64 + "fd" : xlen = 64, flen = 64 ====>76 (was already done.)

[ldionne]

@kamleshbhalui Just to be clear -- you need libunwind approval before you can submit a patch that touches libunwind. If you just wanted to iterate on this privately it's OK, but you'll need approval from the group before submitting.

[kamleshbhalui]

In D80690#2060505, @ldionne wrote:

@kamleshbhalui Just to be clear -- you need libunwind approval before you can submit a patch that touches libunwind. If you just wanted to iterate on this privately it's OK, but you'll need approval from the group before submitting.

Thanks @ldionne , wasn't aware of this. added the group as reviewer.

[ldionne]

In D80690#2060681, @kamleshbhalui wrote:

In D80690#2060505, @ldionne wrote:

@kamleshbhalui Just to be clear -- you need libunwind approval before you can submit a patch that touches libunwind. If you just wanted to iterate on this privately it's OK, but you'll need approval from the group before submitting.

Thanks @ldionne , wasn't aware of this. added the group as reviewer.

That's the reason why libunwind is added as a blocking reviewer to reviews that touch libunwind. At least one maintainer (or person familiar with the code base) needs to give you a thumbs up.

Changes in these runtimes components have very wide impact, and unfortunately they're usually not very well tested. That's why we have these requirements.

[kito-cheng]

libunwind/include/__libunwind_config.h

129–130

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.

[luismarques]

libunwind/include/__libunwind_config.h

129–130

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.

libunwind/src/Registers.hpp

3717–3721

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?

3775–3781

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).

libunwind/src/UnwindRegistersRestore.S

1074

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.

libunwind/src/assembly.h

39

Isn't __riscv_xlen always defined when __riscv is defined? What's the point of this?

40–46

Better to put an #else branch here, to #error out on other cases (e.g. rv128).

56

Ditto #else, to #error out on other cases (e.g. quad-precision FP).

[kamleshbhalui]

addressed @luismarques concerns.
This patch extends soft float case .

[kamleshbhalui]

libunwind/include/__libunwind_config.h

129–130

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.

[kamleshbhalui]

Rebased

[kamleshbhalui]

ping?

[mhorne]

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

libunwind/include/__libunwind_config.h

129–130

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.

142

I don't think you need the extra parentheses in this statement.

libunwind/src/Registers.hpp

3717–3721

Yep, I would also like to see this changed to a typedef before going in. Doing this as a define is very unusual.

3718

Could you please indent the nested #ifs to improve readability?

[kamleshbhalui]

thanks @mhorne for reviewing.
addressed your comments.

[kamleshbhalui]

rebased and cleaned up the diff.

[MaskRay]

libunwind/src/Registers.hpp

3718

See above.

The style is:

# + spaces + if

[mhorne]

libunwind/src/Registers.hpp

3727

Only the preprocessor directives need indentation, not the typedef.

3811

I don't think size_t is an appropriate type for this, although it would likely work in all cases. I think what you want is either an unsigned long or something like the following:

#if __riscv_xlen == 64
typedef uint64_t reg_t;
#elif __riscv_xlen == 32
typedef uint32_t reg_t;
#endif

[kamleshbhalui]

addressed @mhorne and @MaskRay concerns.

[kamleshbhalui]

ping?

[kamleshbhalui]

ping?

[mhorne]

Hi, sorry for the delay in review. It would be helpful if you could go through the inline comments and mark them as "done" if they have been addressed. I see there are still a few comments that have been ignored.

This patch is tricky because there are no consistent indentation rules for preprocessor directives. In general, I think the indentation is only necessary for blocks where it increases readability (such as where _LIBUNWIND_CURSOR_SIZE is defined, or the block where reg_t is defined). Otherwise, it's best to follow the existing convention of the file.

libunwind/include/__libunwind_config.h

136–145

Please look at this block again carefully. It should follow a rule of 1 space of indentation for each level of nesting.

libunwind/src/Registers.hpp

3727

Again, please fix these.

3984

(void)regNum;

libunwind/src/assembly.h

41

Everything inside this #if should have an additional space of indentation.

[kamleshbhalui]

addressed @mhorne comments.

[luismarques]

This seems roughly OK now. Does anyone with libunwind experience want to give it a thumbs up?
#libunwind people, does this LGTY?

libunwind/src/Registers.hpp

3734

Nit: supresing -> suppressing

[luismarques]

In D80690#2278727, @luismarques wrote:

This seems roughly OK now. Does anyone with libunwind experience want to give it a thumbs up?
#libunwind people, does this LGTY?

ping.

[luismarques]

In D80690#2375624, @luismarques wrote:

In D80690#2278727, @luismarques wrote:

This seems roughly OK now. Does anyone with libunwind experience want to give it a thumbs up?
#libunwind people, does this LGTY?

ping.

ping?

[kamleshbhalui]

rebased with the main branch. do not see any conflict.
@ldionne or @abdulras can you please have a look at this, this patch is in this state for a long time.
FYI, I have been using this patch in VxWorks.

[kamleshbhalui]

fixed a nit.

[compnerd]

This is fine for now, but in the future, for such changes, please split this up into a series of patches. The clean ups for the named constants and clean up for floating point handling could have been separate changes which would have reduced the overall size of the changes and focused the patch specifically to adding support for rv32.

[bkramer]

This breaks building libunwind with LIBUNWIND_ENABLE_CROSS_UNWINDING enabled

[• thakis]

Hi,

this doesn't compile on our bots:

FAILED: obj/buildtools/third_party/libunwind/libunwind/libunwind.o 
/b/s/w/ir/cache/goma/client/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/buildtools/third_party/libunwind/libunwind/libunwind.o.d -D_LIBUNWIND_DISABLE_VISIBILITY_ANNOTATIONS -DDCHECK_ALWAYS_ON=1 -DUSE_AURA=1 -DUSE_OZONE=1 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -DFUCHSIA_SDK_VERSION=2.20210302.1.1 -DCR_CLANG_REVISION=\"llvmorg-13-init-1559-g01b87444-3\" -D_LIBCPP_ABI_UNSTABLE -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_ENABLE_NODISCARD -D_LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS -D_LIBCPP_NO_NATIVE_SEMAPHORES -D_LIBCPP_DEBUG=0 -DCR_LIBCXX_REVISION=8fa87946779682841e21e2da977eccfb6cb3bded -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../../buildtools/third_party/libunwind/trunk/include -I../.. -Igen -fno-delete-null-pointer-checks -fno-ident -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -funwind-tables -fPIC -fcolor-diagnostics -fmerge-all-constants -fcrash-diagnostics-dir=../../tools/clang/crashreports -mllvm -instcombine-lower-dbg-declare=0 -fcomplete-member-pointers --target=x86_64-fuchsia -m64 -march=x86-64 -msse3 -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -Xclang -fdebug-compilation-dir -Xclang . -no-canonical-prefixes -Oz -fdata-sections -ffunction-sections -fomit-frame-pointer -g1 -ftrivial-auto-var-init=pattern -fvisibility=hidden -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang check-ipc -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -Werror -Wall -Wno-unused-variable -Wno-misleading-indentation -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-unneeded-internal-declaration -Wno-undefined-var-template -Wno-psabi -Wno-ignored-pragma-optimize -Wno-implicit-int-float-conversion -Wno-final-dtor-non-final-class -Wno-builtin-assume-aligned-alignment -Wno-deprecated-copy -Wno-non-c-typedef-for-linkage -Wno-max-tokens -fstrict-aliasing -fPIC -Wno-unused-function -funwind-tables -std=c++14 -fno-trigraphs -Wno-trigraphs -nostdinc++ -isystem../../buildtools/third_party/libc++/trunk/include -isystem../../buildtools/third_party/libc++abi/trunk/include --sysroot=../../third_party/fuchsia-sdk/sdk/arch/x64/sysroot -fvisibility-inlines-hidden -fexceptions -frtti -c ../../buildtools/third_party/libunwind/trunk/src/libunwind.cpp -o obj/buildtools/third_party/libunwind/libunwind/libunwind.o
In file included from ../../buildtools/third_party/libunwind/trunk/src/libunwind.cpp:21:
In file included from ../../buildtools/third_party/libunwind/trunk/src/AddressSpace.hpp:23:
In file included from ../../buildtools/third_party/libunwind/trunk/src/EHHeaderParser.hpp:17:
In file included from ../../buildtools/third_party/libunwind/trunk/src/DwarfParser.hpp:22:
../../buildtools/third_party/libunwind/trunk/src/Registers.hpp:3739:4: error: "Unsupported __riscv_xlen"
#  error "Unsupported __riscv_xlen"
   ^

Does this assume that we pass any -D flags? Does this assume trunk clang?

[• thakis]

libunwind/src/Registers.hpp

3722

I think you can't assume that __riscv stuff is defined down here. This is in a __LIBUNWIND_TARGET_RISCV block, which, if _LIBUNWIND_IS_NATIVE_ONLY is not defined (which it isn't in LIBUNWIND_ENABLE_CROSS_UNWINDING builds), is defined to 1.