luismarques added a comment.

In D62732#2038309 <https://reviews.llvm.org/D62732#2038309>, @jrtc27 wrote:

> 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62732

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to