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

Rework the entire logic of the GDB stub #10142

Merged
merged 13 commits into from Oct 23, 2021

Conversation

aldelaro5
Copy link
Member

The GDB stub is an old and very unmaintained feature that can be very useful when paired with reversing tools like ghidra, but being unmaintained, it didn't worked very well (and only worked on the interpreter).

This PR does the ground work to rework it entirely to at least a usable state. Here's a summary of the changes:

  • The stub process commands logic now runs every so often while the emulation is running to process potential ctrl+c commands
  • The main gdb stalling logic is now in the CPU::Run instead of the interpreter whenever gdb still has control (basically, after a ctrl+c or breakpoint, but not user pause). This effectively means all CPU backend are now supported.
  • If active, boot to pause is effectively forced until we get a connection and a continue command (this behavior may be changed in another PR, but the focus of this one is to just make it work).
  • Completely rework the breakpoint processing logic to use the same functions than the integrated debugger instead of tracking them separately since the integrated debugger already works very well
  • Add support for the T command that checks if a thread is alive (this is something ghidra wanted for mgba and it's trivial to support so might as well)
  • Allow the stop button to kill the gdb connexion with it
  • Remove the cmake option and the ifdefs, the code is now always enabled
  • Rework the logs (upgraded some debug to info and added one when we break due to GDB)
  • HUGE refactor of the code as it was far from respecting the current coding style

To note: the hardest part was figuring out how to coordinate this with the core safely: I am not experienced with multithreading stuff so when reviewing, I recommend to pay close attention to how this interacts with the core and to make sure there's no race or anything unsafe I am doing here. This pr is mostly to make sure the approach done here is correct for potential further improvements.

For now, I can vouch for a gdb shell to work (mostly tested on the one provided by devkitppc, but gdb-multiarch seemed to work well), but ghidra seems to have issues of its own that doesn't seem to be in the scope of Dolphin.

Source/Core/Core/PowerPC/GDBStub.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/GDBStub.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/GDBStub.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/GDBStub.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/GDBStub.h Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/GDBStub.h Outdated Show resolved Hide resolved
@Warepire
Copy link

Warepire commented Oct 1, 2021

  • If active, boot to pause is effectively forced until we get a connection and a continue command (this behavior may be changed in another PR, but the focus of this one is to just make it work).

This is how gdbserver feature of gdb work when used for remote debugging, if this functionality is not in the way it might be a good idea to keep, as it will work like most gdb users expect out of the box.

@aldelaro5
Copy link
Member Author

  • If active, boot to pause is effectively forced until we get a connection and a continue command (this behavior may be changed in another PR, but the focus of this one is to just make it work).

This is how gdbserver feature of gdb work when used for remote debugging, if this functionality is not in the way it might be a good idea to keep, as it will work like most gdb users expect out of the box.

The idea that me and @phire were talking about were you would start the emu normally WITHOUT the stub, but have the port in the settings and you would use some sort of menu option to start it so it would effectively stall AFTER the emulation started. This is actually how mgba works: you can start and stop it on demand.

It's fine for now if it only works on boot, but I was just saying we might improve it in another pr to not force it on boot.

@aldelaro5 aldelaro5 force-pushed the gdb-stub-rework branch 4 times, most recently from 9ac58d6 to 23507d1 Compare October 2, 2021 00:42
@aldelaro5
Copy link
Member Author

Ok I addressed the review, fixed compilation and also added a race condition fix I found while testing on windows.

Source/Core/Core/Core.cpp Outdated Show resolved Hide resolved
Copy link
Member

@phire phire left a comment

Choose a reason for hiding this comment

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

I'm not seeing any flaws in the multi-threading logic.

But there are a few minor fixes.

Source/Core/Core/Core.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/GDBStub.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/GDBStub.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/GDBStub.h Outdated Show resolved Hide resolved
@dreamsyntax
Copy link
Contributor

dreamsyntax commented Oct 21, 2021

Does this PR make #6325 obsolete?

Source/Core/Core/PowerPC/GDBStub.h Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/GDBStub.h Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/GDBStub.h Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/GDBStub.cpp Outdated Show resolved Hide resolved
@phire
Copy link
Member

phire commented Oct 21, 2021

Does this PR make #6325 obsolete?

Looks like it mostly obsoletes it, and does so with much less code and without an entire thread.

Source/Core/Core/PowerPC/GDBStub.h Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/GDBStub.h Outdated Show resolved Hide resolved
Copy link
Member

@phire phire left a comment

Choose a reason for hiding this comment

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

A few of your commit messages need cleaning up (Code Review Code Review)

other than that and the missing comment in CPU.h, I think it's ready.

Source/Core/Core/HW/CPU.h Show resolved Hide resolved
@phire phire merged commit b997048 into dolphin-emu:master Oct 23, 2021
10 checks passed
@aldelaro5 aldelaro5 deleted the gdb-stub-rework branch October 24, 2021 01:12
@dreamsyntax dreamsyntax mentioned this pull request Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants