Debugger: Make debugger aware of thread status when using threads#3603
Debugger: Make debugger aware of thread status when using threads#3603nuive wants to merge 1 commit into
Conversation
endrift
left a comment
endrift
left a comment
There was a problem hiding this comment.
I'm really not sure what this fixes. It looks like it's just leaking the thread abstraction into the debugger, but only for one check on one callsite that could be done outside the function. Am I missing something?
|
|
||
| void mDebuggerUnsetThread(struct mDebugger* debugger) { | ||
| if (debugger->threadImpl) { | ||
| debugger->threadImpl = 0; |
There was a problem hiding this comment.
| debugger->threadImpl = 0; | |
| debugger->threadImpl = NULL; |
There was a problem hiding this comment.
I thought 0 was the prefered C sintax and NULL for C++?
| 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); |
There was a problem hiding this comment.
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.
| debugger->platform->checkBreakpoints(debugger->platform); | ||
| bool hasBreakpoints = debugger->platform->hasBreakpoints(debugger->platform); | ||
| #ifndef DISABLE_THREADING | ||
| if (impl) { |
There was a problem hiding this comment.
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.
I tried it, but if I do that inside _mCoreThreadRun all commands entered in the debugger window never reach debugger. It never pauses, it never shows help... |
|
I believe this PR is obviated by 141a82e. Let me know. |
2 participants
When mGBA used Threading, the debugger ignored this and would run even when the thread status was not RUNNING. This issue has been addressed by allowing the debugger to know thread's current status at every momment.
Mutex handling has also been added (when using threads) to prevent unwanted or unpredictable changes during critical sections.
Between this and #3602 issue #3355 should be fixed.