Fixed a stack-size bug uncovered by a PUSH SS/POP SS sequence in Windows 95 kernel

modules/pcjs/lib/x86cpu.js

@@ -2173,7 +2173,20 @@ X86CPU.prototype.setSS = function(sel, fInterruptable)
     var regESP = this.getSP();
     var regLSP = this.segSS.load(sel);
     if (regLSP !== X86.ADDR_INVALID) {
-        this.regLSP = (regLSP + regESP)|0;
+        /*
+         * The safest way to update regLSP after a potential change to segSS.base is to call setSP()
+         * with the original stack pointer retrieved above via getSP().  When I tried to be clever and
+         * do this instead:
+         *
+         *      this.regLSP = (regLSP + regESP)|0;
+         *
+         * 16-bit stacks began inadvertently using ESP instead of SP.  The moral: don't be needlessly clever.
+         *
+         * Sprinkle the following assert throughout stack operations to catch that bug in the future:
+         *
+         *      this.assert(!((this.regLSP - this.segSS.base) & ~this.segSS.maskAddr));
+         */
+        this.setSP(regESP);
         if (this.segSS.fExpDown) {
             this.regLSPLimit = (this.segSS.base + this.segSS.maskAddr)|0;
             this.regLSPLimitLow = (this.segSS.base + this.segSS.limit)|0;

modules/pcjs/lib/x86func.js

@@ -1439,7 +1439,11 @@ X86.fnINT = function INT(nIDT, nError, nCycles)
  */
 X86.fnIRET = function IRET()
 {
+    /*
+     * As discussed in fnRETF(), we temporarily set opLSP around operations that fnFault() may need to restart.
+     */
     this.opLSP = this.regLSP;
+
     this.nStepCycles -= this.cycleCounts.nOpCyclesIRet;
 
     if ((this.regCR0 & X86.CR0.MSW.PE) && (this.regPS & X86.PS.NT)) {
@@ -2436,6 +2440,7 @@ X86.fnRCRd = function RCRd(dst, src)
 X86.fnRETF = function RETF(n)
 {
     this.opLSP = this.regLSP;
+
     var newIP = this.popWord();
     var newCS = this.popWord();
 
@@ -3916,13 +3921,17 @@ X86.fnFault = function(nFault, nError, fHalt, nCycles)
          * Prior to each new burst of instructions, stepCPU() sets fComplete to true, and the only (normal) way
          * for fComplete to become false is through stopCPU(), which isn't ordinarily called, except by the Debugger.
          */
+        this.resetSizes();
         this.setIP(this.opLIP - this.segCS.base);
     }
     else if (this.model >= X86.MODEL_80186) {
         if (this.nFault < 0) {
             /*
-             * Single-fault (error code is passed through, and the responsible instruction is restartable)
+             * Single-fault (error code is passed through, and the responsible instruction is restartable;
+             * the call to resetSizes() is critical, otherwise setIP() may update IP with the wrong size if
+             * the current instruction contains an OPERAND size override).
              */
+            this.resetSizes();
             this.setIP(this.opLIP - this.segCS.base);
             if (this.opLSP != X86.ADDR_INVALID) {
                 this.setSP((this.regESP & ~this.segSS.maskAddr) | (this.opLSP - this.segSS.base));

modules/pcjs/lib/x86op0f.js

@@ -1100,6 +1100,11 @@ X86.opSETNLE = function SETNLE()
  */
 X86.opPUSHFS = function PUSHFS()
 {
+    /*
+     * TODO: Reportedly, when the OPERAND size is 32 bits, the 80386 will decrement the stack pointer by 4,
+     * write the selector into the 2 lower bytes, and leave the 2 upper bytes untouched, whereas we will write
+     * a 32-bit value, effectively zeroing the 2 upper bytes.  Need to confirm this.
+     */
     this.pushWord(this.segFS.sel);
     this.nStepCycles -= this.cycleCounts.nOpCyclesPushSeg;
 };
@@ -1167,6 +1172,11 @@ X86.opSHLDcl = function SHLDcl()
  */
 X86.opPUSHGS = function PUSHGS()
 {
+    /*
+     * TODO: Reportedly, when the OPERAND size is 32 bits, the 80386 will decrement the stack pointer by 4,
+     * write the selector into the 2 lower bytes, and leave the 2 upper bytes untouched, whereas we will write
+     * a 32-bit value, effectively zeroing the 2 upper bytes.  Need to confirm this.
+     */
     this.pushWord(this.segGS.sel);
     this.nStepCycles -= this.cycleCounts.nOpCyclesPushSeg;
 };

modules/pcjs/lib/x86ops.js

@@ -126,6 +126,11 @@ X86.opADDAX = function ADDAX()
  */
 X86.opPUSHES = function PUSHES()
 {
+    /*
+     * TODO: Reportedly, when the OPERAND size is 32 bits, the 80386 will decrement the stack pointer by 4,
+     * write the selector into the 2 lower bytes, and leave the 2 upper bytes untouched, whereas we will write
+     * a 32-bit value, effectively zeroing the 2 upper bytes.  Need to confirm this.
+     */
     this.pushWord(this.segES.sel);
     this.nStepCycles -= this.cycleCounts.nOpCyclesPushSeg;
 };
@@ -137,6 +142,9 @@ X86.opPUSHES = function PUSHES()
  */
 X86.opPOPES = function POPES()
 {
+    /*
+     * As discussed in fnRETF(), we temporarily set opLSP around operations that fnFault() may need to restart.
+     */
     this.opLSP = this.regLSP;
     this.setES(this.popWord());
     this.nStepCycles -= this.cycleCounts.nOpCyclesPopReg;
@@ -216,6 +224,11 @@ X86.opORAX = function ORAX()
  */
 X86.opPUSHCS = function PUSHCS()
 {
+    /*
+     * TODO: Reportedly, when the OPERAND size is 32 bits, the 80386 will decrement the stack pointer by 4,
+     * write the selector into the 2 lower bytes, and leave the 2 upper bytes untouched, whereas we will write
+     * a 32-bit value, effectively zeroing the 2 upper bytes.  Need to confirm this.
+     */
     this.pushWord(this.segCS.sel);
     this.nStepCycles -= this.cycleCounts.nOpCyclesPushSeg;
 };
@@ -314,6 +327,11 @@ X86.opADCAX = function ADCAX()
  */
 X86.opPUSHSS = function PUSHSS()
 {
+    /*
+     * TODO: Reportedly, when the OPERAND size is 32 bits, the 80386 will decrement the stack pointer by 4,
+     * write the selector into the 2 lower bytes, and leave the 2 upper bytes untouched, whereas we will write
+     * a 32-bit value, effectively zeroing the 2 upper bytes.  Need to confirm this.
+     */
     this.pushWord(this.segSS.sel);
     this.nStepCycles -= this.cycleCounts.nOpCyclesPushSeg;
 };
@@ -325,6 +343,9 @@ X86.opPUSHSS = function PUSHSS()
  */
 X86.opPOPSS = function POPSS()
 {
+    /*
+     * As discussed in fnRETF(), we temporarily set opLSP around operations that fnFault() may need to restart.
+     */
     this.opLSP = this.regLSP;
     this.setSS(this.popWord());
     this.nStepCycles -= this.cycleCounts.nOpCyclesPopReg;
@@ -404,6 +425,11 @@ X86.opSBBAX = function SBBAX()
  */
 X86.opPUSHDS = function PUSHDS()
 {
+    /*
+     * TODO: Reportedly, when the OPERAND size is 32 bits, the 80386 will decrement the stack pointer by 4,
+     * write the selector into the 2 lower bytes, and leave the 2 upper bytes untouched, whereas we will write
+     * a 32-bit value, effectively zeroing the 2 upper bytes.  Need to confirm this.
+     */
     this.pushWord(this.segDS.sel);
     this.nStepCycles -= this.cycleCounts.nOpCyclesPushSeg;
 };
@@ -415,6 +441,9 @@ X86.opPUSHDS = function PUSHDS()
  */
 X86.opPOPDS = function POPDS()
 {
+    /*
+     * As discussed in fnRETF(), we temporarily set opLSP around operations that fnFault() may need to restart.
+     */
     this.opLSP = this.regLSP;
     this.setDS(this.popWord());
     this.nStepCycles -= this.cycleCounts.nOpCyclesPopReg;