Emulator Issues #13044
closed
Debugger: CheckBreakPoints is hit twice per breakpoint.
0%
Description
What's the problem? Describe what went wrong.
PowerPC::CheckBreakPoints in Jit.cpp and JitAsm.cpp are both hit on a breakpoint. This produces two log outputs per one hit. Also, working on future stuff and having it hit twice is not good. This is for code breakpoints.
Is the issue present in the latest development version? For future reference, please also write down the version number of the latest development version.
Yes, it seems to have been around for quite awhile.
Other Info
I removed the one from JitAsm and it didn't cause any issues for me, but I don't really know if this is the correct fix. The call doesn't return anything, so I don't think it's needed for more than triggering a breakpoint report. So my other question is: Are both jit and jitasm debug sections always hit together? Is this platform dependent?
Updated by JosJuice about 2 years ago
- Status changed from New to Accepted
- Assignee set to JosJuice
I think the reason why this wasn't noticed before is that if you enable breaking on the breakpoint (which I imagine is the most common), Dolphin will break after the JitAsm.cpp check and thus not run the Jit.cpp check.
The intent seems to be that the JitAsm.cpp check runs between blocks and the Jit.cpp check runs in the middle of blocks, but if you place a breakpoint at the beginning of a block, you end up triggering both.
Updated by JosJuice about 2 years ago
Can you check if breakpoints behave as expected in this pull request? https://github.com/dolphin-emu/dolphin/pull/11077
Updated by taolas about 2 years ago
Thanks for looking at this so quickly. The changes actually break a lot of debugging stuff. Step Over goes wacky. Breakpoints stop the game on wrong addresses, etc. I think the jitasm debugging stuff is necessary for things to function properly.
I went back and looked at my test-removal of this from jitasm:
ABI_PushRegistersAndAdjustStack({}, 0);
ABI_CallFunction(PowerPC::CheckBreakPoints);
ABI_PopRegistersAndAdjustStack({}, 0);
It appeared to work, but now I see it no longer puts our a log message when it breaks on a breakpoints, as jitasm prevents the jit hit on break like you said.
So we have generally working behavior when breaking/pausing on a breakpoint and broken duplication when logging, but not pausing for, the breakpoint. The duplicates on log-only may seem like a minor issue, but are causing bigger issues on future work.
Updated by JosJuice about 2 years ago
Okay, thanks for testing. Seems like I'll have to do more in-depth testing of this.
Updated by taolas about 2 years ago
I made a mistake. I thought setting a code breakpoint in the code tab would flag it for being logged, but it doesn't. I re-ran my previous test with
ABI_PushRegistersAndAdjustStack({}, 0);
ABI_CallFunction(PowerPC::CheckBreakPoints);
ABI_PopRegistersAndAdjustStack({}, 0);
removed from jitasm and manually setting break and log. Everything looks to be working correctly. This could be a potential fix.
Updated by JosJuice about 2 years ago
But how about the pull request? Is that one still buggy?
Updated by taolas about 2 years ago
Yeah. The PR makes Breakpoints and stepovers take you to random places. Completely broken.
Updated by JosJuice about 2 years ago
After doing a bunch of analysis today, I understand why the PR is broken, and I think your solution of removing just those three lines is the best one. I've updated the pull request.
Do note: As far as I can tell from the code, this solution will make breakpoints not trigger when you're using Step. That we don't break when using Step makes perfect sense, but would you consider not logging when using Step to be intended behavior?
Updated by taolas about 2 years ago
I checked a normal build and the Step buttons do not produce a log message at all for me. StepOut will stop on a breakpoint, but produces no log message. Again, that's a normal build and not this bugfix build.
Everything seems to be working good with the latest PR update. I see no changes in behavior.
Updated by JosJuice about 2 years ago
- Status changed from Accepted to Fixed
- Fixed in set to 5.0-17611