[Lldb-commits] [PATCH] D62732: [RISCV] Add SystemV ABI

[Luís Marques via Phabricator via lldb-commits]

luismarques added a comment.

In D62732#2193729 <https://reviews.llvm.org/D62732#2193729>, @simoncook wrote:

> 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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62732