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

[Comments Needed] Core: Pause CPU and other systems only if initialized fully #8335

Open
wants to merge 2 commits into
base: master
from

Conversation

@CookiePLMonster
Copy link
Contributor

commented Aug 26, 2019

NOTE: I cannot fully guarantee this PR is free of side effects. Basing on my best intent and the current knowledge of Dolphin's codebase I expect it to work as intended and without unintended side effects, but I would really like other people to review and comment on the change.

Fixes a race condition where invoking RunAsCPUThread before Emulation thread has fully started, resulting in Dolphin attempting to pause yet uninitialized systems.

Cause of the issue

This issue occurs when RunAsCPUThread runs inside one of config changed callbacks processed when Dolphin emits OnEmulationStateChanged to Qt. There is a narrow time window (between s_is_booting.Set(); and s_hardware_initialized.Set();) where calls scheduled to run on CPU thread will try to pause CPU and systems before they are fully initialized. My change makes the constraint stricter, and makes PauseAndLock attempt to pause anything only once the CPU has fully booted, as opposed to current behaviour, where same logic is executed when the CPU has fully booted OR is booting.

Reproducing the issue

This race is relatively rare, so my repro steps are somewhat convoluted and involve adding an extra sleep to the code to make it easier to reproduce:

Prerequisites

a. "Slow down" emulation start routines by adding a sleep (100ms is enough) at the very top of HW::Init(). Reproducing the issue without this step is also possible, but harder - with an added sleep it's possible to achieve a 100% repro rate.
b. Have a game ready with game specific config which differs from the global one - in my test case, I have a game ready with Enable Emulated CPU Clock Override option enabled.

Repro steps

  1. Launch Dolphin.
  2. Open Config, navigate to Advanced. Ensure Enable Emulated CPU Clock Override is unchecked, then close the dialog.
  3. Open Graphics and then close the dialog.
  4. Launch the game.
  5. Observe results.

Observed results

Game crashes at either DSP::GetDSPEmulator() = nullptr or inside ExpansionInterface::PauseAndLock.

Expected results

Game launches normally.

Core: Turn stopping/started/HW initialized booleans into atomic flags
Those flags are written to and read from concurrently,
and reading/writing to the same non-atomic variable is UB
Core: Pause CPU and other systems only if they have initialized fully
Fixes a race condition where invoking RunAsCPUThread before Emulation thread has fully started,
resulting in Dolphin attempting to pause yet uninitialized systems
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
1 participant
You can’t perform that action at this time.