Improve vCont multi-threading support

Updates MRI core to version 1.1-20200506-0.

Added Platform_RtosSetThreadState() API to allow GDB to freeze
threads other than the one currently being single stepped via the
"set scheduler-locking step" command. The MRI core will only use
this new function if Platform_RtosIsSetThreadStateSupported()
return a non-zero value. Otherwise single stepping will silently
ignore an attempt to single step any thread other than the one
which caused the current halt and whether other threads are resumed
while single stepping is determined by the port implementor (usually
they would be resumed as that is the default behaviour expected by
GDB).

I moved alot of the thread tracking functionality of ThreadDebug into
the newly created ThreadList structure. It tracks the thread ids of the
active threads, their original priorities before being lowered to
osPriorityIdle to halt them during debugging, and their frozen/thawed/
single-stepping state. It ping pongs between g_pCurrThreadList and
g_pPrevThreadList so that it can reference the previously known
priority levels as the current priorities might still be those set by
the debugger if GDB requested that they be left frozen while single
stepping another thread.

I moved the call to restoreRtxHandlers() down into the DebugMon handler
before it attempts to awaken mriMain() rather than doing it in
mriMain() just after it awakens. This fixed an infinite loop that could
occur during certain single stepping operations:
* Single step debug event doesn't occur because that particular thread
  isn't scheduled by the RTOS.
* CTRL+C is pressed by the user to force a break.
* Communication channel see the CTRL+C and pends a DebugMon interrupt.
* DebugMon interrupt attempts to wake up mriMain() by signalling the
  MRI_THREAD_DEBUG_EVENT_FLAG flag. This will cause RTX to PendSV.
* When DebugMon handler returns, PendSV handler will execute in an
  attempt to switch the current context over to mriMain thread. This
  handler is hooked though for single stepping by ThreadDebug which
  means that it will pend a DebugMon interrupt again and so enters the
  infinite loop.
With my new fix, the PendSv hook is removed before DebugMon returns so
that the PendSv handler can run without pending yet another interrupt
into DebugMon.

The mriThreadSingleStepThreadId global is now set by
Platform_RtosSetThreadState() when the MRI core sets the state of a
thread to MRI_PLATFORM_THREAD_SINGLE_STEPPING. This allows GDB to
single step a thread other than the one which originally caused the
halt.

I updated Platform_RtosGetExtraThreadInfo() and
Platform_RtosGetThreadContext() in ThreadDebug to return NULL if the
specified thread-id isn't in the current g_pCurrThreadList. This fixes
a memory fault that was being generated by ThreadDebug when GDB would
first connect. When GDB first connects its sends a Hg0 command which
resulted in a call to Platform_RtosGetThreadContext() with a thread-id
of 0. The previous code would start trying to build up a register
context based off of this NULL pointer which was a bad thing.

I updated mriDebugException() to replay the calls to
Platform_RtosSetThreadState() before it makes an early exit for
situations like PC still being in the specified range during a ranged
single step operation. This allows ThreadDebug to make sure that its
g_pCurrThreadList object is always updated to the correct state before
it allows execution of the debuggee to continue.

I modified the behaviour of ranged single step to only skip the first
hardcoded breakpoint in the range whereas before it would skip all
breakpoints that were found within the range.

Now returns an error if GDB specifies more than one single step or more
than one continue action in a single vCont command. This assumption
makes the implementation of the vCont command much simpler and matches
the behaviour that I have seen from GDB so far.

Author: adamgreen

libraries/KernelDebug/src/KernelDebug.cpp

@@ -603,3 +603,12 @@ int Platform_RtosIsThreadActive(uint32_t threadId)
 
     return 0;
 }
+
+int Platform_RtosIsSetThreadStateSupported(void)
+{
+    return 0;
+}
+
+void Platform_RtosSetThreadState(uint32_t threadId, PlatformThreadState state)
+{
+}

libraries/MRI/src/core/cmd_continue.c

@@ -21,18 +21,17 @@
 #include <core/cmd_continue.h>
 
 
-static uint32_t skipHardcodedBreakpoint(void);
 static int shouldSkipHardcodedBreakpoint(void);
 static int isCurrentInstructionHardcodedBreakpoint(void);
 uint32_t ContinueExecution(int setPC, uint32_t newPC)
 {
-    uint32_t returnValue = skipHardcodedBreakpoint();
+    uint32_t returnValue = SkipHardcodedBreakpoint();
     if (setPC)
         Platform_SetProgramCounter(newPC);
     return (returnValue | HANDLER_RETURN_RESUME_PROGRAM | HANDLER_RETURN_RETURN_IMMEDIATELY);
 }
 
