Capture BKPT number

[cjvaughter]

Just a quick update. isBKPT now stores the actual breakpoint number (+1) for later inspection.

[adamgreen]

Thanks for the PR.

I was wondering if it might be better to add a new field for this specific purpose rather than overloading the existing isBKPT field. Something like?

diff --git a/Core/src/CrashCatcher.c b/Core/src/CrashCatcher.c
index f967c7e..e0298c5 100644
--- a/Core/src/CrashCatcher.c
+++ b/Core/src/CrashCatcher.c
@@ -54,6 +54,7 @@ static void initFloatingPointFlag(Object* pObject);
 static int areFloatingPointCoprocessorsEnabled(void);
 static void initIsBKPT(Object* pObject);
 static int isBKPT(uint16_t instruction);
+static uint8_t getBKPTValue(uint16_t instruction);
 static int isBadPC();
 static void setStackSentinel(void);
 static void dumpSignature(void);
@@ -161,17 +162,21 @@ static int areFloatingPointCoprocessorsEnabled(void)

 static void initIsBKPT(Object* pObject)
 {
+    int wasBKPT = 0;
+    uint8_t bkptNumber = 0;
+
     /* On ARMv7M, can use fault status registers to determine if bad PC was cause of fault before checking to see if
        it points to a BKPT instruction. */
     if (CRASH_CATCHER_ISBKPT_SUPPORT && (isARMv6MDevice() || !isBadPC()))
     {
         const uint16_t* pInstruction = uint32AddressToPointer(pObject->pSP->pc);
-        pObject->info.isBKPT = isBKPT(*pInstruction);
-    }
-    else
-    {
-        pObject->info.isBKPT = 0;
+        uint16_t instruction = *pInstruction;
+        wasBKPT = isBKPT(instruction);
+        if (wasBKPT)
+             bkptNumber = getBKPTValue(instruction);
     }
+    pObject->info.isBKPT = wasBKPT;
+    pObject->info.bkptNumber = bkptNumber;
 }

 static int isBKPT(uint16_t instruction)
@@ -179,6 +184,11 @@ static int isBKPT(uint16_t instruction)
     return (instruction & 0xFF00) == 0xBE00;
 }

+static uint8_t getBKPTValue(uint16_t instruction)
+{
+    return instruction & 0x00FF;
+}
+
 static int isBadPC()
 {
     const uint32_t IACCVIOL = 1 << 0;
diff --git a/Core/tests/CrashCatcherTests.cpp b/Core/tests/CrashCatcherTests.cpp
index 0dbc9a2..5475f60 100644
--- a/Core/tests/CrashCatcherTests.cpp
+++ b/Core/tests/CrashCatcherTests.cpp
@@ -70,6 +70,7 @@ TEST_GROUP(CrashCatcher)
     uint32_t                       m_expectedFloatingPointRegisters[32+1];
     int                            m_expectedIsBKPT;
     uint16_t                       m_emulatedInstruction;
+    uint8_t                        m_expectedBkptValue;
     uint8_t                        m_memory[16];

     void setup()
@@ -133,12 +134,14 @@ TEST_GROUP(CrashCatcher)
     {
         m_emulatedInstruction = NOP_INSTRUCTION;
         m_expectedIsBKPT = 0;
+        m_expectedBkptValue = 0;
     }

     void emulateBKPT(uint8_t bkptNumber)
     {
         m_emulatedInstruction = BKPT_INSTRUCTION | (uint16_t)bkptNumber;
         m_expectedIsBKPT = 1;
+        m_expectedBkptValue = bkptNumber;
     }

     void initMemory()
@@ -229,6 +232,7 @@ TEST_GROUP(CrashCatcher)

         CHECK_EQUAL(m_expectedSP, pInfo->sp);
         CHECK_EQUAL(m_expectedIsBKPT, pInfo->isBKPT);
+        CHECK_EQUAL(m_expectedBkptValue, pInfo->bkptNumber);
     }
 };

diff --git a/include/CrashCatcher.h b/include/CrashCatcher.h
index bac49e2..84d4dce 100644
--- a/include/CrashCatcher.h
+++ b/include/CrashCatcher.h
@@ -44,6 +44,8 @@ typedef struct
     uint32_t    sp;
     /* Was this fault actually just a hardcoded breakpoint from which it is safe to continue. */
     int         isBKPT;
+    /* If isBKPT is non-zero then this is the immediate value associated with that BKPT instruction. */
+    uint8_t     bkptNumber;
 } CrashCatcherInfo;

[cjvaughter]

My too-conveniently-tiny change! 😮

Yes that's probably better in the long-term, at the very least for clarity. It does use one more word of stack, but I doubt that's an issue.

[adamgreen]

It does use one more word of stack, but I doubt that's an issue.

Yeah, I think that should be ok. Do you want to make this change to your branch, test, and force push it up for this PR?

[adamgreen]

I committed and pushed up the changes from my diff above. Thanks for the PR which inspired that change.

[cjvaughter]

Been busy. Thanks for the change!

[adamgreen]

Thanks for suggesting this change.