DolphinQt: Properly lock CPU before accessing emulated memory #11554
Conversation
|
Work in progress. Dolphin seems to deadlock when exiting emulation. |
JosJuice
force-pushed
the
host-lock-cpu
branch
from
44f0a03
to
41acbde
Compare
This fixes a problem I was having where using frame advance with the debugger open would frequently cause panic alerts about invalid addresses due to the CPU thread changing MSR.DR while the host thread was trying to access memory. To aid in tracking down all the places where we weren't properly locking the CPU, I've created a new type (in Core.h) that you have to pass as a reference or pointer to functions that require running as the CPU thread.
JosJuice
force-pushed
the
host-lock-cpu
branch
from
41acbde
to
7cecb28
Compare
This avoids a pseudo infinite loop where CodeWidget::UpdateCallstack would lock the CPU in order to read the call stack, causing the CPU to call Host_UpdateDisasmDialog because it's transitioning from running to pausing, causing Host::UpdateDisasmDialog to be emitted, causing CodeWidget::Update to be called, once again causing CodeWidget::UpdateCallstack to be called, repeating the cycle. Dolphin didn't go completely unresponsive during this, because Host_UpdateDisasmDialog schedules the emitting of Host::UpdateDisasmDialog to happen on another thread without blocking, but it was stopping certain operations like exiting emulation from working.
|
Okay, that problem should be fixed now. |
|
A RAII class for PauseAndLock() definitely makes sense, and I understand why you'd want to signal to the user that 'hey, this function is only supposed to be called from the CPU thread, so prove you're that by giving me this object', but... it's kinda weird to have a parameter that does absolutely nothing in most functions it's passed to. |
|
Yeah, I suppose it is a little weird, but this is the best way I could think of to at compile time ensure that the caller has made an effort to lock the CPU. I couldn't have found all the sites that required locking to be added without it. If having that parameter is too much of a bother, I could remove it now that I've found all the sites that require locking... but then the risk is high that we will introduce the problem again when adding some new debugger feature. |
|
Yeah, it's probably fine to keep it, I don't really have a better idea either. |
|
It feels a little weird that sometimes the guard is a pointer and sometimes it's a reference. Potential idea: What if you created a second object. Call it something like |
|
Something like this? Can squash if desired. (The extra Core.h includes you see in the commit are needed for performing the implicit conversion.) |
|
That seems worse to me, now we're actually back-and-forth converting a thing that's never used... |
|
It uses inheritance and doesn't actually get converted. Zero runtime cost. |
|
There is a runtime cost when we call the virtual functions. |
JosJuice
force-pushed
the
host-lock-cpu
branch
from
6ff9cb1
to
6f0266e
Compare
|
I've removed the commit again. What I would've wanted is really something like |
|
Also here: I haven't reviewed this but I'd rather get this merged before anyone creates merge conflicts with this, since this looks pretty annoying to keep updated otherwise. Are you okay with the design as-is, @phire? |
|
It's ok. |
|
Commit 7cecb28 breaks gecko codes |
| @@ -18,6 +18,7 @@ | |||
| #include <fmt/printf.h> | |||
|
|
|||
| #include "Common/Align.h" | |||
| #include "Common/Assert.h" | |||
There was a problem hiding this comment.
This commit doesn't seem to add any assertions to MemoryViewWidget.cpp. Was this include intended to do something? (I noticed this because it caused a merge conflict on #10957)
There was a problem hiding this comment.
Must be a leftover from an earlier version of this PR.
| const std::optional<u32> addr = | ||
| const std::optional<u32> target_addr = | ||
| PowerPC::debug_interface.GetMemoryAddressFromInstruction(code_line); | ||
|
|
||
| if (addr) | ||
| QApplication::clipboard()->setText(QStringLiteral("%1").arg(*addr, 8, 16, QLatin1Char('0'))); | ||
| { | ||
| QApplication::clipboard()->setText( | ||
| QStringLiteral("%1").arg(*target_addr, 8, 16, QLatin1Char('0'))); | ||
| } |
There was a problem hiding this comment.
Shouldn't this be using if (target_addr.has_value())? Now it's checking if GetContextAddress() != 0.
| const std::optional<u32> addr = | ||
| const std::optional<u32> target_addr = | ||
| PowerPC::debug_interface.GetMemoryAddressFromInstruction(code_line); | ||
|
|
||
| if (addr) | ||
| emit ShowMemory(*addr); | ||
| emit ShowMemory(*target_addr); |
| double result; | ||
| { | ||
| Core::CPUThreadGuard guard; | ||
| m_expr->param.func.context = &guard; |
There was a problem hiding this comment.
This only affects the top level of the expression tree (and only if it happens to be a function call), so this mostly isn't working as intended. The concept of a context in the expr library doesn't map nicely to what you want to do here. If this were something performance critical, I think the right fix would be to add a context parameter to expr_eval(). However, given that constructing guards is so cheap after the initial lock, rather than trying so hard to pass this one around it should be fine to just construct additional guards in the functions that need them.
There was a problem hiding this comment.
Thanks for catching that! I wasn't entirely sure if I was correctly understanding how the context worked.
Addressed in PR #11602.
This fixes a problem I was having where using frame advance with the debugger open would frequently cause panic alerts about invalid addresses due to the CPU thread changing MSR.DR while the host thread was trying to access memory.
To aid in tracking down all the places where we weren't properly locking the CPU, I've created a new type (in Core.h) that you have to pass as a reference or pointer to functions that require running as the CPU thread.