emu: remove unsafe from {push,pop}_modrm operations

src/emu/i8088.rs

@@ -85,6 +85,28 @@ macro_rules! step {
     step!(@code form=$form, $done, $cpu, $bus, $prefix, $modrm, $name, $cycles, $rest)
   };
 
+  // Adds tokens to the last argument. Used by @convert to append ".into()" to the last arg
+  (@append-to-last ($($done:tt)*)
+                   ( $(form=$form:tt,)? ($last:tt),
+                     $cpu:expr, $bus:expr, $prefix:tt, $modrm:tt, $name:ident, $cycles:literal,
+                     $rest:tt ) { $($conv:tt)+ } ) => {
+    step!(@code $(form=$form,)? ($($done)* ($last$($conv)+)),
+                $cpu, $bus, $prefix, $modrm, $name, $cycles,
+                $rest )
+  };
+
+  // Recursive case for @append-to-last, walks through already
+  // processed args to find the last one for appending
+  (@append-to-last ($($done:tt)*)
+                   ( $(form=$form:tt,)? ($next:tt $($restdone:tt)+),
+                     $cpu:expr, $bus:expr, $prefix:tt, $modrm:tt, $name:ident, $cycles:literal,
+                     $rest:tt ) { $($conv:tt)+ } ) => {
+    step!(@append-to-last ($($done)* $next)
+                          ( $(form=$form,)? ($($restdone)+),
+                     $cpu, $bus, $prefix, $modrm, $name, $cycles,
+                     $rest ) { $($conv)+ } )
+  };
+
   // === OP DIRECTIVES ===
 
   (@addr $cookie:tt, $cpu:expr, $bus:expr,
@@ -98,49 +120,15 @@ 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
-          }
-    )
-  };
-
   (@const=$val:literal $cookie:tt, $cpu:expr, $bus:expr, $prefix:tt, $modrm:tt) => {
     step!(@arg $cookie, $val)
   };
 
+  // Adds ".into()" onto the last argument
+  (@convert $cookie:tt, $cpu:expr, $bus:expr, $prefix:tt, $modrm:tt) => {
+    step!(@append-to-last () $cookie { .into() } )
+  };
+
   (@cpu $cookie:tt, $cpu:expr, $bus:expr, $prefix:tt, $modrm:tt) => {
     step!(@arg $cookie, $cpu)
   };
@@ -498,7 +486,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
+          0x8F: { 0x00 => pop_modrm[regval=ss, reg=sp, modrm16, convert, bus] / "12/25+" },  // POP r/m16
           0x90 => nop[] / 3,
           0xA0 => mov[reglo=a, addr] / 14,
           0xA1 => mov[reg=a, addr] / 14,
@@ -554,7 +542,7 @@ impl i8088 {
                   0x08 => dec[form=unary_byte, flags, modrm8] / "3/23+", },   // DEC r/m8
           0xFF: { 0x00 => inc[form=unary_word, flags, modrm16] / "3/23+",     // INC r/m16
                   0x08 => dec[form=unary_word, flags, modrm16] / "3/23+",  // DEC r/m16
-                  0x30 => push_modrm[unsafe_bus_maybe, regval=ss, reg=sp, modrm16] / "15/24+" },
+                  0x30 => push_modrm[regval=ss, reg=sp, modrm16, convert, bus] / "15/24+" },
         },
         modrm: {
           0x00 => seg=ds, displace0=(b+si) / M 7,

src/emu/operands.rs

@@ -185,7 +185,7 @@ impl RValue<u8> for RegLo<'_> {
 
 pub enum DynLValue<'a> {
   Reg(Reg<'a>),
-  FarPtr(FarPtr<'a>),
+  FarPtr { segment: u16, offset: u16 },
 }
 
 impl<'a> From<Reg<'a>> for DynLValue<'a> {
@@ -194,8 +194,8 @@ impl<'a> From<Reg<'a>> for DynLValue<'a> {
   }
 }
 
-impl<'a> From<FarPtr<'a>> for DynLValue<'a> {
+impl<'a> From<FarPtr<'_>> for DynLValue<'a> {
-  fn from(farptr: FarPtr<'a>) -> DynLValue<'a> {
+  fn from(farptr: FarPtr<'_>) -> DynLValue<'a> {
-    DynLValue::FarPtr(farptr)
+    DynLValue::FarPtr { segment: farptr.segment, offset: farptr.offset }
   }
 }

src/emu/operations.rs

@@ -1,4 +1,3 @@
-use std::convert::Into;
 use std::fmt::Debug;
 
 use emu::num_traits::AsPrimitive;
@@ -280,42 +279,30 @@ pub fn pop(bus: &mut Bus, ss: u16, mut sp: Reg, mut dst: Reg) {
 // 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
+// when ModR/M selects for a memory location though would mean two
-// checker. We could just NOT pass in a Bus for stack access and
+// mutable references to the Bus and an angry borrow checker.
-// 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
+// We solve this with a wrapper type for dynamic dispatch. If ModR/M
-// argument which is filled whenever the supplied operand is a
+// selects for a register (which is kind of silly since single-byte
-// register, but None whenever the operand is a FarPtr. We then do a
+// opcodes exist for these cases), then we just defer to normal
-// manual dynamic dispatch on the operand type. In the Register case,
+// pop(). If a memory location is selected for however, we don't wrap
-// we just delegate to normal pop(). In the memory case, we use the
+// a full FarPtr but a degraded form that doesn't contain a Bus
-// operand's Bus pointer to access the stack.
+// pointer (and thus isn't a valid [LR]Value). We can promote it back
+// to a full FarPtr using the provided Bus.
 //
-// There is unsafety getting to this point. See the comments on the
+// The Bus argument is also in an unusual position so that it is not
-// @unsafe_bus_maybe argument decoder. I'm not sure that this is
+// being held during the construction of the memory argument which
-// sound. If things start crashing, removal of the modrm variants is a
+// temporarily requires the Bus before downgrading to its dynamic
-// resonable option as they do not appear to be used in any code I
+// form. (This limitation is not essential, but it allows for reuse of
-// currently care about.
+// standard [LR]Value ModR/M code)
-//
+pub fn pop_modrm<'a>(ss: u16, mut sp: Reg, dst: DynLValue<'a>, bus: &mut Bus) {
-// If it turns out that a "real" solution is needed, this can be
+  match dst {
-// rewritten safely. We could remove the reference to the Bus in
+    DynLValue::Reg(reg) => pop(bus, ss, sp, reg),
-// FarPtr, and just *always* pass in a bus to all operations
+    DynLValue::FarPtr { segment: dst_segment, offset: dst_offset } => {
-// everywhere that might require it. This would also mean that
+      let val = <FarPtr as RValue<u16>>::read(&FarPtr { bus: bus, segment: ss, offset: sp.read() });
-// [LR]Value read & write would also need to take a bus argument. That
+      <FarPtr as LValue<u16>>::write(
-// seems like a lot of ugly for these two ops that I don't even care
+        &mut FarPtr { bus: bus, segment: dst_segment, offset: dst_offset },
-// about, so instead of spreading the ugly out evenly and doing it
+        val);
-// "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);
     }
@@ -330,12 +317,12 @@ pub fn push(bus: &mut Bus, ss: u16, mut sp: Reg, val: u16) {
 }
 
 // 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>>) {
+pub fn push_modrm<'a>(ss: u16, sp: Reg, src: DynLValue<'a>, bus: &mut Bus) {
-  match src.into() {
+  match src {
-    DynLValue::Reg(reg) => push(bus.unwrap(), ss, sp, reg.read()),
+    DynLValue::Reg(reg) => push(bus, ss, sp, reg.read()),
-    DynLValue::FarPtr(farptr) => {
+    DynLValue::FarPtr { segment, offset } => {
-      let val: u16 = farptr.read();
+      let val = <FarPtr as RValue<u16>>::read(&FarPtr { bus: bus, segment: segment, offset: offset });
-      push(farptr.bus, ss, sp, val);
+      push(bus, ss, sp, val);
     }
   }
 }