Merge pull request #22612 from hrydgard/misc-fixes

Assorted fixes picked from various unmerged branches

Common/Buffer.cpp

@@ -136,7 +136,9 @@ bool Buffer::FlushToFile(const Path &filename, bool clear) {
 }
 
 void Buffer::PeekAll(std::string *dest) {
-	dest->resize(data_.size());
+	// resize() here would leave that many zero bytes in front of the appended data.
+	dest->clear();
+	dest->reserve(data_.size());
 	data_.iterate_blocks(([dest](const char *blockData, size_t blockSize) {
 		dest->append(blockData, blockSize);
 		return true;

Common/Log.cpp

@@ -136,8 +136,9 @@ bool HandleAssert(bool isDebugAssert, const char *function, const char *file, in
 		const char *threadName = GetCurrentThreadName();
 		OutputDebugStringA(formatted);
 		printf("%s\n", formatted);
-		static const std::string caption = isDebugAssert ? "Debug Assert" : "Critical Assert";
-		std::wstring wcaption = ConvertUTF8ToWString(caption + " " + (threadName ? threadName : "(unknown thread)"));
+		// Not static - it'd freeze at whatever the first assert in the session was.
+		const char *caption = isDebugAssert ? "Debug Assert" : "Critical Assert";
+		std::wstring wcaption = ConvertUTF8ToWString(std::string(caption) + " " + (threadName ? threadName : "(unknown thread)"));
 		switch (MessageBox(g_dialogParent, ConvertUTF8ToWString(text).c_str(), wcaption.c_str(), msgBoxStyle)) {
 		case IDYES:
 			return true;

Common/LoongArch64Emitter.cpp

@@ -1769,8 +1769,9 @@ void LoongArch64Emitter::SetJumpTarget(FixupBranch &branch, const void *dst) {
 		memcpy(writableSrc, &fixup, sizeof(u32));
 		break;
 
-    case FixupBranchType::BZ:
-		_assert_msg_(BranchInRange(branch.ptr, dst), "B destination is too far away (%p -> %p)", branch.ptr, dst);
+	case FixupBranchType::BZ:
+		// BZ encodes 21 bits of offset, not 16 - the direct BEQZ/BNEZ check it this way too.
+		_assert_msg_(BranchZeroInRange(branch.ptr, dst), "BZ destination is too far away (%p -> %p)", branch.ptr, dst);
 		memcpy(&fixup, writableSrc, sizeof(u32));
 		fixup = (fixup & 0xFC0003E0) | EncodeJSd5k16(Opcode32::ZERO, R_ZERO, (s32)distance);
 		memcpy(writableSrc, &fixup, sizeof(u32));

Common/StringUtils.cpp

@@ -86,7 +86,11 @@ long parseLong(std::string s) {
 }
 
 bool containsNoCase(std::string_view haystack, std::string_view needle) {
-	auto pred = [](char ch1, char ch2) { return std::toupper(ch1) == std::toupper(ch2); };
+	// toupper is UB for negative values other than EOF, and char is signed here - non-ASCII bytes
+	// reach this from e.g. ELF symbol names.
+	auto pred = [](char ch1, char ch2) {
+		return std::toupper((unsigned char)ch1) == std::toupper((unsigned char)ch2);
+	};
 	auto found = std::search(haystack.begin(), haystack.end(), needle.begin(), needle.end(), pred);
 	return found != haystack.end();
 }

Core/Core.cpp

@@ -687,6 +687,7 @@ void Core_Break(BreakReason reason, u32 relatedAddress, const BreakpointHit *hit
 		// later at an address nobody is waiting for anymore. Same as gdb dropping its step-resume
 		// breakpoint, or lldb discarding the thread plan, on any stop.
 		g_breakpoints.ClearTempBreakPoint();
+		g_breakpoints.ClearSkipFirst();
 
 		// Same reasoning for a cpu.runUntilTime deadline - it belonged to the run that just ended.
 		CoreTiming::SetBreakDeadlineUs(0);

Core/Debugger/Breakpoints.cpp

@@ -61,9 +61,10 @@ BreakAction MemCheck::Apply(u32 addr, bool write, int size, u32 pc) {
 	return BREAK_ACTION_NONE;
 }
 
-BreakAction MemCheck::Action(u32 addr, bool write, int size, u32 pc, const char *reason) {
+BreakAction MemCheck::Action(u32 addr, bool write, int size, u32 pc, const char *reason, bool skipPause) {
 	// Conditions have always already been checked if we get here.
 	Log(addr, write, size, pc, reason);
+	const BreakAction action = skipPause ? BreakAction(this->action & ~BREAK_ACTION_PAUSE) : this->action;
 
 	BreakpointHit hit;
 	if (WebSocketDebuggerHasClients() || (action & BREAK_ACTION_PAUSE)) {
@@ -358,11 +359,15 @@ BreakAction BreakpointManager::ExecBreakPoint(u32 addr) {
 
 	BreakAction result = BREAK_ACTION_NONE;
 	BreakpointHit hit;
+	const bool skipPause = SkipPauseAt(addr);
 
 	size_t bp = FindBreakpoint(addr);
-	if (bp != INVALID_BREAKPOINT) {
+	if (bp != INVALID_BREAKPOINT && !SkipReportAt(addr)) {
 		BreakPoint &info = breakPoints_[bp];
-		const BreakAction action = info.action;
+		BreakAction action = info.action;
+		if (skipPause) {
+			action = BreakAction(action & ~BREAK_ACTION_PAUSE);
+		}
 
 		bool condPassed = true;
 		if (info.hasCond)
@@ -404,7 +409,7 @@ BreakAction BreakpointManager::ExecBreakPoint(u32 addr) {
 		}
 	}
 
-	if (tempBreakPoint_.valid && tempBreakPoint_.addr == addr) {
+	if (tempBreakPoint_.valid && tempBreakPoint_.addr == addr && !skipPause) {
 		// The condition, when set, narrows down which hit counts - to one thread for a step
 		// ("threadid == ..."), or to a later frame for run-to-cursor ("flipcount > ...").  A hit that
 		// fails it leaves the breakpoint armed, so the next one gets a chance.
@@ -418,6 +423,9 @@ BreakAction BreakpointManager::ExecBreakPoint(u32 addr) {
 		// hit stays kind None when only the temporary breakpoint fired - there's no user
 		// breakpoint to describe in that case, just a step completing.
 		Core_Break(BreakReason::CpuBreakpoint, addr, hit.kind != BreakpointKind::None ? &hit : nullptr);
+		if (hit.kind != BreakpointKind::None) {
+			NoteStoppedOnReport(addr);
+		}
 		System_Notify(SystemNotification::DISASSEMBLY);
 	}
 
@@ -577,7 +585,7 @@ MemCheck *BreakpointManager::FindMemCheckInRange(u32 address, int size) {
 
 BreakAction BreakpointManager::ExecMemCheck(u32 address, bool write, int size, u32 pc, const char *reason)
 {
-	if (!anyMemChecks_)
+	if (!anyMemChecks_ || SkipReportAt(pc))
 		return BREAK_ACTION_NONE;
 	MemCheck *check = FindMemCheckInRange(address, size);
 	if (check) {
@@ -586,7 +594,11 @@ BreakAction BreakpointManager::ExecMemCheck(u32 address, bool write, int size, u
 			return applyAction;
 
 		MemCheck copy = *check;
-		return copy.Action(address, write, size, pc, reason);
+		const BreakAction result = copy.Action(address, write, size, pc, reason, SkipPauseAt(pc));
+		if (result & BREAK_ACTION_PAUSE) {
+			NoteStoppedOnReport(pc);
+		}
+		return result;
 	}
 	return BREAK_ACTION_NONE;
 }
@@ -594,6 +606,11 @@ BreakAction BreakpointManager::ExecMemCheck(u32 address, bool write, int size, u
 BreakAction BreakpointManager::ExecOpMemCheck(u32 address, u32 pc) {
 	// Note: currently, we don't check "on changed" for HLE (ExecMemCheck.)
 	// We'd need to more carefully specify memory changes in HLE for that.
+	// pc may be the delay slot, but the skip is keyed on where execution resumes, which is the branch.
+	const u32 execPc = currentMIPS->pc;
+	if (SkipReportAt(execPc)) {
+		return BREAK_ACTION_NONE;
+	}
 	int size = MIPSAnalyst::OpMemoryAccessSize(pc);
 	if (size == 0 && MIPSAnalyst::OpHasDelaySlot(pc)) {
 		// This means that the delay slot is what tripped us.
@@ -619,7 +636,11 @@ BreakAction BreakpointManager::ExecOpMemCheck(u32 address, u32 pc) {
 				return applyAction;
 
 			MemCheck copy = *check;
-			return copy.Action(address, write, size, pc, "CPU");
+			const BreakAction result = copy.Action(address, write, size, pc, "CPU", SkipPauseAt(execPc));
+			if (result & BREAK_ACTION_PAUSE) {
+				NoteStoppedOnReport(execPc);
+			}
+			return result;
 		}
 	}
 	return BREAK_ACTION_NONE;
@@ -752,23 +773,28 @@ BreakAction BreakpointManager::ExecRegBreakpoint(int reg, u32 pc) {
 	if (info.hasCond && !info.cond.Evaluate())
 		return BREAK_ACTION_NONE;
 
+	if (SkipReportAt(pc)) {
+		return BREAK_ACTION_NONE;
+	}
+	const BreakAction result = SkipPauseAt(pc) ? BreakAction(info.result & ~BREAK_ACTION_PAUSE) : info.result;
+
 	++info.numHits;
 
 	BreakpointHit hit;
-	if (WebSocketDebuggerHasClients() || (info.result & BREAK_ACTION_PAUSE)) {
+	if (WebSocketDebuggerHasClients() || (result & BREAK_ACTION_PAUSE)) {
 		hit.kind = BreakpointKind::Register;
 		hit.pc = pc;
 		hit.address = pc;
 		hit.reg = reg;
 		hit.numHits = info.numHits;
-		hit.logged = (info.result & BREAK_ACTION_LOG) != 0;
-		hit.paused = (info.result & BREAK_ACTION_PAUSE) != 0;
+		hit.logged = (result & BREAK_ACTION_LOG) != 0;
+		hit.paused = (result & BREAK_ACTION_PAUSE) != 0;
 		if (info.hasCond)
 			hit.condition = info.cond.expressionString;
 		WebSocketNotifyBreakpointHit(hit);
 	}
 
-	if (info.result & BREAK_ACTION_LOG) {
+	if (result & BREAK_ACTION_LOG) {
 		if (info.logFormat.empty()) {
 			NOTICE_LOG(Log::JIT, "BKP reg write r%d, PC=%08x (%s)", reg, pc, g_symbolMap->GetDescription(pc).c_str());
 		} else {
@@ -777,28 +803,47 @@ BreakAction BreakpointManager::ExecRegBreakpoint(int reg, u32 pc) {
 			NOTICE_LOG(Log::JIT, "BKP reg write r%d, PC=%08x: %s", reg, pc, formatted.c_str());
 		}
 	}
-	if ((info.result & BREAK_ACTION_PAUSE) && g_breakpoints.CheckSkipFirst() != pc) {
+	if (result & BREAK_ACTION_PAUSE) {
 		Core_Break(BreakReason::RegBreakpoint, pc, &hit);
+		NoteStoppedOnReport(pc);
 	}
 
-	return info.result;
+	return result;
 }
 
 void BreakpointManager::ClearSkipFirst() {
-	breakSkipFirstAt_ = 0;
-	breakSkipFirstTicks_ = 0;
+	skipFirstValid_ = false;
+	stoppedOnReport_ = false;
 }
 
 void BreakpointManager::SetSkipFirst(u32 pc) {
-	breakSkipFirstAt_ = pc;
-	breakSkipFirstTicks_ = CoreTiming::GetTicks(currentMIPS);
+	const u64 ticks = CoreTiming::GetTicks(currentMIPS);
+	// A step-over arms its temporary breakpoint and then resumes, which comes through here again.
+	if (skipFirstValid_ && skipFirstAt_ == pc && skipFirstTicks_ == ticks) {
+		return;
+	}
+	skipFirstValid_ = true;
+	skipFirstAt_ = pc;
+	skipFirstTicks_ = ticks;
+	// Consumed here, since a plain step stops again without going through Core_Break().
+	skipFirstReport_ = stoppedOnReport_ && stoppedOnReportAt_ == pc;
+	if (skipFirstReport_) {
+		stoppedOnReport_ = false;
+	}
 }
 
-u32 BreakpointManager::CheckSkipFirst() const {
-	u32 pc = breakSkipFirstAt_;
-	if (breakSkipFirstTicks_ == CoreTiming::GetTicks(currentMIPS))
-		return pc;
-	return 0;
+bool BreakpointManager::SkipPauseAt(u32 pc) const {
+	return skipFirstValid_ && skipFirstAt_ == pc && skipFirstTicks_ == CoreTiming::GetTicks(currentMIPS);
+}
+
+bool BreakpointManager::SkipReportAt(u32 pc) const {
+	return skipFirstReport_ && SkipPauseAt(pc);
+}
+
+// Call after Core_Break(), which clears it.
+void BreakpointManager::NoteStoppedOnReport(u32 pc) {
+	stoppedOnReport_ = true;
+	stoppedOnReportAt_ = pc;
 }
 
 static MemCheck NotCached(MemCheck mc) {

Core/Debugger/Breakpoints.h

@@ -125,7 +125,7 @@ struct MemCheck {
 	// Called on the stored memcheck (affects numHits, etc.)
 	BreakAction Apply(u32 addr, bool write, int size, u32 pc);
 	// Called on a copy.
-	BreakAction Action(u32 addr, bool write, int size, u32 pc, const char *reason);
+	BreakAction Action(u32 addr, bool write, int size, u32 pc, const char *reason, bool skipPause);
 
 	void Log(u32 addr, bool write, int size, u32 pc, const char *reason) const;
 
@@ -251,8 +251,10 @@ public:
 	// instruction that would write to reg - does not itself execute the instruction.
 	BreakAction ExecRegBreakpoint(int reg, u32 pc);
 
+	// Call when starting to run or step from pc. Breakpoints at pc then don't pause until the
+	// instruction there has run, and if one of them is what stopped us, it doesn't report again.
 	void SetSkipFirst(u32 pc);
-	u32 CheckSkipFirst() const;
+	// Execution stopped, or jumped (reset, savestate load).
 	void ClearSkipFirst();
 
 	// Includes uncached addresses.
@@ -301,10 +303,24 @@ private:
 	std::atomic<bool> anyMemChecks_;
 	std::atomic<u32> regBreakpointMask_;
 
+	bool SkipPauseAt(u32 pc) const;
+	bool SkipReportAt(u32 pc) const;
+	void NoteStoppedOnReport(u32 pc);
+
 	std::vector<BreakPoint> breakPoints_;
 	TempBreakPoint tempBreakPoint_;
-	u32 breakSkipFirstAt_ = 0;
-	u64 breakSkipFirstTicks_ = 0;
+	// GetTicks() only moves when an instruction retires, so (pc, ticks) stops matching as soon as
+	// the instruction at pc has run, and a loop back to pc is a new execution.
+	bool skipFirstValid_ = false;
+	u32 skipFirstAt_ = 0;
+	u64 skipFirstTicks_ = 0;
+	// Set when the breakpoint we're resuming from is the one that stopped us, and so already
+	// logged and counted this execution.
+	bool skipFirstReport_ = false;
+	// The instruction whose breakpoint stopped us. Only the address: inside a JIT block the tick
+	// count isn't settled yet, so SetSkipFirst() pairs it with the ticks once we've stopped.
+	bool stoppedOnReport_ = false;
+	u32 stoppedOnReportAt_ = 0;
 
 	std::vector<MemCheck> memChecks_;
 	std::vector<MemCheck> memCheckRangesRead_;

Core/MIPS/IR/IRFrontend.cpp

@@ -41,7 +41,7 @@ IRFrontend::IRFrontend(bool startDefaultPrefix) {
 
 	// The debugger sets this so that "go" on a breakpoint will actually... go.
 	// But if they reset, we can end up hitting it by mistake, since it's based on PC and ticks.
-	g_breakpoints.SetSkipFirst(0);
+	g_breakpoints.ClearSkipFirst();
 }
 
 void IRFrontend::DoState(PointerWrap &p) {
@@ -61,7 +61,7 @@ void IRFrontend::DoState(PointerWrap &p) {
 
 	// The debugger sets this so that "go" on a breakpoint will actually... go.
 	// But if they reset, we can end up hitting it by mistake, since it's based on PC and ticks.
-	g_breakpoints.SetSkipFirst(0);
+	g_breakpoints.ClearSkipFirst();
 }
 
 bool IRFrontend::UpdateFCR31(u32 fcr31) {

Core/MIPS/IR/IRInterpreter.cpp

@@ -53,11 +53,6 @@ alignas(16) static const uint32_t lowBytesMask[4] = {
 };
 
 u32 IRRunBreakpoint(u32 pc) {
-	// Should we skip this breakpoint?
-	uint32_t skipFirst = g_breakpoints.CheckSkipFirst();
-	if (skipFirst == pc || skipFirst == currentMIPS->pc)
-		return 0;
-
 	// Did we already hit one?
 	if (coreState != CORE_RUNNING_CPU && coreState != CORE_NEXTFRAME)
 		return 1;
@@ -67,11 +62,6 @@ u32 IRRunBreakpoint(u32 pc) {
 }
 
 u32 IRRunMemCheck(u32 pc, u32 addr) {
-	// Should we skip this breakpoint?
-	uint32_t skipFirst = g_breakpoints.CheckSkipFirst();
-	if (skipFirst == pc || skipFirst == currentMIPS->pc)
-		return 0;
-
 	// Did we already hit one?
 	if (coreState != CORE_RUNNING_CPU && coreState != CORE_NEXTFRAME)
 		return 1;

Core/MIPS/MIPSTables.cpp

@@ -1297,7 +1297,7 @@ static inline int GetGPRWriteTarget(const MIPSInstruction *instr, MIPSOpcode op)
 }
 
 static bool CheckExecBreakpoints(const MIPSState *mips, u32 pc) {
-	if (g_breakpoints.HasBreakPoints() && g_breakpoints.NeedsBreakCheckAt(pc) && g_breakpoints.CheckSkipFirst() != mips->pc) {
+	if (g_breakpoints.HasBreakPoints() && g_breakpoints.NeedsBreakCheckAt(pc)) {
 		g_breakpoints.ExecBreakPoint(pc);
 		// No temp-breakpoint cleanup needed here (the JIT path never had any either) - Core_Break()
 		// drops it for us on the way into stepping, whichever breakpoint it was that stopped us.
@@ -1309,7 +1309,7 @@ static bool CheckExecBreakpoints(const MIPSState *mips, u32 pc) {
 
 // instr must not be null - the callers skip these checks for undecodable opcodes.
 static bool CheckMemBreakpoints(const MIPSState *mips, const MIPSInstruction *instr, const MIPSOpcode op) {
-	if ((instr->flags & (IN_MEM | OUT_MEM)) != 0 && g_breakpoints.CheckSkipFirst() != mips->pc && instr->interpret != &Int_Syscall) {
+	if ((instr->flags & (IN_MEM | OUT_MEM)) != 0 && instr->interpret != &Int_Syscall) {
 		// This is common for all IN_MEM/OUT_MEM funcs.
 		int offset = (instr->flags & IS_VFPU) != 0 ? SignExtend16ToS32(op & 0xFFFC) : SignExtend16ToS32(op);
 		u32 addr = (mips->r[_RS(op)] + offset) & 0xFFFFFFFC;
@@ -1327,7 +1327,7 @@ static bool CheckMemBreakpoints(const MIPSState *mips, const MIPSInstruction *in
 }
 
 static bool CheckRegBreakpoints(const MIPSState *mips, const MIPSInstruction *instr, const MIPSOpcode op, u32 regBPMask) {
-	if ((instr->flags & (OUT_RT | OUT_RD | OUT_RA)) != 0 && g_breakpoints.CheckSkipFirst() != mips->pc) {
+	if ((instr->flags & (OUT_RT | OUT_RD | OUT_RA)) != 0) {
 		int regTarget = GetGPRWriteTarget(instr, op);
 		if (regTarget >= 0 && (regBPMask & (1u << regTarget)) != 0) {
 			g_breakpoints.ExecRegBreakpoint(regTarget, mips->pc);

Core/TiltEventProcessor.cpp

@@ -55,6 +55,13 @@ inline float ApplyDeadzoneAxis(float x, float deadzone) {
 
 inline void ApplyDeadzoneXY(float x, float y, float *adjustedX, float *adjustedY, float deadzone, bool circular) {
 	if (circular) {
+		if (deadzone >= 0.99f) {
+			// Same guard as ApplyDeadzoneAxis - without it the factor below divides by ~0.
+			*adjustedX = x;
+			*adjustedY = y;
+			return;
+		}
+
 		if (x == 0.0f && y == 0.0f) {
 			*adjustedX = 0.0f;
 			*adjustedY = 0.0f;

unittest/UnitTest.cpp

@@ -903,6 +903,55 @@ bool TestTempBreakpoints() {
 	g_breakpoints.RemoveBreakPoint(kAddrA);
 	EXPECT_FALSE(g_breakpoints.HasBreakPoints());
 
+	// Resuming from a breakpoint must not pause there again, and must not report it again if it's
+	// the one that stopped us. A breakpoint we merely stepped onto still has to log and count.
+	// Nothing retires an instruction here unless we move downcount, so the ticks stay put.
+	{
+		auto hitsAt = [](u32 addr) -> int {
+			for (const BreakPoint &bp : g_breakpoints.GetBreakpoints()) {
+				if (bp.addr == addr)
+					return (int)bp.numHits;
+			}
+			return -1;
+		};
+
+		g_breakpoints.ClearSkipFirst();
+		g_breakpoints.AddBreakPoint(kAddrA);
+		g_breakpoints.AddBreakPoint(kAddrB);
+
+		EXPECT_TRUE((g_breakpoints.ExecBreakPoint(kAddrA) & BREAK_ACTION_PAUSE) != 0);
+		EXPECT_EQ_INT(hitsAt(kAddrA), 1);
+
+		// Resuming off it. A step-over sets the skip twice for one resume, and that must not lose it.
+		g_breakpoints.SetSkipFirst(kAddrA);
+		g_breakpoints.SetSkipFirst(kAddrA);
+		EXPECT_EQ_INT((int)g_breakpoints.ExecBreakPoint(kAddrA), (int)BREAK_ACTION_NONE);
+		EXPECT_EQ_INT(hitsAt(kAddrA), 1);
+
+		// Stepped onto B, never reported there: it logs and counts, only the pause is dropped.
+		g_breakpoints.SetSkipFirst(kAddrB);
+		EXPECT_EQ_INT((int)(g_breakpoints.ExecBreakPoint(kAddrB) & BREAK_ACTION_PAUSE), 0);
+		EXPECT_EQ_INT(hitsAt(kAddrB), 1);
+
+		// An unrelated address still pauses and reports.
+		EXPECT_TRUE((g_breakpoints.ExecBreakPoint(kAddrA) & BREAK_ACTION_PAUSE) != 0);
+		EXPECT_EQ_INT(hitsAt(kAddrA), 2);
+
+		// Step off A, which runs it, and land on A again (a one-instruction loop). That's a new
+		// execution, which nothing has reported yet.
+		g_breakpoints.SetSkipFirst(kAddrA);
+		EXPECT_EQ_INT((int)g_breakpoints.ExecBreakPoint(kAddrA), (int)BREAK_ACTION_NONE);
+		currentMIPS->downcount -= 1;
+		g_breakpoints.SetSkipFirst(kAddrA);
+		EXPECT_EQ_INT((int)(g_breakpoints.ExecBreakPoint(kAddrA) & BREAK_ACTION_PAUSE), 0);
+		EXPECT_EQ_INT(hitsAt(kAddrA), 3);
+		currentMIPS->downcount += 1;
+
+		g_breakpoints.ClearSkipFirst();
+		g_breakpoints.RemoveBreakPoint(kAddrA);
+		g_breakpoints.RemoveBreakPoint(kAddrB);
+	}
+
 	g_symbolMap = nullptr;
 	return true;
 }