From e68d3ed00f8540f3be2ebe6852f026a839603e6a Mon Sep 17 00:00:00 2001
From: Adam Higerd <chighland@gmail.com>
Date: Sun, 9 Aug 2020 23:36:46 -0500
Subject: [PATCH] Stack traces: fix tracing of indirect jumps and interrupt
 handlers

---
 src/arm/debugger/debugger.c | 34 +++++++++++++++++++++++-----------
 src/debugger/cli-debugger.c |  8 +++++++-
 2 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/src/arm/debugger/debugger.c b/src/arm/debugger/debugger.c
index fefcaec30..7b75bdc92 100644
--- a/src/arm/debugger/debugger.c
+++ b/src/arm/debugger/debugger.c
@@ -29,6 +29,16 @@ static bool ARMDecodeCombined(struct ARMCore* cpu, struct ARMInstructionInfo* in
 	}
 }
 
+static inline int ARMGetStack(struct ARMRegisterFile* regs) {
+	if (!regs) {
+		return MODE_USER;
+	}
+	if (regs->cpsr.priv == MODE_SYSTEM) {
+		return MODE_USER;
+	}
+	return regs->cpsr.priv;
+}
+
 static bool ARMDebuggerUpdateStackTraceInternal(struct mDebuggerPlatform* d, uint32_t pc) {
 	struct ARMDebugger* debugger = (struct ARMDebugger*) d;
 	struct ARMCore* cpu = debugger->cpu;
@@ -36,7 +46,8 @@ static bool ARMDebuggerUpdateStackTraceInternal(struct mDebuggerPlatform* d, uin
 	struct mStackTrace* stack = &d->p->stackTrace;
 
 	struct mStackFrame* frame = mStackTraceGetFrame(stack, 0);
-	if (frame && frame->frameBaseAddress < (uint32_t) cpu->gprs[ARM_SP]) {
+	int currentStack = ARMGetStack(&cpu->regs);
+	if (frame && frame->frameBaseAddress < (uint32_t) cpu->gprs[ARM_SP] && currentStack == ARMGetStack(frame->regs)) {
 		// The stack frame has been popped off the stack. This means the function
 		// has been returned from, or that the stack pointer has been otherwise
 		// manipulated. Either way, the function is done executing.
@@ -45,7 +56,7 @@ static bool ARMDebuggerUpdateStackTraceInternal(struct mDebuggerPlatform* d, uin
 			shouldBreak = shouldBreak || frame->breakWhenFinished;
 			mStackTracePop(stack);
 			frame = mStackTraceGetFrame(stack, 0);
-		} while (frame && frame->frameBaseAddress < (uint32_t) cpu->gprs[ARM_SP]);
+		} while (frame && frame->frameBaseAddress < (uint32_t) cpu->gprs[ARM_SP] && currentStack == ARMGetStack(frame->regs));
 		if (shouldBreak) {
 			struct mDebuggerEntryInfo debuggerInfo = {
 				.address = pc,
@@ -84,10 +95,10 @@ static bool ARMDebuggerUpdateStackTraceInternal(struct mDebuggerPlatform* d, uin
 		return false;
 	}
 
-	bool isCall = interrupt || (info.branchType & ARM_BRANCH_LINKED);
+	bool isCall = (info.branchType & ARM_BRANCH_LINKED);
 	uint32_t destAddress;
 
-	if (interrupt && info.branchType == ARM_BRANCH_NONE) {
+	if (interrupt && !isCall) {
 		// The stack frame was already pushed up above, so there's no
 		// action necessary here, but we still want to check for a
 		// breakpoint down below.
@@ -115,6 +126,9 @@ static bool ARMDebuggerUpdateStackTraceInternal(struct mDebuggerPlatform* d, uin
 			bool isBranch = ARMInstructionIsBranch(info.mnemonic);
 			int reg = (isBranch ? info.op1.reg : info.op2.reg);
 			destAddress = cpu->gprs[reg];
+			if ((info.operandFormat & ARM_OPERAND_MEMORY_2) && (info.branchType & ARM_BRANCH_INDIRECT)) {
+				destAddress = cpu->memory.load32(cpu, destAddress, NULL);
+			}
 			if (isBranch || (info.op1.reg == ARM_PC && !isMovPcLr)) {
 				// ARMv4 doesn't have the BLX opcode, so it uses an assignment to LR before a BX for that purpose.
 				struct ARMInstructionInfo prevInfo;
@@ -145,25 +159,23 @@ static bool ARMDebuggerUpdateStackTraceInternal(struct mDebuggerPlatform* d, uin
 		return false;
 	}
 
-	if (info.branchType & ARM_BRANCH_INDIRECT) {
-		destAddress = cpu->memory.load32(cpu, destAddress, NULL);
-	}
-
 	if (isCall) {
 		int instructionLength = isWideInstruction ? WORD_SIZE_ARM : WORD_SIZE_THUMB;
 		frame = mStackTracePush(stack, pc, destAddress + instructionLength, cpu->gprs[ARM_SP], &cpu->regs);
 		if (!(debugger->stackTraceMode & STACK_TRACE_BREAK_ON_CALL)) {
 			return false;
 		}
-	} else {
-		mStackTracePop(stack);
+	} else if (!interrupt) {
+		if (frame && currentStack == ARMGetStack(frame->regs)) {
+			mStackTracePop(stack);
+		}
 		if (!(debugger->stackTraceMode & STACK_TRACE_BREAK_ON_RETURN)) {
 			return false;
 		}
 	}
 	struct mDebuggerEntryInfo debuggerInfo = {
 		.address = pc,
-		.type.st.traceType = isCall ? STACK_TRACE_BREAK_ON_CALL : STACK_TRACE_BREAK_ON_RETURN,
+		.type.st.traceType = (interrupt || isCall) ? STACK_TRACE_BREAK_ON_CALL : STACK_TRACE_BREAK_ON_RETURN,
 		.pointId = 0
 	};
 	mDebuggerEnter(d->p, DEBUGGER_ENTER_STACK, &debuggerInfo);
diff --git a/src/debugger/cli-debugger.c b/src/debugger/cli-debugger.c
index 691474f3a..c2748d913 100644
--- a/src/debugger/cli-debugger.c
+++ b/src/debugger/cli-debugger.c
@@ -1035,7 +1035,13 @@ static void _reportEntry(struct mDebugger* debugger, enum mDebuggerEntryReason r
 	case DEBUGGER_ENTER_STACK:
 		if (info) {
 			if (info->type.st.traceType == STACK_TRACE_BREAK_ON_CALL) {
-				cliDebugger->backend->printf(cliDebugger->backend, "Hit function call at at 0x%08X\n", info->address);
+				struct mStackTrace* stack = &cliDebugger->d.stackTrace;
+				struct mStackFrame* frame = mStackTraceGetFrame(stack, 0);
+				if (frame->interrupt) {
+					cliDebugger->backend->printf(cliDebugger->backend, "Hit interrupt at at 0x%08X\n", info->address);
+				} else {
+					cliDebugger->backend->printf(cliDebugger->backend, "Hit function call at at 0x%08X\n", info->address);
+				}
 			} else {
 				cliDebugger->backend->printf(cliDebugger->backend, "Hit function return at at 0x%08X\n", info->address);
 			}