-static uint32_t skipHardcodedBreakpoint(void)
+uint32_t SkipHardcodedBreakpoint(void)
 {
     if (shouldSkipHardcodedBreakpoint())
     {
@@ -125,7 +124,7 @@ uint32_t HandleContinueWithSignalCommand(void)
 */
 uint32_t HandleDetachCommand(void)
 {
-    skipHardcodedBreakpoint();
+    SkipHardcodedBreakpoint();
     PrepareStringResponse("OK");
     return HANDLER_RETURN_RESUME_PROGRAM;
 }

libraries/MRI/src/core/cmd_continue.h

@@ -20,12 +20,14 @@
 
 /* Real name of functions are in mri namespace. */
 uint32_t mriCmd_ContinueExecution(int setPC, uint32_t newPC);
+uint32_t mriCmd_SkipHardcodedBreakpoint(void);
 uint32_t mriCmd_HandleContinueCommand(void);
 uint32_t mriCmd_HandleContinueWithSignalCommand(void);
 uint32_t mriCmd_HandleDetachCommand(void);
 
 /* Macroes which allow code to drop the mri namespace prefix. */
 #define ContinueExecution               mriCmd_ContinueExecution
+#define SkipHardcodedBreakpoint         mriCmd_SkipHardcodedBreakpoint
 #define HandleContinueCommand           mriCmd_HandleContinueCommand
 #define HandleContinueWithSignalCommand mriCmd_HandleContinueWithSignalCommand
 #define HandleDetachCommand             mriCmd_HandleDetachCommand

libraries/MRI/src/core/cmd_vcont.c

@@ -23,6 +23,7 @@
 #include <core/mri.h>
 #include <core/platforms.h>
 #include <core/try_catch.h>
+#include <string.h>
 
 
 typedef enum
@@ -54,6 +55,12 @@ typedef struct
 } Action;
 
 
+/* Store last continue/step actions here so that they can be used to replay Platform_RtosSetThreadState() calls
+   during ranged single steps. */
+static Action g_lastContinueAction;
+static Action g_lastStepAction;
+
+
 static uint32_t handleVContQueryCommand(void);
 static uint32_t handleVContCommand(void);
 static Action   parseAction(Buffer* pBuffer);
@@ -64,8 +71,11 @@ static Action   parseSingleStepWithSignalAction(Buffer* pBuffer);
 static Action   parseRangedSingleStepAction(Buffer* pBuffer);
 static AddressRange parseAddressRange(Buffer* pBuffer);
 static ThreadId parseOptionalThreadId(Buffer* pBuffer);
+static int      doesThreadIdMatchHaltedThreadId(const Action* pAction);
+static uint32_t handleSingleStepAndContinueCommands(const Action* pContinueAction, const Action* pStepAction);
 static uint32_t handleAction(const Action* pAction);
-static uint32_t handleRangedSingleStep(const Action* pAction);
+static uint32_t handleSingleStepAndContinueCommandsWithSetThreadState(const Action* pContinueAction, const Action* pStepAction);
+static uint32_t handleActionWithSetThreadState(const Action* pAction);
 static uint32_t justAdvancedPastBreakpoint(uint32_t continueReturn);
 /* Handle the extended 'v' commands used by gdb.
 
@@ -116,9 +126,8 @@ static uint32_t handleVContQueryCommand(void)
 */
 static uint32_t handleVContCommand(void)
 {
-    uint32_t          returnValue = 0;
-    int               useDefault = 1;
-    Action            defaultAction = { {0, THREAD_ID_NONE}, {0, 0}, ACTION_NONE };
+    Action            continueAction = { {0, THREAD_ID_NONE}, {0, 0}, ACTION_NONE };
+    Action            stepAction = { {0, THREAD_ID_NONE}, {0, 0}, ACTION_NONE };
     Buffer*           pBuffer = GetBuffer();
     Action            action;
 
@@ -134,14 +143,29 @@ static uint32_t handleVContCommand(void)
             return 0;
         }
 
-        if (action.threadId.type == THREAD_ID_NONE)
+        switch (action.type)
         {
-            defaultAction = action;
-        }
-        else
-        {
-            returnValue = handleAction(&action);
-            useDefault = 0;
+            case ACTION_RANGED_SINGLE_STEP:
+            case ACTION_SINGLE_STEP:
+                if (stepAction.type != ACTION_NONE)
+                {
+                    /* Should only have 1 single step action in vCont command. */
+                    PrepareStringResponse(MRI_ERROR_INVALID_ARGUMENT);
+                    return 0;
+                }
+                stepAction = action;
+                break;
+            case ACTION_CONTINUE:
+                if (action.threadId.type == THREAD_ID_SPECIFIC || continueAction.type != ACTION_NONE)
+                {
+                    /* Continue action shouldn't specify thread id & there should only be 1 of them in vCont command. */
+                    PrepareStringResponse(MRI_ERROR_INVALID_ARGUMENT);
+                    return 0;
+                }
+                continueAction = action;
+                break;
+            default:
+                break;
         }
     }
     if (Buffer_BytesLeft(pBuffer) > 0)
