The Debugger convinced me to make probeAddr() safe for any 1/2/4-byte read

modules/pcjs/lib/debugger.js

@@ -2060,10 +2060,7 @@ if (DEBUGGER) {
      */
     Debugger.prototype.getWord = function(dbgAddr, fAdvance)
     {
-        if (!dbgAddr.fData32) {
+        return dbgAddr.fData32? this.getLong(dbgAddr, fAdvance? 4 : 0) : this.getShort(dbgAddr, fAdvance? 2 : 0);
-            return this.getShort(dbgAddr, fAdvance? 2 : 0);
-        }
-        return this.getLong(dbgAddr, fAdvance? 4 : 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");

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)
+ * Since originally written, I've also relaxed the requirement that the request be contained entirely
- * and ensure that the data is contained entirely with the requested block (which we assert below).
+ * 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) {
+    if (block && block.type == Memory.TYPE.UNPAGED) block = this.mapPageBlock(addr, false, true);
-        block = this.mapPageBlock(addr, false, true);
+
-    }
     if (block) {
         var off = addr & this.nBlockLimit;
-        /*
+        if (!size || size == 1) {
-         * 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:
             return block.readByteDirect(off, addr);
-        case 2:
+        }
-            return block.readShortDirect(off, addr);
+        if (size == 2) {
-        case 4:
+            if (off < this.nBlockLimit) {
-            return block.readLongDirect(off, addr);
+                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;