Page MenuHomePhabricator

[RISCV] Add SystemV ABI
Needs ReviewPublic

Authored by luismarques 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
41 ↗(On Diff #202428)

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
1970–1978

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
2060–2084 ↗(On Diff #202428)

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
2060–2084 ↗(On Diff #202428)

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 ?

Yeah, I don't think we want to be merging code we can't test even in a non-automated way. Even if this code is completely bug-free, the inability to test it just means we risk having it bit-rot with nobody noticing.

Yeah, I don't think we want to be merging code we can't test even in a non-automated way. Even if this code is completely bug-free, the inability to test it just means we risk having it bit-rot with nobody noticing.

We now have two open-source debug servers that we can use to test this patch: gdbserver and QEMU's gdbstub, both of which now have RISC-V support. I rebased this patch and tested it with both.
My initial tests were with QEMU/gdbstub, and I was quite pleased with the results. I could set breakpoints, continue execution, step line-by-line and instruction-by-instruction and print registers (GPRs and CSRs). The backtrace was only showing the current frame. Still, I would say that LLDB was in a good enough state to be useful. Next, I tested it against gdbserver (compiled from git main) running in a Fedora RISC-V machine. Setting a breakpoint would seemingly succeed (e.g. it reported success, with the correct code address), but the code doesn't actually trap. You can still break with Ctrl-C though, and print registers and so on. So there seem to be some additional issues, but I also had problems when connecting less recent versions of GDB to that debug server, so we might just need to do some adjustments to match the GDB changes.

It seems that, once rebased, this patch is in pretty good shape. lowRISC is keen to see LLDB get RISC-V support, and I'm now ramping up work to help with that. Given the great work that we already have in this patch, I think an important first step would be to land it. None of the issues I encountered were obviously due to the code in this patch (and the things still marked as TODO), so I don't see any major downside to merging it. I will do a more in-depth investigation of the issues I encountered, but that shouldn't be a blocker to this patch. Landing this contribution would make it easier for others to contribute the missing pieces, and fixes for the issues, without having to submit duplicate work.

If it helps, feel free to use this commit [1], it's pretty much just a rebase of this patch.
@simoncook please let me know if you plan to update this patch and get it approved and landed (LGTM, once rebased), or other ways in which it would make sense to coordinate our work.

[1] https://github.com/luismarques/llvm-project/commit/4a0cccb0cfa8cb5c59c8803077a76751498447ec

Thanks for looking at this @luismarques We had planned to put more effort into this patch, but time got in the way for quite a lot, but I'm glad it's working; thanks for the rebase I'll update this to match shortly. And it's good that it looks like it's mostly working. I'm curious about your backtrace showing one frame, is that something without debug information, since the example I was using when writing this did show a backtrace back to main? It would be good to understand why that disn't produce the expected output.

As for next steps, if we're happy with the state then I think this should land (assuming qemu is sufficient given it is public), and then we can flesh out other bits which give a better experience. I'm not sure how to connect this to any automated testing, or where to document any way of checking this manually, the state of that isn't quite clear, so any clarity there helps.

Beyond this I think the next stage is implementing the parts for calling functions within a target, which if you could help with that would be great. I see that as a follow up patch to this, I don't see the two necessarily having to land together, since this part enables a useful debugging experience already.

As for next steps, if we're happy with the state then I think this should land (assuming qemu is sufficient given it is public), and then we can flesh out other bits which give a better experience. I'm not sure how to connect this to any automated testing, or where to document any way of checking this manually, the state of that isn't quite clear, so any clarity there helps.

Yeah, I think that after the rebase this is nearly ready to land. The only additional suggestions I have at this point are:

  • We should probably follow the convention and put the registers in Plugins/Process/Utility/RegisterInfos_riscv.h, like is done for other archs. If that isn't trivial I guess it could be a follow-up patch.
  • Review the list of callee-saved registers. Aren't x25 and x26 missing?
  • Nit: there's a typo in this patch that I missed in my rebased commit and should be corrected: "poinrter".
  • I had renamed the RISCVflags members, if you use my rebased commit please check if you agree with that alternative.

I'm curious about your backtrace showing one frame, is that something without debug information, since the example I was using when writing this did show a backtrace back to main? It would be good to understand why that disn't produce the expected output.

It is with debug information. I had been looking at other issues, but I'm going to look into this and I'll let you know what I find out.

Beyond this I think the next stage is implementing the parts for calling functions within a target, which if you could help with that would be great. I see that as a follow up patch to this, I don't see the two necessarily having to land together, since this part enables a useful debugging experience already.

I agree, we can land this and provide follow-up patches. For my next steps I was looking into fixing the issues I was experiencing, if possible, and then implementing those TODOs. One of the issues I've diagnosed is that we need to add extra logic to handle RV32 vs RV64 based on the ELF header, like is done for MIPS, otherwise it incorrectly assumes RV32 when it should be RV64. I'll provide a follow-up patch.

Thanks for the feedback and the patch @simoncook!

luismarques commandeered this revision.Thu, Sep 3, 5:42 AM
luismarques added a reviewer: simoncook.

Commandeering the patch, as discussed in the last RISC-V sync-up call. I'll update it with only minor changes; further development can be done in follow-up patches.

  • Fix list of callee saved registers;
  • Fix typo;
  • Fix trivial clang-format issue.

@labath @jrtc27 @clayborg Now that we have at least 3 open-source debug servers that we can use to test this with (OpenOCD, QEMU gdbstub, gdbserver) perhaps this can be merged? I had very good results using this patch with OpenOCD. This patch doesn't include automated tests, but I'm not sure what tests would be required for this patch, or that it makes sense to require them at this point. I'll be doing more work for LLDB RISC-V support, and I'll provide tests for specific fixes going forward.

@luismarques , what's the recommended gdb-server implementation you recommend for me to try this on a riscv machine?

@luismarques , what's the recommended gdb-server implementation you recommend for me to try this on a riscv machine?

I compiled mine from the mainline binutils-gdb git repository (running on Fedora riscv64). But I suggest that you used OpenOCD instead, as I think that RISC-V gdbserver had some issues. If I recall correctly, gdbserver wouldn't return from the c command even if it hit a breakpoint. GDB was using vCont instead so it didn't run into that problem. Anyway, that's in my TODO list to better reduce the issue and report bugs/send patches, but just to let you know.

@labath @jrtc27 @clayborg Now that we have at least 3 open-source debug servers that we can use to test this with (OpenOCD, QEMU gdbstub, gdbserver) perhaps this can be merged? I had very good results using this patch with OpenOCD. This patch doesn't include automated tests, but I'm not sure what tests would be required for this patch, or that it makes sense to require them at this point. I'll be doing more work for LLDB RISC-V support, and I'll provide tests for specific fixes going forward.

Sounds reasonable to me. Maybe it's still to early for that but have you tried running (part of) the test suite under QEMU yet? It should give you a pretty good idea of the state of things and gives you a bunch of test coverage for free. You'll probably want to consider setting up a bot for that down the road anyway.

Sounds reasonable to me. Maybe it's still to early for that but have you tried running (part of) the test suite under QEMU yet? It should give you a pretty good idea of the state of things and gives you a bunch of test coverage for free. You'll probably want to consider setting up a bot for that down the road anyway.

I haven't tried that yet. When I did try to build LLVM with LLDB support under a QEMU VM (Fedora RISC-V) it failed because CreateHostNativeRegisterContextLinux didn't yet have a RISC-V implementation. I imagine that it might be possible to run part of the tests without solving that issue, but I added that to my TODO list anyway. I'll be able to devote more time to LLDB work around the beginning of October.