@@ -150,9 +174,11 @@ static uint32_t handleVContCommand(void)
         PrepareStringResponse(MRI_ERROR_INVALID_ARGUMENT);
         return 0;
     }
-    if (useDefault)
-        returnValue = handleAction(&defaultAction);
-    return returnValue;
+
+    if (Platform_RtosIsSetThreadStateSupported())
+        return handleSingleStepAndContinueCommandsWithSetThreadState(&continueAction, &stepAction);
+    else
+        return handleSingleStepAndContinueCommands(&continueAction, &stepAction);
 }
 
 static Action parseAction(Buffer* pBuffer)
@@ -284,57 +310,134 @@ static ThreadId parseOptionalThreadId(Buffer* pBuffer)
     return threadId;
 }
 
+static int doesThreadIdMatchHaltedThreadId(const Action* pAction)
+{
+    return pAction->threadId.type == THREAD_ID_SPECIFIC && pAction->threadId.id == Platform_RtosGetHaltedThreadId();
+}
+
+static uint32_t handleSingleStepAndContinueCommands(const Action* pContinueAction, const Action* pStepAction)
+{
+    uint32_t returnValue = 0;
+
+    if (pStepAction->type != ACTION_NONE)
+        returnValue |= handleAction(pStepAction);
+    else
+        returnValue |= handleAction(pContinueAction);
+
+    return returnValue;
+}
+
 static uint32_t handleAction(const Action* pAction)
 {
     const   int       noSetPC = 0;
     const   uint32_t  newPC = 0;
+    uint32_t          returnValue = 0;
 
-    if (pAction->threadId.type == THREAD_ID_SPECIFIC && pAction->threadId.id != Platform_RtosGetHaltedThreadId())
-    {
-        /* The only specific thread ID that can be handled is the halting thread's ID. */
-        PrepareStringResponse(MRI_ERROR_INVALID_ARGUMENT);
-        return 0;
-    }
-
+    /* No matter what thread ID is specified, it will be treated as though it was the halting thread-id since that is
+       all a stub running without Platform_RtosSetThreadState() can handle anyway. */
     switch (pAction->type)
     {
         case ACTION_CONTINUE:
             return ContinueExecution(noSetPC, newPC);
         case ACTION_SINGLE_STEP:
             return HandleSingleStepCommand();
         case ACTION_RANGED_SINGLE_STEP:
-            return handleRangedSingleStep(pAction);
+            returnValue = HandleSingleStepCommand();
+            if (returnValue)
+                SetSingleSteppingRange(&pAction->range);
+            return returnValue;
         default:
             PrepareStringResponse(MRI_ERROR_INVALID_ARGUMENT);
             return 0;
    }
 }
 
-static uint32_t handleRangedSingleStep(const Action* pAction)
+static uint32_t handleSingleStepAndContinueCommandsWithSetThreadState(const Action* pContinueAction, const Action* pStepAction)
 {
-    const uint32_t noSetPC = 0;
-    uint32_t       returnValue = 0;
-    uint32_t       currPC;
+    uint32_t returnValue = 0;
+    uint32_t skippedHardcodedBreakpoint = 0;
+
+    if (pContinueAction->type != ACTION_NONE || doesThreadIdMatchHaltedThreadId(pStepAction))
+        skippedHardcodedBreakpoint = SkipHardcodedBreakpoint();
+
+    if (pContinueAction->type != ACTION_NONE)
+        returnValue |= handleActionWithSetThreadState(pContinueAction);
+
+    if (pStepAction->type != ACTION_NONE)
+    {
+        if (pStepAction->threadId.type != THREAD_ID_SPECIFIC)
+        {
+            PrepareStringResponse(MRI_ERROR_INVALID_ARGUMENT);
+            return 0;
+        }
+        returnValue |= handleActionWithSetThreadState(pStepAction);
+    }
 
-    do
+    if (pContinueAction->type == ACTION_NONE && pStepAction->type == ACTION_NONE)
     {
-        returnValue = ContinueExecution(noSetPC, 0);
-        currPC = Platform_GetProgramCounter();
-    } while (justAdvancedPastBreakpoint(returnValue) && currPC >= pAction->range.start && currPC < pAction->range.end);
+        PrepareStringResponse(MRI_ERROR_INVALID_ARGUMENT);
+        return 0;
+    }
+
+    g_lastContinueAction = *pContinueAction;
+    g_lastStepAction = *pStepAction;
 
