From 40fed58ecf5ad40ed092c9242ae263c4d2785ff9 Mon Sep 17 00:00:00 2001 From: Jared Burce Date: Mon, 29 Mar 2021 00:20:42 -0700 Subject: [PATCH] emu: remove unsafe from {push,pop}_modrm operations --- src/emu/i8088.rs | 70 ++++++++++++++++++------------------------- src/emu/operands.rs | 8 ++--- src/emu/operations.rs | 69 +++++++++++++++++------------------------- 3 files changed, 61 insertions(+), 86 deletions(-) diff --git a/src/emu/i8088.rs b/src/emu/i8088.rs index c30e5a5..fc7a072 100644 --- a/src/emu/i8088.rs +++ b/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, diff --git a/src/emu/operands.rs b/src/emu/operands.rs index e793e02..62be6f7 100644 --- a/src/emu/operands.rs +++ b/src/emu/operands.rs @@ -185,7 +185,7 @@ impl RValue for RegLo<'_> { pub enum DynLValue<'a> { Reg(Reg<'a>), - FarPtr(FarPtr<'a>), + FarPtr { segment: u16, offset: u16 }, } impl<'a> From> for DynLValue<'a> { @@ -194,8 +194,8 @@ impl<'a> From> for DynLValue<'a> { } } -impl<'a> From> for DynLValue<'a> { - fn from(farptr: FarPtr<'a>) -> DynLValue<'a> { - DynLValue::FarPtr(farptr) +impl<'a> From> for DynLValue<'a> { + fn from(farptr: FarPtr<'_>) -> DynLValue<'a> { + DynLValue::FarPtr { segment: farptr.segment, offset: farptr.offset } } } diff --git a/src/emu/operations.rs b/src/emu/operations.rs index 6495738..b6498c8 100644 --- a/src/emu/operations.rs +++ b/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 -// 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. +// when ModR/M selects for a memory location though would mean two +// mutable references to the Bus and an angry borrow checker. // -// 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. +// We solve this with a wrapper type for dynamic dispatch. If ModR/M +// selects for a register (which is kind of silly since single-byte +// opcodes exist for these cases), then we just defer to normal +// pop(). If a memory location is selected for however, we don't wrap +// a full FarPtr but a degraded form that doesn't contain a Bus +// 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 -// @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>) { - match dst.into() { - DynLValue::Reg(reg) => pop(bus.unwrap(), ss, sp, reg), - DynLValue::FarPtr(mut farptr) => { - let val = >::read(&FarPtr { bus: farptr.bus, segment: ss, offset: sp.read() }); - farptr.write(val); +// The Bus argument is also in an unusual position so that it is not +// being held during the construction of the memory argument which +// temporarily requires the Bus before downgrading to its dynamic +// form. (This limitation is not essential, but it allows for reuse of +// standard [LR]Value ModR/M code) +pub fn pop_modrm<'a>(ss: u16, mut sp: Reg, dst: DynLValue<'a>, bus: &mut Bus) { + match dst { + DynLValue::Reg(reg) => pop(bus, ss, sp, reg), + DynLValue::FarPtr { segment: dst_segment, offset: dst_offset } => { + let val = >::read(&FarPtr { bus: bus, segment: ss, offset: sp.read() }); + >::write( + &mut FarPtr { bus: bus, segment: dst_segment, offset: dst_offset }, + 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>) { - 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 push_modrm<'a>(ss: u16, sp: Reg, src: DynLValue<'a>, bus: &mut Bus) { + match src { + DynLValue::Reg(reg) => push(bus, ss, sp, reg.read()), + DynLValue::FarPtr { segment, offset } => { + let val = >::read(&FarPtr { bus: bus, segment: segment, offset: offset }); + push(bus, ss, sp, val); } } }