Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add watchpoints and CSR support to GDBStub #14

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

SamuelWAnderson45
Copy link
Contributor

@SamuelWAnderson45 SamuelWAnderson45 commented Oct 2, 2022

This PR adds support for GDB watchpoints to Sedna's GDB stub. It also adds support for floating point registers and CSRs. That even includes the Sedna specific 0xbc0 CSR for switching to 32 bit mode.

Implementation notes:
The watchpoints are stored and looked up from a simple array. It should have very low overhead. (Also this is what QEMU does.) If we wanted to support having lots of watchpoints with lookups faster than O(n), we would need an Interval Tree. I partially implemented one, but I don't think it's worth the effort, considering it will perform worse due to overhead with real world usage.

Also there are various quirks with 32 bit specific registers. In general, GDB doesn't support switching CPU modes so we just have to pretend the CPU is always in 64 bit mode

Calculating the size (as well as isEmpty) on a subset of a SortedSet is a somewhat expensive operation that involves walking the tree of the parent set (since a subset object is merely a thin wrapper around the full set). It is not O(1) like you might expect.

This removes the isEmpty() call, so we walk the tree one fewer times.
Adds support for specifying an XML target description.
Adds support for the p/P packets to read/write a single register.
These combined allow GDB to read/write floating point and csr registers.
@SamuelWAnderson45
Copy link
Contributor Author

I meant to squash those commits, oh well

Also I had a couple questions

  1. How do you normally debug R5CPUTemplate? I can't place breakpoints in it because the actual class is generated at Runtime. Very frustrating
  2. Not really a question but just a headsup. It looks like CI can't auth against maven which is interesting. I've never been able to get the GitHub maven to work for me either, even with a Github token. I've always rebuilt Sedna's dependencies locally. So not sure what's up with that

@fnuecke
Copy link
Owner

fnuecke commented Oct 2, 2022

Very very cool, will have a closer look soon!

  1. Yes, it is super annoying -- I've used a hack of calling a temporary Utils.DebugBreakpoint() method which has a breakpoint where needed.
    I plan on switching to dev-time generation of the interpreter instead of runtime sometime. I.e. pulling out the instruction file parsing plus decoder switch gen into a separate app/main() and run that before compiling sedna itself, generating source code instead of bytecode.
    As for why it's generated at runtime at all, that's a bit of a relic from the time where I also tried to generate "traces" (I think I got that idea from QEMU?), i.e. when jumping, generate actual Java Bytecode from all following instructions until the next jump (and store it in an address->generated method lookup), then call that generated Java method instead of one-by-one interpreting the instructions. However, after optimizing the interpreter switch, I couldn't really get this fast enough to be worth it (mostly due to the virtcall overhead, IIRC). The only way I currently see this approach outperforming the interpreter switch is to translate the whole executable code block in one go (to enable direct method calls), as a massive switch to allow yielding/pausing, and regenerating that when something writes to the related memory areas, but... not convinced that'd be worth it/actually be faster (due to it having to be able to pause).
  2. Ah, I forgot about that again... it only works when there was a recent other run so the caches exist. And Github prevents actions triggered by PRs to access secrets (which by default is reasonable) but also doesn't allow overriding this for specific secrets/specific PRs (which is annoying). And sadly their maven still needs credentials (which is super annoying). Someone pointed out it'd be nice to host sedna (+ceres) on maven central, but I haven't had the time to jump through all the hoops to be able to publish to that. Suggestions welcome.

@SamuelWAnderson45
Copy link
Contributor Author

SamuelWAnderson45 commented Oct 2, 2022

Ah that's why it's called interpretTrace32/64. I really like the idea, basically a JIT. Too bad it didn't work out. Build time generation would be nice

Perhaps its worth pulling interpretTrace and the decoder out of R5CPUTemplate into it's own class, so everything else in R5CPUTemplate can be debugged easily.

Copy link
Owner

@fnuecke fnuecke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Sorry for the wait, I had finally caught the corona, and had to catch up on a couple of other things first, after that. Just a few minor things I've commented on.

I'm hoping to do some rough perf tests next week (currently on mobile internet with data limit because my ISP is a derp, so don't want to let gradle loose).

src/main/java/li/cil/sedna/riscv/R5CPUTemplate.java Outdated Show resolved Hide resolved
src/main/resources/gdb/target.xml Outdated Show resolved Hide resolved
src/main/java/li/cil/sedna/gdbstub/GDBStub.java Outdated Show resolved Hide resolved
@fnuecke
Copy link
Owner

fnuecke commented Nov 18, 2022

I'll give it a spin this weekend, and unless I stumble upon something I'm not sure how to fix, I'll merge.

Thanks again for your work on the debugger front, I really appreciate it a lot!

@kristibektashi
Copy link

I'll give it a spin this weekend, and unless I stumble upon something I'm not sure how to fix, I'll merge.

Thanks again for your work on the debugger front, I really appreciate it a lot!

Yet this was never merged

Co-authored-by: ja2142 <radoslaw.olko@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants