dolphin
dolphin copied to clipboard
dolphin-emu
Core/DolphinQt: Add Pause on Panic Option for DSI Exceptions and Unknown Instruction
Overview
- Adds
Pause on Panicto Config -> Advanced
- JitBase.cpp change is necessary because otherwise the JIT splices would break on the next block rather than the instruction that threw the exception.
- Defaults to OFF since this has the same performance hit that enabling MMU does.
- Currently, this is only affecting panics for invalid read/writes. Ex
stw r4, r5where r5 is a non-valid address (ex 99232100) - Additionally, I also check for the flag in Interpreter.cpp for Unknown Instruction. I do not want to default this to pause/break as this will negatively impact a user who runs into an Unknown Instruction that doesn't crash Dolphin or the emulation. This will be updated with the Panic message rewrite future PR.
Why
- When debugging games/homebrew if a read/write exception occurs, there is no easy way to observe the callstack and registers at time of fault. With this, the emulation will pause on any such exception while enabled.
TODO / Before Merge (All Done)
- ~~Does not yet save the checked state on program close.~~ | Done
- ~~Verify the name "Pause on Panic Handles" is okay- open to different names.~~ | Done (changed to Pause on Panic Handler)
- ~~Expand to all PanicMessage types if reasonable~~ Opted to only account for the most common scenarios DSI & Unknown Instruction. A future PR will rework the Yes/No/Ignore entirely, which will follow this https://github.com/dolphin-emu/dolphin/pull/10175#issuecomment-946149157.
- ~~Do I need to account for this PR: https://github.com/dolphin-emu/dolphin/pull/10162 ?~~ Nope, thanks JosJuice
Do I need to account for this PR: https://github.com/dolphin-emu/dolphin/pull/10162 ?
If you're not touching Jit64, you don't need to touch JitArm64 either. And if you're not touching JitArm64, you don't have to take that PR into account.
I know you are also working on pausing for other panic alert types. Do you actually want to have a single option that enables pausing on panic handlers?
Or would it be smarter to have the feature enabled by default, giving the user the choice between ignore/stop/break) for every (supported) panic alert except for MMU exceptions (which require either memchecks or interpeter).
Also, should it be called Pause on Panic, or Break on Panic, with explicit mention of debugging in the tooltip?
Talking about the tooltip, you probably want to mention that the cost is the same as enabling MMU.
I know you are also working on pausing for other panic alert types. Do you actually want to have a single option that enables pausing on panic handlers?
Or would it be smarter to have the feature enabled by default, giving the user the choice between ignore/stop/break) for every (supported) panic alert except for MMU exceptions (which require either memchecks or interpeter).
If we go this route / default behavior, then I think we should change the behavior of the Panic Messages entirely. Of course, Pause/Break wouldn't be available without the memcheck/performance hit for read/write exceptions, but it would be valid for Unknown Instruction scenario below.
Unknown Instruction (ill instruction) for example:
Current Dolphin behavior is as follows:
Yes / Ignore / No - What am I saying Yes/No to?
Yes - The emulation skips the bad instruction. If another panic happens later another pop-up message will appear.
Ignore - The same as Yes, but also setting a flag to no longer pop up future panic messages.
No - The program (Dolphin) crashes.
Ideally, I would propose: **Note, see next comment below this post
Not sure how feasible since Yes/Ignore/No is shared across a bunch of scenarios.
Also, should it be called Pause on Panic, or Break on Panic, with explicit mention of debugging in the tooltip?
I don't think its necessary to say Break / mention debugger, as having this on would affect users in non-debug as well. If it was on by default, then even if they chose "Ignore" earlier and later run into exceptions all they would see is the emulation paused, with seemingly no reason.
Talking about the tooltip, you probably want to mention that the cost is the same as enabling MMU.
Yes, or if MMU is on there is no additional cost.
If we go with new default behavior I propose this. Though, read/writes exceptions would still need the checkbox to work properly under advanced. Maybe Pause would be hidden for those exceptions if not enabled.
The truly ideal experience would be something like this. We should do away with "Ignore for this session" and instead just call it Ignore All. When the user Ends Emulation, then we would also automatically reset the 'Ignore for this session' flag, with respect to the Panic Handle option in Interface.
Ignore would do what the current "Yes" behavior does. Ignore All would do what the current Ignore for this session behavior does (but it will only persist for current run until emulation is stopped) Pause would pause the emulation on the instruction after the unknown/bad instruction. Stop would stop the emulation completely.
Yes, or if MMU is on there is no additional cost.
Well, in the MMU is enabled, you aren't going to get any MMU related panic alerts. Not sure if we have panic alerts inside a MMIO handler, which would be the only other source of memory instruction related panics.
Yes, or if MMU is on there is no additional cost.
Well, in the MMU is enabled, you aren't going to get any MMU related panic alerts. Not sure if we have panic alerts inside a MMIO handler, which would be the only other source of memory instruction related panics.
Oh I understand that. I just mean as far as performance hit its identical to MMU Enabled since both just have the memcheck flag on.
I've updated this per suggestions. I would rather get the most common exceptions (read/write and unknown instr) out now. I've updated the hint to properly reflect that.
In another PR I will look at reworking the Panic MessageBox entirely, where we can then have behaviors like in my mock2 image https://github.com/dolphin-emu/dolphin/pull/10175#issuecomment-946149157 , but this is a bigger effort that will affect a lot code.
As-is I think it is ready to merge as a minor improvement.
Going to rebase and resolve the conflicts. Did some testing on latest dev dolphin, the idea behind this change is still needed even with improvements to the exception handler (it is still very easy to crash to desktop in latest dev)
Migrated to new config format
old: https://github.com/dolphin-emu/dolphin/pull/10175/commits/1acced43dee8ede59fbfbd9106a2d854c0ca87ea
I think this would be better if it weren't persistent across launches. I definitely would use this if it got merged, but not every time I use Dolphin. Maybe it could go under Options alongside Boot to Pause?
I think this would be better if it weren't persistent across launches. I definitely would use this if it got merged, but not every time I use Dolphin. Maybe it could go under Options alongside Boot to Pause?
I'm not opposed to this, but seeing as its similar perf hit to MMU I think it makes sense to have the context under config->advanced. Open to other opinions though.
This PR still LGTM. You might want to squash the 2 commits into a single one with a proper commit message.
Done, rebased on latest master + squashed