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

core, citra_qt: IPC Recorder #4847

Merged
merged 10 commits into from Sep 21, 2019
Merged

Conversation

zhaowenlan1779
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 commented Jul 22, 2019

This is based on @Subv's request on Discord (in a private channel) about half a month ago. I posted the chat here for anyone interested.

This implementation here basically satisfies all the needs, but the filter is built in the frontend instead of the core. The IPC requests are very frequent (actually a lot over 100 per second), which means that it is not feasible to keep this recorder on for a long time, as both time and memory costs can be high. However it should be okay for debugging purposes. My not-so-powerful laptop can have Pokemon UM run at 3x at near 150% speed while also recording all the 30000+ requests (gdb unattached), but the Qt widget does seem a bit unresponsive while browsing, and there was a substantial input delay of over a second for some reason...

This implementation features a docked widget, very similar to the previous debuggers, that basically contains a list to hold all recorded requests. A switch to toggle the feature and a simple filter (taking all columns into consideration) as well as a 'Clear' button are also present. Requests that have a status of 'HLE Unimplemented' or 'Error' (Handled but result code is not RESULT_SUCCESS) will be marked out with a red color. Double clicking any entry brings up the 'View Record' dialog which shows all relevant information on the request as well as 4 command buffers (request/reply, untranslated/translated). Note that, currently only HLE requests have function names available (based on the registered handlers info of their HLE implementations). LLE requests only have a function code.

For details of the implementation, refer to the commit messages. I do not really like the way the service and function names are got, but I just cannot come up with a better (or more proper) solution.

Screenshot:
image


This change is Reviewable

@zhaowenlan1779
Copy link
Member Author

zhaowenlan1779 commented Jul 22, 2019

MSVC CI failure looks real, I'll look into it should be fixed now

Copy link
Member

@wwylele wwylele left a comment

Overall looks good

src/core/hle/kernel/ipc_debugger/recorder.h Outdated Show resolved Hide resolved
src/core/hle/kernel/ipc_debugger/recorder.cpp Outdated Show resolved Hide resolved
src/citra_qt/debugger/ipc/record_dialog.cpp Outdated Show resolved Hide resolved
src/citra_qt/debugger/ipc/record_dialog.cpp Outdated Show resolved Hide resolved
src/core/hle/kernel/svc.cpp Outdated Show resolved Hide resolved
src/core/hle/kernel/hle_ipc.cpp Outdated Show resolved Hide resolved
@zhaowenlan1779 zhaowenlan1779 force-pushed the ipc-debugger branch 2 times, most recently from 7ac6715 to 7b18bb3 Compare Jul 23, 2019
Copy link
Member

@lioncash lioncash left a comment

Mostly just minor suggestions. Overall the structure itself looks good.

src/core/hle/kernel/ipc_debugger/recorder.cpp Outdated Show resolved Hide resolved
src/core/hle/kernel/ipc_debugger/recorder.h Show resolved Hide resolved
src/core/hle/kernel/ipc_debugger/recorder.cpp Outdated Show resolved Hide resolved
src/core/hle/kernel/ipc_debugger/recorder.cpp Outdated Show resolved Hide resolved
src/core/hle/kernel/ipc_debugger/recorder.cpp Outdated Show resolved Hide resolved
src/core/hle/kernel/ipc_debugger/recorder.cpp Outdated Show resolved Hide resolved
src/core/hle/kernel/hle_ipc.cpp Outdated Show resolved Hide resolved
src/core/hle/kernel/hle_ipc.cpp Outdated Show resolved Hide resolved
src/core/hle/kernel/hle_ipc.cpp Outdated Show resolved Hide resolved
src/core/hle/kernel/hle_ipc.cpp Outdated Show resolved Hide resolved
@jroweboy
Copy link
Contributor

jroweboy commented Aug 17, 2019

Temporarily removing from canary until we can get you to rebase it. Please add it back on after you rebase 👍

This is for displaying the function name for HLE requests. Probably it is possible to do the same for LLE ones but it would require having the HLE handlers available even when not using them, which doesn't seem to make sense and is more of a hack than a proper solution in my opinion.
This class resides in Kernel mainly because that, it's hard for kernel objects to get references to the System (and therefore to the Recorder), while much easier for KernelSystem. If this is to be moved to System the code will likely get much more complex with System (or Recorder) references passed everywhere.
Refer to the previous commit message for reasons why this is in kernel.
All necessary objects are available here, making this a great place for the registering part.
The 'translate' function is a great place to put this in IMO as it is possible to get both untranslated and translated cmdbufs. However a kernel reference has to be passed here, but it is not too hard fortunately.
Pretty much the same as LLE requests, the 'translate' part is chosen. A function is added to the context class to record requests that involves unimplemented HLE functions.
This is for displaying the service names. This function is only used in the frontend, because Recorder which is in the Kernel cannot and should not have access to SM in the System.
This 'View Record' dialog shows all relevant information on a record as well as the command buffers.
This widget provides a simple list of recorded requests as well as a simple filter and the 'Clear' button. Requests with status 'HLE Unimplemented' or 'Error' will be marked out with the red color.

This widget handles retrival of service name. For reasons refer to a previous commit message.

Added the widget to the Debugging menu, pretty much the same as other debugging widgets.
@zhaowenlan1779
Copy link
Member Author

zhaowenlan1779 commented Aug 28, 2019

Updated to use QFormLayout which aligns the widgets automatically and looks better.
image

Copy link
Contributor

@jroweboy jroweboy left a comment

I briefly read through everything and it looks fine to merge. Thanks for the hard work!

@jroweboy jroweboy merged commit 223bfc9 into citra-emu:master Sep 21, 2019
2 of 4 checks passed
@zhaowenlan1779 zhaowenlan1779 deleted the ipc-debugger branch Sep 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants