CodeViewWidget bugfix: Fix invalid symbol updates

[TryTwo]

Fixes this issue being hit again: #11593

Bug

The behavior described in that Pull was resulting in HotkeyScheduler infinitely hanging on:

void CPUManager::EnableStepping(bool stepping)
{
  std::lock_guard stepping_lock(m_stepping_lock);

while trying to do a FrameAdvance (hotkey) after the invalid thread guard was attempted. Only happens if loading a game while codewidget is active and possibly only if symbols are involved.

#12788 also bypasses the hotkey bug in that particular case.

Fix

Prevent PPCSymbol signals from triggering Updates during boot. These signals are unnecessary at that time. Thread guard breaks when Core::IsRunningAndStarted == false, so the check just makes sure that is true.

This could be avoided if Core::GetState() returned Core::State::Starting when s_hardware_initialized && !s_is_started, but I don't know if that'd break anything, so I didn't attempt it.

/edit I reduced the size and scope of this PR. Talking to mitaclaw, just focusing on handling the signal in CodeViewWidget would be better.

[JMC47]

This looks pretty simple and small to me. If no one else reviews this soon, just ping me and I'll merge it.

[JosJuice]

This feels like a flimsy fix. The actual bug is inside CPUThreadGuard/PauseAndLock, so that's where we should fix the bug. Still... I guess there's no real use in updating while the core is starting, so I suppose this change at least shouldn't make anything worse.

How about this: I will try to make a more proper fix this weekend, and if I don't have anything good by the end of the weekend, we can go forward with this change. (But maybe with the IsRunningAndStarted check moved to inside Update? Is it possible for any of the other signals to trigger while the core is starting?)

[TryTwo]

Yeah, this bug has happened multiple times in the past, and a universal fix would be best, but I'm not sure how to fix it..

On the other hand it shouldn't pass the Core Is Paused checked and update during boot, so your suggestion to put it inside the Update function is probably correct to future proof that behavior. I think that should happen regardless of other fixes. I'll work on it in a bit.

[TryTwo]

I moved the check to Update and put an additional check in CodeWidget::UpdateCallstack, as that has been an offender in that past.

[JosJuice]

Okay, unfortunately this turned out to be really complicated. Almost all of the complexity in Core.cpp seems to be there for a reason. My understanding is that the reason why GetState starts returning Paused halfway through the boot process is to let the user debug the apploader (as you can't use the debugger when the state is Starting). But PR #9623 in combination with us having gotten much more thorough with locking the CPU thread means that this can cause hangs.

My idea of a "simple" fix that changes what values GetState returns isn't going to be done soon, if ever. (Unless we want to break the ability to debug apploaders?) Long-term I would like to fix PauseAndLock so it waits for EmuThread to finish what it's doing if needed, fixing the bug described in PR #11593. But that's a complex task, especially since it raises some nasty risks for panic alert deadlocks. For the time being, feel free move forward with your quick fix.

There's one thing I don't understand with the description of this PR, though. What does frame advance have to do with the problem? Do you have to do a frame advance while the core is booting for the GUI thread to hang?

[JosJuice]

Actually, has debugging the apploader ever worked? The BS2 emulation code looks like it can't be interrupted, and there's this comment from 2009:

dolphin/Source/Core/Core/Boot/Boot_BS2Emu.cpp

Line 158 in 46a8993

// TODO - Make Apploader(or just RunFunction()) debuggable!!!

[JosJuice]

Assuming we don't need to care about apploader debugging, I've now uploaded my alternative fix as PR #12828. @TryTwo, can you test whether it fixes your issue?

[AdmiralCurtiss]

I've never tried it but I don't think apploader debugging has ever worked, considering there's a draft PR from Pokechu22 to try to make it work: #10923

[JosJuice]

I see. I think PR #12828 would break Pokechu's approach there... But it would be possible to go one step further than that approach and have move the disc reads into PPC code. Then the boot process could write PPC code to RAM and set PC to it, and the apploader would run once we enter the normal CPU loop. That gives us the performance benefit of the JIT when running the apploader too. So I would say I'm cautiously positive to the idea I had with PR #12828.

@Pokechu22, any opinion?

[Pokechu22]

My existing PR should use the JIT (it just uses HLE for reads). Doing disc reads via PPC code would work, but introduces complexities on Wii because then it would need to interact with IOS (as otherwise you get encrypted disc data, and on real hardware the DI interface is not available without ahbprot if I recall correctly, though I don't think we implement that). HLE would still be needed for HLEAppLoaderReport, too, but that's relatively simple.

[JosJuice]

My existing PR should use the JIT (it just uses HLE for reads).

I must have read it too quickly then :) If you're calling JIT -> HLE -> disc read, that sounds fine to me. There should be no drawback to that compared to JIT -> DI -> disc read, other than not accurately emulating load times, which is unimportant.

I don't think there's any reason to block my PR because of apploader debugging then.

[TryTwo]

There's one thing I don't understand with the description of this PR, though. What does frame advance have to do with the problem? Do you have to do a frame advance while the core is booting for the GUI thread to hang?

A recent PR placing FrameAdvance on the Host thread fixed this. This was originally triggering a hang when FrameAdvance was running on the hotkey thread and it encountered a mutex that was unlocked before being locked by the host thread. I'm not familiar enough to point out the technical issue there.

I looked at your code and it appears much more reasonable and should avoid these unexpected states hitting the UI widgets. I'll test it soon Thanks for working on this!

[TryTwo]

I tested @JosJuice's PR. It does work to protect from invalid update calls. The only thing I noticed is that there is still a short Pause state during booting, which may trigger some debug updates, which can come from PPCSymbolsChanged. With the updates being safe now, this is probably okay.