From ef282eee475d8ed48fb33aa12b75f9468a4ac053 Mon Sep 17 00:00:00 2001 From: Jeff Parsons Date: Fri, 6 Nov 2015 14:00:49 -0800 Subject: [PATCH] The Debugger convinced me to make probeAddr() safe for any 1/2/4-byte read --- modules/pcjs/lib/debugger.js | 53 ++++++++++++++++++++++++++++-------- modules/pcjs/lib/x86cpu.js | 49 +++++++++++++++++++-------------- 2 files changed, 69 insertions(+), 33 deletions(-) diff --git a/modules/pcjs/lib/debugger.js b/modules/pcjs/lib/debugger.js index 6e4f06df3..dca24fd8b 100644 --- a/modules/pcjs/lib/debugger.js +++ b/modules/pcjs/lib/debugger.js @@ -2060,10 +2060,7 @@ if (DEBUGGER) { */ Debugger.prototype.getWord = function(dbgAddr, fAdvance) { - if (!dbgAddr.fData32) { - return this.getShort(dbgAddr, fAdvance? 2 : 0); - } - return this.getLong(dbgAddr, fAdvance? 4 : 0); + return dbgAddr.fData32? this.getLong(dbgAddr, fAdvance? 4 : 0) : this.getShort(dbgAddr, fAdvance? 2 : 0); }; /** @@ -2107,6 +2104,9 @@ if (DEBUGGER) { /** * setByte(dbgAddr, b, inc) * + * WARNING: Be careful with the editing commands that use function, because we don't have a safe + * counterpart to cpu.probeAddr(). + * * @this {Debugger} * @param {DbgAddr} dbgAddr * @param {number} b @@ -2125,6 +2125,9 @@ if (DEBUGGER) { /** * setShort(dbgAddr, w, inc) * + * WARNING: Be careful with the editing commands that use function, because we don't have a safe + * counterpart to cpu.probeAddr(). + * * @this {Debugger} * @param {DbgAddr} dbgAddr * @param {number} w @@ -3360,7 +3363,7 @@ if (DEBUGGER) { i += sReplace.length; continue; } - $i++; + i++; } /* * Replace every ^XXXX:XXXX, where XXXX:XXXX is a segmented address, with the FCB filename stored at that address. @@ -3856,7 +3859,13 @@ if (DEBUGGER) { sStopped += " ("; if (this.checksEnabled()) { sStopped += this.cOpcodes + " opcodes, "; - this.cOpcodes = this.cOpcodesStart = 0; + /* + * $ops displays progress by calculating cOpcodes - cOpcodesStart, so before + * zeroing cOpcodes, we should subtract cOpcodes from cOpcodesStart (since we're + * effectively subtracting cOpcodes from cOpcodes as well). + */ + this.cOpcodesStart -= this.cOpcodes; + this.cOpcodes = 0; } sStopped += this.nCycles + " cycles, " + msTotal + " ms, " + nCyclesPerSecond + " hz)"; if (MAXDEBUG && this.chipset) { @@ -6300,7 +6309,7 @@ if (DEBUGGER) { * Displays information about the given software interrupt (assuming that said interrupt is in progress). * * These messages also reset the system variable $ops (by updating cOpcodesStart), to make it easier to see - * how many opcodes were executed since the interrupt "started". + * how many opcodes were executed since these interrupts "started". * * @this {Debugger} * @param {string|undefined} sInt @@ -7151,6 +7160,9 @@ if (DEBUGGER) { /** * getCall(dbgAddr, fFar) * + * Given a possible return address (typically from the stack), look for a matching CALL (or INT) that + * immediately precedes that address. + * * @this {Debugger} * @param {DbgAddr} dbgAddr * @param {boolean} [fFar] @@ -7167,8 +7179,18 @@ if (DEBUGGER) { dbgAddr.addr = null; var s = this.getInstruction(dbgAddr); if (s.indexOf("CALL") > 0 || fFar && s.indexOf("INT") > 0) { - sCall = s; - break; + /* + * Verify that the length of this CALL (or INT), when added to the address of the CALL (or INT), + * matches the original return address. We do this by getting the string index of the opcode bytes, + * subtracting that from the string index of the next space, and dividing that difference by two, + * to yield the length of the CALL (or INT) instruction, in bytes. + */ + var i = s.indexOf(' '); + var j = s.indexOf(' ', i+1); + if (off + (j - i - 1)/2 == offOrig) { + sCall = s; + break; + } } } off--; @@ -7202,7 +7224,7 @@ if (DEBUGGER) { this.println("stack trace for " + this.toHexAddr(dbgAddrStack)); while (cFrames < nFrames) { - var sCall = null, cTests = 256; + var sCall = null, sCallPrev = null, cTests = 256; while ((dbgAddrStack.off >>> 0) < (this.cpu.regLSPLimit >>> 0)) { dbgAddrCall.off = this.getWord(dbgAddrStack, true); /* @@ -7225,14 +7247,21 @@ if (DEBUGGER) { break; } } - if (!sCall) break; + /* + * The sCallPrev check eliminates duplicate sequential calls, which are usually (but not always) + * indicative of a false positive, in which case the previous call is probably bogus as well, but + * at least we won't duplicate that mistake. Of course, there are always exceptions, recursion + * being one of them, but it's rare that we're debugging recursive code. + */ + if (!sCall || sCall == sCallPrev) break; var sSymbol = null; if (sCmd == "ks") { var a = sCall.match(/[0-9A-F]+$/); if (a) sSymbol = this.doList(a[0]); } - sCall = str.pad(sCall, 50) + " ;" + (sSymbol || "stack=" + this.toHexAddr(dbgAddrStack) + " return=" + this.toHexAddr(dbgAddrCall)); + sCall = str.pad(sCall, 50) + " ;" + (sSymbol || "stack=" + this.toHexAddr(dbgAddrStack)); // + " return=" + this.toHexAddr(dbgAddrCall)); this.println(sCall); + sCallPrev = sCall; cFrames++; } if (!cFrames) this.println("no return addresses found"); diff --git a/modules/pcjs/lib/x86cpu.js b/modules/pcjs/lib/x86cpu.js index 6ee3cb466..c4f0ef66b 100644 --- a/modules/pcjs/lib/x86cpu.js +++ b/modules/pcjs/lib/x86cpu.js @@ -2901,44 +2901,51 @@ X86CPU.prototype.setBinding = function(sHTMLType, sBinding, control) * functions, like fnFaultMessage(), that must also avoid triggering faults, since they're not part of * standard CPU operation. * - * NOTE: If the size parameter is used, then the caller is required to provide a valid size (1, 2 or 4) - * and ensure that the data is contained entirely with the requested block (which we assert below). + * Since originally written, I've also relaxed the requirement that the request be contained entirely + * within a single block; this was never a problem for any size-aligned request, but unfortunately, it + * was difficult for the Debugger to guarantee that every 2 or 4-byte request would be always be word or + * dword-aligned. So now requests that straddle blocks will be broken into smaller probeAddr() requests. * * @this {X86CPU} * @param {number} addr is a linear address - * @param {number} [size] is a length (default is 1) + * @param {number} [size] is a length (default is 1; if specified, must be 1, 2 or 4) * @param {boolean} [fLinear] (true for linear probe, false for physical; linear is the default) - * @return {number|null} byte (8-bit) value at that address, or null if invalid + * @return {number|null} value at the specified address, or null if invalid */ X86CPU.prototype.probeAddr = function(addr, size, fLinear) { - var aBlocks = fLinear === false? this.aBusBlocks : this.aMemBlocks; + var aBlocks = (fLinear === false? this.aBusBlocks : this.aMemBlocks); var block = aBlocks[(addr & this.nMemMask) >>> this.nBlockShift]; - if (block && block.type == Memory.TYPE.UNPAGED) { - block = this.mapPageBlock(addr, false, true); - } + if (block && block.type == Memory.TYPE.UNPAGED) block = this.mapPageBlock(addr, false, true); + if (block) { var off = addr & this.nBlockLimit; - /* - * TODO: We actually hit this assert in rare cases where the Debugger is disassembling - * an instruction straddling a page boundary that also references a short or long operand. - * The best solution is to change the Debugger's getShort(), getLong(), etc, functions to - * use getByte() internally, which in turn will never call probeAddr() with a size > 1. - */ - this.assert(off + (size || 1) <= this.nBlockSize); - switch(size) { - default: + if (!size || size == 1) { return block.readByteDirect(off, addr); - case 2: - return block.readShortDirect(off, addr); - case 4: - return block.readLongDirect(off, addr); + } + if (size == 2) { + if (off < this.nBlockLimit) { + return block.readShortDirect(off, addr); + } + return block.readByteDirect(off, addr) | (this.probeAddr(addr + 1, 1, fLinear) << 8); + } + if (size == 4) { + if (off < this.nBlockLimit - 2) { + return block.readLongDirect(off, addr); + } + if (off == this.nBlockLimit - 1) { + return block.readShortDirect(off, addr) | (this.probeAddr(addr + 2, 2, fLinear) << 16); + } + return block.readByteDirect(off, addr) | (this.probeAddr(addr + 1, 1, fLinear) << 8) | (this.probeAddr(addr + 2, 1, fLinear) << 16) | (this.probeAddr(addr + 3, 1, fLinear) << 24); } } + /* * Since the Bus component initializes all unused portions of physical address space with an empty * block, we have also written mapPageBlock() to return an empty block (memEmpty) whenever there is * no valid mapping. So if we ever end up here, this may represent a hole that needs plugging. + * + * It's also possible the caller passed a bogus parameter, such as an invalid size (must be 1, 2 or 4). */ this.assert(false); return null;