Closed einstein95 closed 3 years ago
What OS? I scanned 9 dotcodes in a row (one at a time) on Windows and it didn't reproduce. Also what sync settings?
Windows 10 Home 1909 build 18363.997, tried on all sync settings (Video, Audio, Video+Audio)
Does this happen if you hit cancel too? What happens if you try loading multiple cards at once? Do you have a second window open?
I'm not sure what you mean by hitting cancel. Loading multiple cards works fine. I do not have a second window open.
I have also seemingly gotten the same outcome by loading a dotcode on the "Application" screen (once you have scanned one of a multi-part dotcode).
I meant if you open the dotcode file dialog and hit cancel instead of selecting a file
Doesn't error or anything
In the logs, the last thing printed before each soft lock, before a bunch of [STUB] GBA I/O: Stub I/O register read: 088
, is [GAME ERROR] GBA Memory: Bad BIOS Load32: 0x00000008
Those log messages are almost certainly unrelated, especially the first one. And when you say it doesn't "error" do you mean it doesn't have message of any sort, or do you mean it doesn't freeze?
It does not freeze or anything, it behaves as it should.
So at some point I think I saw this happen, and though I can't reproduce it anymore, I think it had only happened to me when I had a second window open. Did you always have two windows open when doing this?
No, I always had the one window open.
@endrift, I can reproduce this freeze with one window. Using Window 10 (64-bit, installer .exe).
What's your configuration? And GPU?
W10 and Intel integrated GPU.
I suspect a run condition with the graphics and dialog box code.
Uh, not very likely. You didn't provide your configuration though.
For what it's worth, to avoid situations like this exact one, mGBA pauses all of the running cores while the dialog box is open. But I'm not sure why it's not unpausing in this specific circumstance.
@endrift, here is the config.ini. Change the extension to .ini. config.txt
Ah, looks like one of the pieces of info I need is in the qt.ini file, so I'll want that too. Thanks.
@endrift, similar lockup behavior on computer can be achieved by replacing the ROM with different roms 3-4 times. Bit inconsistent in reproduction but could be useful. This finding rules out any code that is exclusive to e-readers.
By using the file open dialog you mean?
Yes.
@endrift, I have narrowed down the problem to the setpause call in continueall().
I'm a little confused. I don't see setPaused or continueAll in that backtrace.
@Testsr can you see if changing the implementation of mCoreThreadUnpause
to this (in src/core/thread.c) fixes it?
@endrift It did not fix the issue.
If you hold down Ctrl-P to rapidly pause and unpause the emulator does it happen without opening the dialog?
Nope it does not trigger the lock. In fact, when the program enters that softlock state, the pause button does not do anything.
I meant does it trigger the lock. Also wait, you said "softlock state". So the UI is still functioning, but the game just doesn't continue?
Here are a few thing that do break the softlock state. -shutdown -change the sync settings, and then pressing the pause button again.
That sounds about right. It seems that for whatever reason the audio thread doesn't come back when it unpauses. I'm not sure why though.
Can you see if changing mSDLResumeAudio to this (src/platform/sdl/sdl-audio.c) changes anything? And if it outputs anything to the log viewer?
This change does not work and no logs. I can reproduce the lock on QT Multimedia as well.
So it doesn't even put anything in the logs?
You're still not answering all of my questions. Like, does rapidly toggling pausing trigger the frozen state? Does anything show up in the logs with that patch? What do you mean when you say you "narrowed down" the problem?
Does rapidly toggling pausing trigger the frozen state?- No, it does not. Does anything show up in the logs with that patch?- Nothing shows up. What do you mean when you say you "narrowed down" the problem? - I modified the continueAll function to create a file and exit, if the controler ptr was null. The program did not exit when I reproduced the softlock.
Why would the controller pointer ever be null? That doesn't narrow down the problem at all.
One more question for right now: when the problem occurs, does it show up as paused in the menu? Toggling it probably won't make a difference here though. If it shows up as paused still then the signal that it got unpaused probably got lost somehow.
@endrift Yes,it does.
What about applying this patch? Does it affect anything? A crash counts as affecting something.
diff --git a/src/core/thread.c b/src/core/thread.c
index 4fbe6e95c..49406a341 100644
--- a/src/core/thread.c
+++ b/src/core/thread.c
@@ -525,10 +525,12 @@ void mCoreThreadUnpause(struct mCoreThread* threadContext) {
bool frameOn = threadContext->impl->sync.videoFrameOn;
MutexLock(&threadContext->impl->stateMutex);
_waitOnInterrupt(threadContext->impl);
- if (threadContext->impl->state == THREAD_PAUSED || threadContext->impl->state == THREAD_PAUSING) {
+ if (threadContext->impl->state == THREAD_PAUSED) {
threadContext->impl->state = THREAD_RUNNING;
ConditionWake(&threadContext->impl->stateCond);
frameOn = threadContext->impl->frameWasOn;
+ } else {
+ abort();
}
MutexUnlock(&threadContext->impl->stateMutex);
@@ -551,7 +553,7 @@ void mCoreThreadTogglePause(struct mCoreThread* threadContext) {
bool frameOn = threadContext->impl->sync.videoFrameOn;
MutexLock(&threadContext->impl->stateMutex);
_waitOnInterrupt(threadContext->impl);
- if (threadContext->impl->state == THREAD_PAUSED || threadContext->impl->state == THREAD_PAUSING) {
+ if (threadContext->impl->state == THREAD_PAUSED) {
threadContext->impl->state = THREAD_RUNNING;
ConditionWake(&threadContext->impl->stateCond);
frameOn = threadContext->impl->frameWasOn;
@@ -568,9 +570,12 @@ void mCoreThreadTogglePause(struct mCoreThread* threadContext) {
void mCoreThreadPauseFromThread(struct mCoreThread* threadContext) {
bool frameOn = true;
MutexLock(&threadContext->impl->stateMutex);
- if (threadContext->impl->state == THREAD_RUNNING || (threadContext->impl->interruptDepth && threadContext->impl->savedState == THREAD_RUNNING)) {
+ if (threadContext->impl->state == THREAD_RUNNING) {
threadContext->impl->state = THREAD_PAUSING;
frameOn = false;
+ } else if (threadContext->impl->interruptDepth && threadContext->impl->savedState == THREAD_RUNNING) {
+ threadContext->impl->savedState = THREAD_PAUSING;
+ frameOn = false;
}
MutexUnlock(&threadContext->impl->stateMutex);
diff --git a/src/platform/qt/CoreController.cpp b/src/platform/qt/CoreController.cpp
index 613f486a5..3010c5ad7 100644
--- a/src/platform/qt/CoreController.cpp
+++ b/src/platform/qt/CoreController.cpp
@@ -117,13 +117,17 @@ CoreController::CoreController(mCore* core, QObject* parent)
m_threadContext.pauseCallback = [](mCoreThread* context) {
CoreController* controller = static_cast<CoreController*>(context->userData);
+ QMutexLocker locker(&controller->m_stateMutex);
QMetaObject::invokeMethod(controller, "paused");
+ controller->m_stateCond.wakeAll();
};
m_threadContext.unpauseCallback = [](mCoreThread* context) {
CoreController* controller = static_cast<CoreController*>(context->userData);
+ QMutexLocker locker(&controller->m_stateMutex);
QMetaObject::invokeMethod(controller, "unpaused");
+ controller->m_stateCond.wakeAll();
};
m_threadContext.logger.d.log = [](mLogger* logger, int category, enum mLogLevel level, const char* format, va_list args) {
@@ -412,6 +416,7 @@ void CoreController::reset() {
}
void CoreController::setPaused(bool paused) {
+ QMutexLocker locker(&m_stateMutex);
if (paused == isPaused()) {
return;
}
@@ -419,6 +424,7 @@ void CoreController::setPaused(bool paused) {
addFrameAction([this]() {
mCoreThreadPauseFromThread(&m_threadContext);
});
+ m_stateCond.wait(&m_stateMutex);
} else {
mCoreThreadUnpause(&m_threadContext);
}
@@ -953,11 +959,13 @@ void CoreController::finishFrame() {
memcpy(m_completeBuffer.data(), m_activeBuffer.constData(), width * height * BYTES_PER_PIXEL);
}
- QMutexLocker locker(&m_actionMutex);
- QList<std::function<void ()>> frameActions(m_frameActions);
- m_frameActions.clear();
- for (auto& action : frameActions) {
- action();
+ {
+ QMutexLocker locker(&m_actionMutex);
+ QList<std::function<void ()>> frameActions(m_frameActions);
+ m_frameActions.clear();
+ for (auto& action : frameActions) {
+ action();
+ }
}
updateKeys();
diff --git a/src/platform/qt/CoreController.h b/src/platform/qt/CoreController.h
index 166c60e7b..df2b1bc00 100644
--- a/src/platform/qt/CoreController.h
+++ b/src/platform/qt/CoreController.h
@@ -11,6 +11,7 @@
#include <QMutex>
#include <QObject>
#include <QSize>
+#include <QWaitCondition>
#include "VFileDevice.h"
@@ -215,6 +216,9 @@ private:
QMutex m_actionMutex{QMutex::Recursive};
QMutex m_bufferMutex;
+ QMutex m_stateMutex;
+ QWaitCondition m_stateCond;
+
int m_activeKeys = 0;
bool m_autofire[32] = {};
int m_autofireStatus[32] = {};
The patch does not fix the softlock, or abort the program if the softlock occurs, but if the program is in the softlocked state, the program hangs if it attempts to open the File Explorer again.
That does tell me that the hang is almost certainly from the callback not being called though, which is useful information. It's super frustrating to not be able to reproduce this locally though, because I'm grasping at straws for what could be going on. Is your computer single core? What's the CPU model?
My computer is dual core, and the model is i5-6300U.
Can you see if anything gets put in the logs if you trigger the bug with this diff applied?
diff --git a/src/core/thread.c b/src/core/thread.c
index 23eb00c53..93b0b4132 100644
--- a/src/core/thread.c
+++ b/src/core/thread.c
@@ -195,6 +195,8 @@ static THREAD_ENTRY _mCoreThreadRun(void* context) {
threadContext->resetCallback(threadContext);
}
+ bool wasPaused = false;
+
struct mCoreThreadInternal* impl = threadContext->impl;
while (impl->state < THREAD_EXITING) {
#ifdef USE_DEBUGGERS
@@ -242,6 +244,9 @@ static THREAD_ENTRY _mCoreThreadRun(void* context) {
}
while (impl->state >= THREAD_WAITING && impl->state <= THREAD_MAX_WAITING) {
ConditionWait(&impl->stateCond, &impl->stateMutex);
+ if (wasPaused && deferred != THREAD_PAUSED) {
+ mLOG(STATUS, ERROR, "Unexpected state transition from %i to %i (%i)", deferred, impl->state, impl->savedState);
+ }
if (impl->sync.audioWait) {
MutexUnlock(&impl->stateMutex);
@@ -254,11 +259,13 @@ static THREAD_ENTRY _mCoreThreadRun(void* context) {
MutexUnlock(&impl->stateMutex);
switch (deferred) {
case THREAD_PAUSING:
+ wasPaused = true;
if (threadContext->pauseCallback) {
threadContext->pauseCallback(threadContext);
}
break;
case THREAD_PAUSED:
+ wasPaused = false;
if (threadContext->unpauseCallback) {
threadContext->unpauseCallback(threadContext);
}
@endrift When the softlock is reproduced , the log says [ERROR] Status: Unexpected state transition from 0 to 3 (0).
Well, that confirms a suspicion. It explains a lot actually. Here, try this patch:
diff --git a/src/core/thread.c b/src/core/thread.c
index 23eb00c53..d9692961f 100644
--- a/src/core/thread.c
+++ b/src/core/thread.c
@@ -195,6 +195,8 @@ static THREAD_ENTRY _mCoreThreadRun(void* context) {
threadContext->resetCallback(threadContext);
}
+ bool wasPaused = false;
+
struct mCoreThreadInternal* impl = threadContext->impl;
while (impl->state < THREAD_EXITING) {
#ifdef USE_DEBUGGERS
@@ -242,6 +244,9 @@ static THREAD_ENTRY _mCoreThreadRun(void* context) {
}
while (impl->state >= THREAD_WAITING && impl->state <= THREAD_MAX_WAITING) {
ConditionWait(&impl->stateCond, &impl->stateMutex);
+ if (wasPaused && deferred != THREAD_PAUSED) {
+ mLOG(STATUS, ERROR, "Unexpected state transition from %i to %i (%i)", deferred, impl->state, impl->savedState);
+ }
if (impl->sync.audioWait) {
MutexUnlock(&impl->stateMutex);
@@ -254,11 +259,13 @@ static THREAD_ENTRY _mCoreThreadRun(void* context) {
MutexUnlock(&impl->stateMutex);
switch (deferred) {
case THREAD_PAUSING:
+ wasPaused = true;
if (threadContext->pauseCallback) {
threadContext->pauseCallback(threadContext);
}
break;
case THREAD_PAUSED:
+ wasPaused = false;
if (threadContext->unpauseCallback) {
threadContext->unpauseCallback(threadContext);
}
@@ -267,7 +274,10 @@ static THREAD_ENTRY _mCoreThreadRun(void* context) {
if (threadContext->run) {
threadContext->run(threadContext);
}
+ MutexLock(&threadContext->impl->stateMutex);
threadContext->impl->state = threadContext->impl->savedState;
+ ConditionWake(&threadContext->impl->stateCond);
+ MutexUnlock(&threadContext->impl->stateMutex);
break;
case THREAD_RESETING:
core->reset(core);
Not quite @endrift , but I think we are on the right track.
Well, parts of that patch are needed anyway, so I'm gonna commit it. But I'm still trying to figure out how it re-enters that loop...
Ok, I did a major refactor of how thread interrupts are handling, let me know if this diff fixes or breaks anything. Make sure to pull first, though, as some smaller pieces that were clearly wrong I pushed already, and this might have a merge conflict with it.
diff --git a/src/core/thread.c b/src/core/thread.c
index f4c1892d6..8f8163f56 100644
--- a/src/core/thread.c
+++ b/src/core/thread.c
@@ -196,6 +196,9 @@ static THREAD_ENTRY _mCoreThreadRun(void* context) {
}
struct mCoreThreadInternal* impl = threadContext->impl;
+ enum mCoreThreadState deferred = THREAD_RUNNING;
+ bool wasPaused = false;
+
while (impl->state < THREAD_EXITING) {
#ifdef USE_DEBUGGERS
struct mDebugger* debugger = core->debugger;
@@ -212,34 +215,38 @@ static THREAD_ENTRY _mCoreThreadRun(void* context) {
}
}
- enum mCoreThreadState deferred = THREAD_RUNNING;
MutexLock(&impl->stateMutex);
- while (impl->state > THREAD_MAX_RUNNING && impl->state < THREAD_EXITING) {
- deferred = impl->state;
-
+ if (deferred != THREAD_RUNNING) {
+ // We already did the deferred callback, advance the state machine
switch (deferred) {
- case THREAD_INTERRUPTING:
- impl->state = THREAD_INTERRUPTED;
- ConditionWake(&impl->stateCond);
- break;
case THREAD_PAUSING:
impl->state = THREAD_PAUSED;
break;
case THREAD_RESETING:
- impl->state = THREAD_RUNNING;
+ impl->state = impl->savedState;
+ break;
+ case THREAD_RUN_ON:
+ impl->state = impl->savedState;
+ ConditionWake(&threadContext->impl->stateCond);
break;
default:
break;
}
+ deferred = THREAD_RUNNING;
+ }
- if (deferred >= THREAD_MIN_DEFERRED && deferred <= THREAD_MAX_DEFERRED) {
+ while (deferred == THREAD_RUNNING && impl->state > THREAD_MAX_RUNNING && impl->state < THREAD_EXITING) {
+ if (impl->state >= THREAD_MIN_DEFERRED && impl->state <= THREAD_MAX_DEFERRED) {
+ // Callbacks need to be called outside of the critical section
+ deferred = impl->state;
break;
}
- deferred = impl->state;
- if (deferred == THREAD_INTERRUPTED) {
- deferred = impl->savedState;
+ if (impl->state == THREAD_INTERRUPTING) {
+ impl->state = THREAD_INTERRUPTED;
+ ConditionWake(&impl->stateCond);
}
+
while (impl->state >= THREAD_WAITING && impl->state <= THREAD_MAX_WAITING) {
ConditionWait(&impl->stateCond, &impl->stateMutex);
@@ -250,15 +257,22 @@ static THREAD_ENTRY _mCoreThreadRun(void* context) {
MutexLock(&impl->stateMutex);
}
}
+ if (wasPaused && impl->state == THREAD_RUNNING) {
+ deferred = THREAD_PAUSED;
+ }
}
MutexUnlock(&impl->stateMutex);
+
+ // Deferred callbacks can't be run inside of the critical section
switch (deferred) {
case THREAD_PAUSING:
+ wasPaused = true;
if (threadContext->pauseCallback) {
threadContext->pauseCallback(threadContext);
}
break;
case THREAD_PAUSED:
+ wasPaused = false;
if (threadContext->unpauseCallback) {
threadContext->unpauseCallback(threadContext);
}
@@ -267,10 +281,6 @@ static THREAD_ENTRY _mCoreThreadRun(void* context) {
if (threadContext->run) {
threadContext->run(threadContext);
}
- MutexLock(&threadContext->impl->stateMutex);
- threadContext->impl->state = threadContext->impl->savedState;
- ConditionWake(&threadContext->impl->stateCond);
- MutexUnlock(&threadContext->impl->stateMutex);
break;
case THREAD_RESETING:
core->reset(core);
@@ -410,12 +420,10 @@ void mCoreThreadEnd(struct mCoreThread* threadContext) {
void mCoreThreadReset(struct mCoreThread* threadContext) {
MutexLock(&threadContext->impl->stateMutex);
- if (threadContext->impl->state == THREAD_INTERRUPTED || threadContext->impl->state == THREAD_INTERRUPTING) {
- threadContext->impl->savedState = THREAD_RESETING;
- } else {
- threadContext->impl->state = THREAD_RESETING;
- }
- ConditionWake(&threadContext->impl->stateCond);
+ _waitOnInterrupt(threadContext->impl);
+ threadContext->impl->savedState = threadContext->impl->state;
+ threadContext->impl->state = THREAD_RESETING;
+ _waitUntilNotState(threadContext->impl, THREAD_RESETING);
MutexUnlock(&threadContext->impl->stateMutex);
}
@@ -460,9 +468,7 @@ void mCoreThreadInterrupt(struct mCoreThread* threadContext) {
return;
}
threadContext->impl->savedState = threadContext->impl->state;
- _waitOnInterrupt(threadContext->impl);
threadContext->impl->state = THREAD_INTERRUPTING;
- ConditionWake(&threadContext->impl->stateCond);
_waitUntilNotState(threadContext->impl, THREAD_INTERRUPTING);
MutexUnlock(&threadContext->impl->stateMutex);
}
@@ -480,7 +486,9 @@ void mCoreThreadInterruptFromThread(struct mCoreThread* threadContext) {
MutexUnlock(&threadContext->impl->stateMutex);
return;
}
- threadContext->impl->savedState = threadContext->impl->state;
+ if (threadContext->impl->state <= THREAD_MAX_WAITING || threadContext->impl->state >= THREAD_MAX_DEFERRED) {
+ threadContext->impl->savedState = threadContext->impl->state;
+ }
threadContext->impl->state = THREAD_INTERRUPTING;
ConditionWake(&threadContext->impl->stateCond);
MutexUnlock(&threadContext->impl->stateMutex);
@@ -505,7 +513,6 @@ void mCoreThreadRunFunction(struct mCoreThread* threadContext, void (*run)(struc
_waitOnInterrupt(threadContext->impl);
threadContext->impl->savedState = threadContext->impl->state;
threadContext->impl->state = THREAD_RUN_ON;
- ConditionWake(&threadContext->impl->stateCond);
_waitUntilNotState(threadContext->impl, THREAD_RUN_ON);
MutexUnlock(&threadContext->impl->stateMutex);
}
@@ -528,7 +535,7 @@ void mCoreThreadUnpause(struct mCoreThread* threadContext) {
bool frameOn = threadContext->impl->sync.videoFrameOn;
MutexLock(&threadContext->impl->stateMutex);
_waitOnInterrupt(threadContext->impl);
- if (threadContext->impl->state == THREAD_PAUSED || threadContext->impl->state == THREAD_PAUSING) {
+ if (threadContext->impl->state == THREAD_PAUSED) {
threadContext->impl->state = THREAD_RUNNING;
ConditionWake(&threadContext->impl->stateCond);
frameOn = threadContext->impl->frameWasOn;
diff --git a/src/platform/qt/CoreController.cpp b/src/platform/qt/CoreController.cpp
index 137f3db53..2581cda40 100644
--- a/src/platform/qt/CoreController.cpp
+++ b/src/platform/qt/CoreController.cpp
@@ -402,13 +402,7 @@ void CoreController::stop() {
}
void CoreController::reset() {
- bool wasPaused = isPaused();
- setPaused(false);
- Interrupter interrupter(this);
mCoreThreadReset(&m_threadContext);
- if (wasPaused) {
- setPaused(true);
- }
}
void CoreController::setPaused(bool paused) {
@endrift This diff is almost there, IMO. The softlock can still happen, but the reset button now works eventually.
What do you mean by "eventually"?
After loading the second or third dotcode while the emulation is unpaused, mGBA can pause itself and then softlock if unpaused manually.
Manually pausing between loading dotcodes gets passed this. I have successfully loaded Excitebike (10 strips total) this way.