emu: Hack in push/pop support for ModR/M. UNSAFE, and maybe UNSOUND.

src/emu/i8088.rs

@@ -104,6 +104,45 @@ macro_rules! step {
     step!(@arg $cookie, $bus)
   };
 
+  // Horrible, Ugly, No Good Hack.
+  //
+  // Enqueue a mutable reference to the bus without telling the borrow
+  // checker, iff the ModR/M byte selects for a register operand. See
+  // comments on push_modrm/pop_modrm in the operations module.
+  //
+  // Safety: Yolo! (Iolo?)
+  //
+  // Well, maybe. The reason we can't just enqueue the bus directly is
+  // because the borrowck can't know statically that the cases where
+  // we take the bus here (ModR/M within 0xC0..=0xC7) are also the
+  // cases where there won't be a FarPtr operand taking a conflicting
+  // bus ref.
+  //
+  // It is up to the caller to make sure that there isn't actually
+  // another reference to the bus, as that would violate aliasing
+  // rules. Of course, there IS another bus reference, the one we're
+  // copying from! So we can't touch that at all for the lifetime of
+  // the reference we're creating here, either.
+  //
+  // Is this enough to be safe? Who knows! Rust hasn't yet actually
+  // defined its aliasing rules or UB and doesn't seem to be in any
+  // hurry to. LLVM talks in terms of "memory access done through a
+  // pointer" which as long as we're not doing anything with the
+  // original pointer while the other one exists sort of suggests that
+  // this should be okay but also doesn't really sound like a
+  // guarantee.
+  (@unsafe_bus_maybe $cookie:tt, $cpu:expr, $bus:expr, $prefix:tt,
+              ($modrm_val:ident, $modrm:tt, $modrm16:tt, $modrm8:tt)) => {
+    step!(@arg $cookie,
+          if (0xC0..=0xC7).contains(&$modrm_val) {
+            // Sneak a mutable reference past the borrow checker.
+            unsafe { ($bus as *mut Bus).as_mut() }
+          } else {
+            None
+          }
+    )
+  };
+
   (@cpu $cookie:tt, $cpu:expr, $bus:expr, $prefix:tt, $modrm:tt) => {
     step!(@arg $cookie, $cpu)
   };
@@ -451,6 +490,7 @@ impl i8088 {
                   0x08 => mov[reg=cs, modrm16] / "2/12+",
                   0x10 => mov[reg=ss, modrm16] / "2/12+",
                   0x18 => mov[reg=ds, modrm16] / "2/12+", },
+          0x8F: { 0x00 => pop_modrm[unsafe_bus_maybe, regval=ss, reg=sp, modrm16] / "12/25+" },  // POP r/m16
           0x90 => nop[] / 3,
           0xA0 => mov[reglo=a, addr] / 14,
           0xA1 => mov[reg=a, addr] / 14,
@@ -493,7 +533,8 @@ impl i8088 {
           0xFE: { 00 => inc[width=byte, flags, modrm8] / "3/23+",      // INC r/m8
                   08 => dec[width=byte, flags, modrm8] / "3/23+", },   // DEC r/m8
           0xFF: { 00 => inc[width=word, flags, modrm16] / "3/23+",     // INC r/m16
-                  08 => dec[width=word, flags, modrm16] / "3/23+", },  // DEC r/m16
+                  08 => dec[width=word, flags, modrm16] / "3/23+",  // DEC r/m16
+                  30 => push_modrm[unsafe_bus_maybe, regval=ss, reg=sp, modrm16] / "15/24+" },
         },
         modrm: {
           0x00 => seg=ds, displace0=(b+si) / M 7,

src/emu/operands.rs

@@ -1,4 +1,5 @@
 use std::cell::Cell;
+use std::convert::From;
 
 use emu::num_traits as nt;
 
@@ -148,3 +149,20 @@ impl RValue<u8> for RegLo<'_> {
     read_lo(self.reg)
   }
 }