-    if (justAdvancedPastBreakpoint(returnValue))
+    if (doesThreadIdMatchHaltedThreadId(pStepAction) && justAdvancedPastBreakpoint(skippedHardcodedBreakpoint))
     {
-        /* Treat the advance through the range over hardcoded breakpoints as the single step and don't
-           resume execution but send T stop response instead. */
+        /* Treat the advance over hardcoded breakpoints as the single step and don't resume execution but send T
+            stop response instead. */
         return Send_T_StopResponse();
     }
 
-    Platform_EnableSingleStep();
-    SetSingleSteppingRange(&pAction->range);
     return returnValue;
 }
 
+static uint32_t handleActionWithSetThreadState(const Action* pAction)
+{
+    uint32_t returnValue = 0;
+
+    switch (pAction->type)
+    {
+        case ACTION_CONTINUE:
+            Platform_RtosSetThreadState(MRI_PLATFORM_ALL_THREADS, MRI_PLATFORM_THREAD_THAWED);
+            returnValue = HANDLER_RETURN_RESUME_PROGRAM | HANDLER_RETURN_RETURN_IMMEDIATELY;
+            break;
+        case ACTION_SINGLE_STEP:
+            Platform_RtosSetThreadState(pAction->threadId.id, MRI_PLATFORM_THREAD_SINGLE_STEPPING);
+            Platform_EnableSingleStep();
+            returnValue = HANDLER_RETURN_RESUME_PROGRAM | HANDLER_RETURN_RETURN_IMMEDIATELY;
+            break;
+        case ACTION_RANGED_SINGLE_STEP:
+            Platform_RtosSetThreadState(pAction->threadId.id, MRI_PLATFORM_THREAD_SINGLE_STEPPING);
+            Platform_EnableSingleStep();
+            SetSingleSteppingRange(&pAction->range);
+            returnValue = HANDLER_RETURN_RESUME_PROGRAM | HANDLER_RETURN_RETURN_IMMEDIATELY;
+            break;
+        default:
+            break;
+   }
+   return returnValue;
+}
+
 static uint32_t justAdvancedPastBreakpoint(uint32_t continueReturn)
 {
     return continueReturn & HANDLER_RETURN_SKIPPED_OVER_BREAK;
 }
+
+
+void ReplaySetThreadStateCalls(void)
+{
+    if (!Platform_RtosIsSetThreadStateSupported())
+        return;
+    if (g_lastContinueAction.type != ACTION_NONE)
+        Platform_RtosSetThreadState(MRI_PLATFORM_ALL_THREADS, MRI_PLATFORM_THREAD_THAWED);
+    if (g_lastStepAction.type != ACTION_NONE)
+        Platform_RtosSetThreadState(g_lastStepAction.threadId.id, MRI_PLATFORM_THREAD_SINGLE_STEPPING);
+}
+
+
+void ForgetSetThreadStateCalls(void)
+{
+    memset(&g_lastContinueAction, 0, sizeof(g_lastContinueAction));
+    memset(&g_lastStepAction, 0, sizeof(g_lastStepAction));
+}

libraries/MRI/src/core/cmd_vcont.h

@@ -20,8 +20,12 @@
 
 /* Real name of functions are in mri namespace. */
 uint32_t mriCmd_HandleVContCommands(void);
+void     mriCmd_ReplaySetThreadStateCalls(void);
+void     mriCmd_ForgetSetThreadStateCalls(void);
 
 /* Macroes which allow code to drop the mri namespace prefix. */
-#define HandleVContCommands     mriCmd_HandleVContCommands
+#define HandleVContCommands         mriCmd_HandleVContCommands
+#define ReplaySetThreadStateCalls   mriCmd_ReplaySetThreadStateCalls
+#define ForgetSetThreadStateCalls   mriCmd_ForgetSetThreadStateCalls
 
 #endif /* CMD_VCONT_H_ */

libraries/MRI/src/core/mri.c

@@ -194,7 +194,10 @@ void mriDebugException(MriContext* pContext)
         {
             resumeExecution = pTempBreakpointCallback(pvTempBreakpointContext);
             if (resumeExecution)
+            {
+                ReplaySetThreadStateCalls();
                 return;
+            }
         }
     }
 
@@ -206,6 +209,7 @@ void mriDebugException(MriContext* pContext)
         {
             Platform_DisableSingleStep();
             Platform_EnableSingleStep();
+            ReplaySetThreadStateCalls();
             return;
         }
     }
@@ -220,6 +224,7 @@ void mriDebugException(MriContext* pContext)
         Semihost_HandleSemihostRequest() &&
         !justSingleStepped )
     {
+        ReplaySetThreadStateCalls();
         prepareForDebuggerExit();
         return;
     }
@@ -228,6 +233,7 @@ void mriDebugException(MriContext* pContext)
         Platform_DisplayFaultCauseToGdbConsole();
     Send_T_StopResponse();
 
+    ForgetSetThreadStateCalls();
     GdbCommandHandlingLoop();
 
     prepareForDebuggerExit();