Page MenuHomePhabricator

[RISCV] Support libunwind for riscv32
Needs ReviewPublic

Authored by kamleshbhalui on May 27 2020, 11:08 PM.

Details

Reviewers
mhorne
lenary
luismarques
Group Reviewers
Restricted Project
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.

Diff Detail

Unit TestsFailed

TimeTest
330 mslinux > libunwind.libunwind::Unknown Unit Message ("")
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /mnt/disks/ssd0/agent/llvm-project/libunwind/test/frameheadercache_test.pass.cpp -v -DLIBUNWIND_NO_TIMER -funwind-tables -I/mnt/disks/ssd0/agent/llvm-project/libunwind/include -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/mnt/disks/ssd0/agent/llvm-project/libunwind/test/../../libcxx/test/support -Werror -Wall -Wextra -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -std=c++2a -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -L/mnt/disks/ssd0/agent/llvm-project/build/./lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/./lib -nodefaultlibs -lm -lpthread -lc -lunwind -ldl -lgcc -o /mnt/disks/ssd0/agent/llvm-project/build/projects/libunwind/test/Output/frameheadercache_test.pass.cpp.dir/t.tmp.exe

Event Timeline

kamleshbhalui created this revision.May 27 2020, 11:08 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 27 2020, 11:08 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
luismarques added inline comments.May 28 2020, 12:31 AM
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 marked an inline comment as done.May 28 2020, 12:41 AM
kamleshbhalui added inline comments.
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 marked an inline comment as done.May 28 2020, 12:42 AM
kamleshbhalui added inline comments.
libunwind/src/UnwindRegistersRestore.S
1084–1115

This is good idea.
thanks. will incorporate this.

kamleshbhalui marked an inline comment as done.May 28 2020, 1:00 AM
kamleshbhalui added inline comments.
libunwind/src/UnwindRegistersRestore.S
1074

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

asb added a subscriber: asb.May 28 2020, 5:41 AM
kamleshbhalui removed a reviewer: Restricted Project.

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

@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 added a reviewer: Restricted Project.May 28 2020, 10:11 AM

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

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

kamleshbhalui added 1 blocking reviewer(s): Restricted Project.May 28 2020, 10:48 AM
kito-cheng added inline comments.May 28 2020, 7:00 PM
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 added inline comments.May 30 2020, 3:56 AM
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 updated this revision to Diff 270442.EditedJun 12 2020, 10:10 AM

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

kamleshbhalui marked an inline comment as done.Jun 12 2020, 10:38 AM
kamleshbhalui added inline comments.
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 edited the summary of this revision. (Show Details)Jun 13 2020, 12:49 AM

@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?

thanks @mhorne for reviewing.
addressed your comments.

rebased and cleaned up the diff.

MaskRay added inline comments.
libunwind/src/Registers.hpp
3718

See above.

The style is:

# + spaces + if

mhorne added inline comments.Jun 22 2020, 5:45 AM
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

addressed @mhorne and @MaskRay concerns.

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 marked an inline comment as done.Jul 26 2020, 8:42 PM
kamleshbhalui marked 23 inline comments as done.Jul 26 2020, 9:31 PM

addressed @mhorne comments.

ldionne removed a subscriber: ldionne.Jul 27 2020, 6:07 AM
swpenim added a subscriber: swpenim.Aug 2 2020, 7:31 PM

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