diff --git a/Common/Buffer.cpp b/Common/Buffer.cpp index a7e02df8b0..74ec5b747d 100644 --- a/Common/Buffer.cpp +++ b/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; diff --git a/Common/Log.cpp b/Common/Log.cpp index 183a1721f7..f95bb7549c 100644 --- a/Common/Log.cpp +++ b/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; diff --git a/Common/LoongArch64Emitter.cpp b/Common/LoongArch64Emitter.cpp index 8ceedcdf75..bd1ab4f1c3 100644 --- a/Common/LoongArch64Emitter.cpp +++ b/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)); diff --git a/Common/StringUtils.cpp b/Common/StringUtils.cpp index 4bf4d1cdaa..23bb1626a3 100644 --- a/Common/StringUtils.cpp +++ b/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(); } diff --git a/Core/Core.cpp b/Core/Core.cpp index d37365d54b..84fbeca516 100644 --- a/Core/Core.cpp +++ b/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); diff --git a/Core/Debugger/Breakpoints.cpp b/Core/Debugger/Breakpoints.cpp index bcdc07ec3d..999203bc65 100644 --- a/Core/Debugger/Breakpoints.cpp +++ b/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) { diff --git a/Core/Debugger/Breakpoints.h b/Core/Debugger/Breakpoints.h index 9713d1b1e1..ad28b42682 100644 --- a/Core/Debugger/Breakpoints.h +++ b/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 anyMemChecks_; std::atomic regBreakpointMask_; + bool SkipPauseAt(u32 pc) const; + bool SkipReportAt(u32 pc) const; + void NoteStoppedOnReport(u32 pc); + std::vector 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 memChecks_; std::vector memCheckRangesRead_; diff --git a/Core/MIPS/IR/IRFrontend.cpp b/Core/MIPS/IR/IRFrontend.cpp index f90ad5835e..9373319494 100644 --- a/Core/MIPS/IR/IRFrontend.cpp +++ b/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) { diff --git a/Core/MIPS/IR/IRInterpreter.cpp b/Core/MIPS/IR/IRInterpreter.cpp index 5ff0593c11..a5686aa448 100644 --- a/Core/MIPS/IR/IRInterpreter.cpp +++ b/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; diff --git a/Core/MIPS/MIPSTables.cpp b/Core/MIPS/MIPSTables.cpp index 7688e6e54b..d9a5221f42 100644 --- a/Core/MIPS/MIPSTables.cpp +++ b/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); diff --git a/Core/TiltEventProcessor.cpp b/Core/TiltEventProcessor.cpp index ab093a578f..279fc6ff84 100644 --- a/Core/TiltEventProcessor.cpp +++ b/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; diff --git a/unittest/UnitTest.cpp b/unittest/UnitTest.cpp index a230e5aeaf..90ec581f63 100644 --- a/unittest/UnitTest.cpp +++ b/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; }