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

gdbstub: Ensure gdbstub doesn't drop packets crucial to initialization #5106

Merged
merged 2 commits into from Mar 3, 2020

Conversation

GovanifY
Copy link
Contributor

@GovanifY GovanifY commented Feb 23, 2020

This fixes yuzu-emu/yuzu#1850, a common issue between citra and yuzu's gdbstub implementation.
Under the citra-qt codebase, during the VideoCore shader initialization, the EmuThread->RunLoop logic, used to emulate the game and, among other things, handle gdbstub packets, is frozen. This ends up making gdb send back its packet a second time and eventually sending the next packet in the list, in our case qSupported followed by vMustReplyEmpty. By this time gdb is expecting a reply to vMustReplyEmpty, qSupported having been dropped but citra just decided to unfreeze the gdbstub thread and process the packets, replying to the answer of qSupported.

To fix that, I introduced a way to defer the initialization of the gdb stub to the first handling of packets. This has two benefits:

  1. This allows the qt thread to not be frozen in place and still reactive along with its loading bar.
  2. This allows us to not have to modify the per-UI code to ensure the gdbstub works.

This was the least hacky fix I could think of, anything else requiring to change the behaviour of EmuThread and/or VideoCore. Moreso the HandlePacket function of gdbstub actually already has to check each time if the server is started, so the cost of this check is negligible, which is definitely something to keep in mind as HandlePacket is part of the main running loop of the EmuThread.


This change is Reviewable

@zhaowenlan1779
Copy link
Member

zhaowenlan1779 commented Feb 24, 2020

Sorry I'm not familiar with the gdbstub code, however I don't know how this didn't disable GDBStub completely. With this the GDBStub server is only enabled when HandlePacket is called, but the call to HandlePacket in Core is guarded with IsServerEnabled and therefore HandlePacket won't be called before the server is enabled. Are you sure this really works?

@GovanifY
Copy link
Contributor Author

GovanifY commented Feb 24, 2020

It seems to work for me, at least better than before this PR:

[  17.956777] Debug.GDBStub <Info> core/gdbstub/gdbstub.cpp:Init:1160: Starting GDB server on port 24689...
[  17.956834] Debug.GDBStub <Info> core/gdbstub/gdbstub.cpp:Init:1194: Waiting for gdb to connect...

[  18.150535] Debug.GDBStub <Info> core/gdbstub/gdbstub.cpp:Init:1207: Client connected.
[  18.150650] Debug.GDBStub <Debug> core/gdbstub/gdbstub.cpp:HandlePacket:1060: Packet: qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;fork-events+;vfork-events+;exec-events+;vContSupported+;QThreadEvents+;no-resumed+
[  18.150652] Debug.GDBStub <Debug> core/gdbstub/gdbstub.cpp:HandleQuery:537: gdb: query 'Supported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;fork-events+;vfork-events+;exec-events+;vContSupported+;QThreadEvents+;no-resumed+'

[  18.150688] Debug.GDBStub <Debug> core/gdbstub/gdbstub.cpp:HandlePacket:1060: Packet: vMustReplyEmpty
[  18.150716] Debug.GDBStub <Debug> core/gdbstub/gdbstub.cpp:HandlePacket:1060: Packet: Hg0
[  18.150757] Debug.GDBStub <Debug> core/gdbstub/gdbstub.cpp:HandlePacket:1060: Packet: qXfer:features:read:target.xml:0,1000
[  18.150757] Debug.GDBStub <Debug> core/gdbstub/gdbstub.cpp:HandleQuery:537: gdb: query 'Xfer:features:read:target.xml:0,1000'

[  18.151225] Debug.GDBStub <Debug> core/gdbstub/gdbstub.cpp:HandlePacket:1060: Packet: qTStatus
[  18.151225] Debug.GDBStub <Debug> core/gdbstub/gdbstub.cpp:HandleQuery:537: gdb: query 'TStatus'

[  18.151251] Debug.GDBStub <Debug> core/gdbstub/gdbstub.cpp:HandlePacket:1060: Packet: qTfV
[  18.151252] Debug.GDBStub <Debug> core/gdbstub/gdbstub.cpp:HandleQuery:537: gdb: query 'TfV'

Here is the related backtrace for the why:

gdb-peda$ bt
#0  GDBStub::Init () at /home/govanify/Documents/projects/programming/EMULATORS/citra/src/core/gdbstub/gdbstub.cpp:1218
#1  GDBStub::ToggleServer (status=0x1) at /home/govanify/Documents/projects/programming/EMULATORS/citra/src/core/gdbstub/gdbstub.cpp:1126
#2  GDBStub::ToggleServer (status=<optimized out>) at /home/govanify/Documents/projects/programming/EMULATORS/citra/src/core/gdbstub/gdbstub.cpp:1120
#3  0x00005555558576cf in GDBStub::HandlePacket () at /home/govanify/Documents/projects/programming/EMULATORS/citra/src/core/gdbstub/gdbstub.cpp:1047
#4  0x00005555558443f5 in Core::System::RunLoop (this=this@entry=0x555556417c20 <Core::System::s_instance>, tight_loop=tight_loop@entry=0x1) at /home/govanify/Documents/projects/programming/EMULATORS/citra/src/core/core.cpp:53
#5  0x00005555557373cb in EmuThread::run (this=0x55555f3ecb50) at /home/govanify/Documents/projects/programming/EMULATORS/citra/src/./core/core.h:71

If I'm not wrong IsServerEnabled is the setting flag, and not the "is the gdbstub running" flag.

Copy link
Member

@zhaowenlan1779 zhaowenlan1779 left a comment

Yeah you are right. As the GDBStub was toggled when Settings are applied, this still worked.

GDBStub::ToggleServer(values.use_gdbstub);

src/core/gdbstub/gdbstub.cpp Show resolved Hide resolved
@GovanifY
Copy link
Contributor Author

GovanifY commented Mar 2, 2020

Is there any reason why this hasn't been merged since this has been approved? Are we waiting for some merge window?

@zhaowenlan1779
Copy link
Member

zhaowenlan1779 commented Mar 2, 2020

Not really, it's more like we merge PRs after someone randomly declaring 'I'll merge this in 24 hours if no more comments' and then no one responds.


I'll merge this in 24 hours if no more comments.

@zhaowenlan1779 zhaowenlan1779 added the pr:pending-merge label Mar 2, 2020
@zhaowenlan1779 zhaowenlan1779 merged commit 7afcc0d into citra-emu:master Mar 3, 2020
3 checks passed
bunnei added a commit to yuzu-emu/yuzu that referenced this issue Mar 24, 2020
Port citra-emu/citra#5106: "gdbstub: Ensure gdbstub doesn't drop packets crucial to initialization"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
canary-merge pr:pending-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants