Skip to content

Debugger: Make debugger aware of thread status when using threads #3603

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.

Sign up for GitHub

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

Open
nuive wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Debugger: Make debugger aware of thread status when using threads #3603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion include/mgba/debugger/debugger.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,10 +212,13 @@ struct mDebuggerPlatform {
void (*nextInstructionInfo)(struct mDebuggerPlatform* d, struct mDebuggerInstructionInfo* info);
};

struct mCoreThreadInternal;

struct mDebugger {
struct mCPUComponent d;
struct mDebuggerPlatform* platform;
enum mDebuggerState state;
struct mCoreThreadInternal* threadImpl;
struct mCore* core;
struct mScriptBridge* bridge;
struct mStackTrace stackTrace;
Expand Down Expand Up @@ -245,9 +248,13 @@ void mDebuggerInit(struct mDebugger*);
void mDebuggerDeinit(struct mDebugger*);

void mDebuggerAttach(struct mDebugger*, struct mCore*);
#ifndef DISABLE_THREADING
void mDebuggerSetThread(struct mDebugger*, struct mCoreThreadInternal*);
void mDebuggerUnsetThread(struct mDebugger*);
#endif
void mDebuggerAttachModule(struct mDebugger*, struct mDebuggerModule*);
void mDebuggerDetachModule(struct mDebugger*, struct mDebuggerModule*);
void mDebuggerRunTimeout(struct mDebugger* debugger, int32_t timeoutMs);
void mDebuggerRunTimeout(struct mDebugger*, int32_t);
Copy link
Member

@endrift endrift Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please leave the name timeoutMs in the signature. I remove parameter names that are obvious, but timeoutMs includes the unit, which just int32_t doesn't.

void mDebuggerRun(struct mDebugger*);
void mDebuggerRunFrame(struct mDebugger*);
void mDebuggerEnter(struct mDebugger*, enum mDebuggerEntryReason, struct mDebuggerEntryInfo*);
Expand Down
11 changes: 9 additions & 2 deletions src/core/thread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,9 +285,10 @@ static THREAD_ENTRY _mCoreThreadRun(void* context) {
#ifdef ENABLE_DEBUGGERS
struct mDebugger* debugger = core->debugger;
if (debugger) {
MutexUnlock(&impl->stateMutex);
if (!debugger->threadImpl) {
mDebuggerSetThread(core->debugger, impl);
}
mDebuggerRun(debugger);
MutexLock(&impl->stateMutex);
if (debugger->state == DEBUGGER_SHUTDOWN) {
impl->state = mTHREAD_EXITING;
}
Expand Down Expand Up @@ -385,6 +386,12 @@ static THREAD_ENTRY _mCoreThreadRun(void* context) {
impl->state = mTHREAD_SHUTDOWN;
}
ConditionWake(&threadContext->impl->stateOffThreadCond);

#ifdef ENABLE_DEBUGGERS
if (core->debugger && core->debugger->threadImpl) {
mDebuggerUnsetThread(core->debugger);
}
#endif
MutexUnlock(&impl->stateMutex);

if (core->opts.rewindEnable) {
Expand Down
118 changes: 98 additions & 20 deletions src/debugger/debugger.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
#include <mgba/debugger/debugger.h>

#ifndef DISABLE_THREADING
#include <mgba/core/thread.h>
#endif

#include <mgba/core/core.h>

#include <mgba/internal/debugger/cli-debugger.h>
Expand Down Expand Up @@ -97,6 +101,20 @@ void mDebuggerAttach(struct mDebugger* debugger, struct mCore* core) {
core->attachDebugger(core, debugger);
}

#ifndef DISABLE_THREADING
void mDebuggerSetThread(struct mDebugger* debugger, struct mCoreThreadInternal* thread) {
if (!debugger->threadImpl) {
debugger->threadImpl = thread;
}
}

void mDebuggerUnsetThread(struct mDebugger* debugger) {
if (debugger->threadImpl) {
debugger->threadImpl = 0;
Copy link
Member

@endrift endrift Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
debugger->threadImpl = 0;
debugger->threadImpl = NULL;

Copy link
Contributor Author

@nuive nuive Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought 0 was the prefered C sintax and NULL for C++?

}
}
#endif

void mDebuggerAttachModule(struct mDebugger* debugger, struct mDebuggerModule* module) {
module->p = debugger;
*mDebuggerModuleListAppend(&debugger->modules) = module;
Expand Down Expand Up @@ -127,45 +145,105 @@ void mDebuggerRunTimeout(struct mDebugger* debugger, int32_t timeoutMs) {
size_t i;
size_t anyPaused = 0;

#ifndef DISABLE_THREADING
struct mCoreThreadInternal* impl = debugger->threadImpl;
#endif

switch (debugger->state) {
case DEBUGGER_RUNNING:
if (!debugger->platform->hasBreakpoints(debugger->platform)) {
debugger->core->runLoop(debugger->core);
} else {
debugger->core->step(debugger->core);
debugger->platform->checkBreakpoints(debugger->platform);
bool hasBreakpoints = debugger->platform->hasBreakpoints(debugger->platform);
#ifndef DISABLE_THREADING
if (impl) {
Copy link
Member

@endrift endrift Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost all of these checks can be put outside of the switch statement by just doing an unlock before and lock afterwards. The only exceptions are the early exit if the thread is paused, which doesn't really seem like it belongs in this function anyway (are there any other callsites that can't do the check itself?), and the two cases at the end, which you can do an early check for before unlocking. By doing the checks external to the switch statement, you can reduce duplication of code and massively reduce the diff size at the same time.

if (impl->state == mTHREAD_RUNNING) {
if (!hasBreakpoints) {
MutexUnlock(&impl->stateMutex);
debugger->core->runLoop(debugger->core);
MutexLock(&impl->stateMutex);
} else {
MutexUnlock(&impl->stateMutex);
debugger->core->step(debugger->core);
debugger->platform->checkBreakpoints(debugger->platform);
MutexLock(&impl->stateMutex);
}
}
} else
#endif
{
if (!hasBreakpoints) {
debugger->core->runLoop(debugger->core);
} else {
debugger->core->step(debugger->core);
debugger->platform->checkBreakpoints(debugger->platform);
}
}
break;
case DEBUGGER_CALLBACK:
debugger->core->step(debugger->core);
debugger->platform->checkBreakpoints(debugger->platform);
for (i = 0; i < mDebuggerModuleListSize(&debugger->modules); ++i) {
struct mDebuggerModule* module = *mDebuggerModuleListGetPointer(&debugger->modules, i);
if (module->needsCallback) {
module->custom(module);
#ifndef DISABLE_THREADING
if (impl) {
MutexUnlock(&impl->stateMutex);
debugger->core->step(debugger->core);
debugger->platform->checkBreakpoints(debugger->platform);
for (i = 0; i < mDebuggerModuleListSize(&debugger->modules); ++i) {
struct mDebuggerModule* module = *mDebuggerModuleListGetPointer(&debugger->modules, i);
if (module->needsCallback) {
module->custom(module);
}
}
MutexLock(&impl->stateMutex);
} else
#endif
{
debugger->core->step(debugger->core);
debugger->platform->checkBreakpoints(debugger->platform);
for (i = 0; i < mDebuggerModuleListSize(&debugger->modules); ++i) {
struct mDebuggerModule* module = *mDebuggerModuleListGetPointer(&debugger->modules, i);
if (module->needsCallback) {
module->custom(module);
}
}
}
break;
case DEBUGGER_PAUSED:
for (i = 0; i < mDebuggerModuleListSize(&debugger->modules); ++i) {
struct mDebuggerModule* module = *mDebuggerModuleListGetPointer(&debugger->modules, i);
if (module->isPaused) {
if (module->paused) {
module->paused(module, timeoutMs);
#ifndef DISABLE_THREADING
if (impl) {
MutexUnlock(&impl->stateMutex);
for (i = 0; i < mDebuggerModuleListSize(&debugger->modules); ++i) {
struct mDebuggerModule* module = *mDebuggerModuleListGetPointer(&debugger->modules, i);
if (module->isPaused) {
if (module->paused) {
module->paused(module, timeoutMs);
}
if (module->isPaused) {
++anyPaused;
}
} else if (module->needsCallback) {
module->custom(module);
}
}
MutexLock(&impl->stateMutex);
} else
#endif
{
for (i = 0; i < mDebuggerModuleListSize(&debugger->modules); ++i) {
struct mDebuggerModule* module = *mDebuggerModuleListGetPointer(&debugger->modules, i);
if (module->isPaused) {
++anyPaused;
if (module->paused) {
module->paused(module, timeoutMs);
}
if (module->isPaused) {
++anyPaused;
}
} else if (module->needsCallback) {
module->custom(module);
}
} else if (module->needsCallback) {
module->custom(module);
}
}
if (debugger->state == DEBUGGER_PAUSED && !anyPaused) {
debugger->state = DEBUGGER_RUNNING;
}
break;
case DEBUGGER_CREATED:
mLOG(DEBUGGER, ERROR, "Attempted to run debugger before initializtion");
mLOG(DEBUGGER, ERROR, "Attempted to run debugger before initialization");
return;
case DEBUGGER_SHUTDOWN:
return;
Expand Down