+
+pub enum DynLValue<'a> {
+  Reg(Reg<'a>),
+  FarPtr(FarPtr<'a>),
+}
+
+impl<'a> From<Reg<'a>> for DynLValue<'a> {
+  fn from(reg: Reg<'a>) -> DynLValue<'a> {
+    DynLValue::Reg(reg)
+  }
+}
+
+impl<'a> From<FarPtr<'a>> for DynLValue<'a> {
+  fn from(farptr: FarPtr<'a>) -> DynLValue<'a> {
+    DynLValue::FarPtr(farptr)
+  }
+}

src/emu/operations.rs

@@ -1,9 +1,10 @@
+use std::convert::Into;
 use std::fmt::Debug;
 use std::marker::PhantomData;
 
 use emu::dos;
 use emu::i8088::{Flags, RepPrefix, i8088};
-use emu::operands::{Address, FarPtr, LValue, Operand, Reg, RValue};
+use emu::operands::{Address, DynLValue, FarPtr, LValue, Operand, Reg, RValue};
 use emu::pc::Bus;
 
 macro_rules! string_op {
@@ -320,9 +321,57 @@ pub fn nop() {}
 pub fn pop(bus: &mut Bus, ss: u16, mut sp: Reg, mut dst: Reg) {
   let ptr = FarPtr { bus: bus, segment: ss, offset: sp.read() };
   dst.write(ptr.read());
+  // XXX: Not checking for stack faults or anything
   sp.write(sp.read() + 2);
 }
 
+// Ugly hack for the ModR/M case. The ModR/M byte may select for a
+// memory operand, in which case we are supplied with a FarPtr for our
+// operand which contains a mutable pointer to the Bus. Unfortunately,
+// we also want a mutable Bus pointer for accessing the stack. In the
+// normal pop() function, this is passed in as an argument. Doing this
+// here though would mean two mutable references and an angry borrow
+// checker. We could just NOT pass in a Bus for stack access and
+// instead just use the one from the memory operand, but alas the
+// ModR/M byte can also select for a register operand and those do
+// not carry a Bus pointer so we'd be left without one in that case.
+// This is all the more annoying because there isn't a great reason
+// to do this since all the registers that ModR/M can select for have
+// their own dedicated single-byte opcodes for push/pop.
+//
+// The "solution" employed here takes a Bus pointer Option as an
+// argument which is filled whenever the supplied operand is a
+// register, but None whenever the operand is a FarPtr. We then do a
+// manual dynamic dispatch on the operand type. In the Register case,
+// we just delegate to normal pop(). In the memory case, we use the
+// operand's Bus pointer to access the stack.
+//
+// There is unsafety getting to this point. See the comments on the
+// @unsafe_bus_maybe argument decoder. I'm not sure that this is
+// sound. If things start crashing, removal of the modrm variants is a
+// resonable option as they do not appear to be used in any code I
+// currently care about.
+//
+// If it turns out that a "real" solution is needed, this can be
+// rewritten safely. We could remove the reference to the Bus in
+// FarPtr, and just *always* pass in a bus to all operations
+// everywhere that might require it. This would also mean that
+// [LR]Value read & write would also need to take a bus argument. That
+// seems like a lot of ugly for these two ops that I don't even care
+// about, so instead of spreading the ugly out evenly and doing it
+// "right", I'm concentrating it here.
+pub fn pop_modrm<'a>(bus: Option<&mut Bus>, ss: u16, mut sp: Reg, dst: impl Into<DynLValue<'a>>) {
+  match dst.into() {
+    DynLValue::Reg(reg) => pop(bus.unwrap(), ss, sp, reg),
+    DynLValue::FarPtr(mut farptr) => {
+      let val = <FarPtr as RValue<u16>>::read(&FarPtr { bus: farptr.bus, segment: ss, offset: sp.read() });
+      farptr.write(val);
+      // XXX: Not checking for stack faults or anything
+      sp.write(sp.read() + 2);
+    }
+  }
+}
+
 pub fn push(bus: &mut Bus, ss: u16, mut sp: Reg, val: u16) {
   // XXX: Not checking for stack faults or anything
   sp.write(sp.read() - 2);
@@ -330,6 +379,17 @@ pub fn push(bus: &mut Bus, ss: u16, mut sp: Reg, val: u16) {
   ptr.write(val);
 }
 
+// Ugly hack for the ModR/M case. See the comments on pop_modrm.
+pub fn push_modrm<'a>(bus: Option<&mut Bus>, ss: u16, sp: Reg, src: impl Into<DynLValue<'a>>) {
+  match src.into() {
+    DynLValue::Reg(reg) => push(bus.unwrap(), ss, sp, reg.read()),
+    DynLValue::FarPtr(farptr) => {
+      let val: u16 = farptr.read();
+      push(farptr.bus, ss, sp, val);
+    }
+  }
+}
+
 pub fn scasb(flags: &mut Flags,
              bus: &mut Bus,
              rep: RepPrefix,