Page MenuHomePhabricator

[RISCV] Add SystemV ABI
Needs ReviewPublic

Authored by simoncook on May 31 2019, 7:06 AM.

Details

Summary

This implements the SystemV ABI for RISC-V for LLDB.

Diff Detail

Event Timeline

simoncook created this revision.May 31 2019, 7:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2019, 7:06 AM
mhorne added a subscriber: mhorne.Jun 1 2019, 10:55 AM

You are following all of the patterns for all of the architectures. It would be nice for us to cleanup DynamicRegisterInfo.cpp, Platform.cpp, and Thread.cpp eventually so we don't need to modify these files when a few arch is added by abstracting things into lldb_private::Architecture, but that is beyond the scope of this change.

lldb/source/Plugins/ABI/SysV-riscv/ABISysV_riscv.cpp
42

No need to fill in the "eRegisterKindProcessPlugin" field of each register info. That is designed to be the number that any process plug-in (probably ProcessGDBRemote.cpp) fills in, so best to not fill this in. The orders are: EH frame, DWARF, generic, process plug-in and then LLDB. So I would change all of these to:

{0, 0, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, 0},
lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
624–635 ↗(On Diff #202428)

Seems like you filled all of this info in the registers defs already in ABISysV_riscv.cpp? Do we need this here? Some platforms weren't setting these correctly and this code was meant to correct that.

lldb/source/Target/Platform.cpp
1904–1912

This would be great to factor out into lldb_private::Architecture eventually. You are correctly following the patterns that are in LLDB. I will see if I can get this to happen soon so we don't run into these issues again.

lldb/source/Target/Thread.cpp
2048–2074

Seems like the original logic before your mods is not correct. Not sure the default case ever gets used here. The arch of x86_64, x86, arm, aarch64 or thumb should all be caught before we get to the default statement for apple targets. I will add Jason Molenda to the change to get a comment on this.

Jason, can you take a look at "Thread.cpp" and my inlined comment?

xiaobai added subscribers: compnerd, xiaobai.

@compnerd You might be interested in this.

jasonmolenda added inline comments.Jun 3 2019, 4:27 PM
lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
624–635 ↗(On Diff #202428)

There's AugmentRegisterInfoViaABI() over in ProcessGDBRemote which does fill in eh_frame, dwarf, and generic register numbers based on the ABI's register table. It seems like that should be sufficient - although I see a bunch of other architectures doing the exact same thing here in DynamicRegisterInfo::AddRegister.

lldb/source/Target/Thread.cpp
2048–2074

The default case shouldn't ever be taken - it was the unwinder we used (I think you wrote it during bringup) before UnwindLLDB existed. I think we've kept it around in case people need an initial unwinder during bringup of new architectures, but maybe it's not serving any purpose at all any more.

simoncook updated this revision to Diff 205287.Jun 18 2019, 2:49 AM
  • Refactored register tables to match style used in i386/x86_64
  • Add enum for RISC-V DWARF numbers
  • Add F registers (assuming 32-bit, at runtime this seems to be overwritten to 64-bit if D extension is provided)
  • Add default unwind plan for first frame
simoncook retitled this revision from [WIP][RISCV] Initial port of LLDB for RISC-V to [RISCV] Add SystemV ABI.
simoncook edited the summary of this revision. (Show Details)

Rebase, implement all hooks that aren't PrepareTrivialCall/function calling related. If its possible to commit these two separately, I think it would be best to have that as a separate patch whereby preparing arguments for the various RISC-V hard-float ABIs can be done independently of breakpoints/unwinding/etc.

echristo edited reviewers, added: labath; removed: espindola.Nov 4 2019, 2:12 PM
abidh added a subscriber: abidh.Nov 7 2019, 3:53 AM

So, if I understand correctly, this patch doesn't just add an "ABI" plugin (for which we have in the past agreed that we don't know of a better way to test than just running "TestReturnValue" on the given architecture), but it actually adds all the bits and pieces necessary to support risc-v debugging, *sans* an actual debug server. This means that one cannot test this code, even if he happened to have risc-v hardware around. I'm not really sure what's our position on that, which is why it took me so long to write this comment, and why I am reluctant to hit accept (even though the code itself seems pretty good).

I take it you have some proprietary/downstream debug server/hardware probe that you're using this with? And you're not putting that stuff in lldb-server because lldb-server is too heavy for your use case ?