Refactor ALU code into its own module, and fix things.

* Fix overflow flag being ignored
* Fix barrel shifter misimplementations
* Rustfmt fixes


Former-commit-id: a14d50372f7850a04b204accf4e5d8b924e4b48e

src/arm7tdmi/alu.rs

@@ -0,0 +1,262 @@
+use bit::BitIndex;
+use num::FromPrimitive;
+
+use super::{Core, CpuError, CpuResult, REG_PC};
+
+#[derive(Debug, Primitive)]
+pub enum AluOpCode {
+    AND = 0b0000,
+    EOR = 0b0001,
+    SUB = 0b0010,
+    RSB = 0b0011,
+    ADD = 0b0100,
+    ADC = 0b0101,
+    SBC = 0b0110,
+    RSC = 0b0111,
+    TST = 0b1000,
+    TEQ = 0b1001,
+    CMP = 0b1010,
+    CMN = 0b1011,
+    ORR = 0b1100,
+    MOV = 0b1101,
+    BIC = 0b1110,
+    MVN = 0b1111,
+}
+
+impl AluOpCode {
+    pub fn is_setting_flags(&self) -> bool {
+        use AluOpCode::*;
+        match self {
+            TST | TEQ | CMP | CMN => true,
+            _ => false,
+        }
+    }
+
+    pub fn is_logical(&self) -> bool {
+        use AluOpCode::*;
+        match self {
+            MOV | MVN | ORR | EOR | AND | BIC | TST | TEQ => true,
+            _ => false,
+        }
+    }
+    pub fn is_arithmetic(&self) -> bool {
+        use AluOpCode::*;
+        match self {
+            ADD | ADC | SUB | SBC | RSB | RSC | CMP | CMN => true,
+            _ => false,
+        }
+    }
+}
+
+#[derive(Debug, PartialEq, Primitive)]
+pub enum BarrelShiftOpCode {
+    LSL = 0,
+    LSR = 1,
+    ASR = 2,
+    ROR = 3,
+}
+
+#[derive(Debug, PartialEq)]
+pub enum ShiftedRegister {
+    ByAmount(u32, BarrelShiftOpCode),
+    ByRegister(usize, BarrelShiftOpCode),
+}
+
+impl From<u32> for ShiftedRegister {
+    fn from(v: u32) -> ShiftedRegister {
+        let typ = BarrelShiftOpCode::from_u8(v.bit_range(5..7) as u8).unwrap();
+        if v.bit(4) {
+            let rs = v.bit_range(8..12) as usize;
+            ShiftedRegister::ByRegister(rs, typ)
+        } else {
+            let amount = v.bit_range(7..12) as u32;
+            ShiftedRegister::ByAmount(amount, typ)
+        }
+    }
+}
+
+#[derive(Debug, PartialEq)]
+pub enum BarrelShifterValue {
+    ImmediateValue(i32),
+    RotatedImmediate(u32, u32),
+    ShiftedRegister {
+        reg: usize,
+        shift: ShiftedRegister,
+        added: Option<bool>,
+    },
+}
+
+impl BarrelShifterValue {
+    /// Decode operand2 as an immediate value
+    pub fn decode_rotated_immediate(&self) -> Option<i32> {
+        if let BarrelShifterValue::RotatedImmediate(immediate, rotate) = self {
+            return Some(immediate.rotate_right(*rotate) as i32);
+        }
+        None
+    }
+}
+
+impl Core {
+    /// Performs a generic barrel shifter operation
+    fn barrel_shift(&mut self, val: i32, amount: u32, shift: BarrelShiftOpCode) -> i32 {
+        //
+        // From GBATEK:
+        // Zero Shift Amount (Shift Register by Immediate, with Immediate=0)
+        //  LSL#0: No shift performed, ie. directly Op2=Rm, the C flag is NOT affected.
+        //  LSR#0: Interpreted as LSR#32, ie. Op2 becomes zero, C becomes Bit 31 of Rm.
+        //  ASR#0: Interpreted as ASR#32, ie. Op2 and C are filled by Bit 31 of Rm.
+        //  ROR#0: Interpreted as RRX#1 (RCR), like ROR#1, but Op2 Bit 31 set to old C.
+        //
+        // From ARM7TDMI Datasheet:
+        // 1 LSL by 32 has result zero, carry out equal to bit 0 of Rm.
+        // 2 LSL by more than 32 has result zero, carry out zero.
+        // 3 LSR by 32 has result zero, carry out equal to bit 31 of Rm.
+        // 4 LSR by more than 32 has result zero, carry out zero.
+        // 5 ASR by 32 or more has result filled with and carry out equal to bit 31 of Rm.
+        // 6 ROR by 32 has result equal to Rm, carry out equal to bit 31 of Rm.
+        // 7 ROR by n where n is greater than 32 will give the same result and carry out
+        //   as ROR by n-32; therefore repeatedly subtract 32 from n until the amount is
+        //   in the range 1 to 32 and see above.
+        //
+        match shift {
+            BarrelShiftOpCode::LSL => match amount {
+                0 => val,
+                x if x < 32 => {
+                    self.cpsr.set_C(val.wrapping_shr(32 - x) & 1 == 1);
+                    val << x
+                }
+                32 => {
+                    self.cpsr.set_C(val & 1 == 1);
+                    0
+                }
+                _ => {
+                    self.cpsr.set_C(false);
+                    0
+                }
+            },
+            BarrelShiftOpCode::LSR => match amount {
+                x if x > 0 && x < 32 => {
+                    self.cpsr.set_C(val >> (amount - 1) & 1 == 1);
+                    ((val as u32) >> amount) as i32
+                }
+                0 | 32 => {
+                    self.cpsr.set_C((val as u32).bit(31));
+                    0
+                }
+                _ => {
+                    self.cpsr.set_C(false);
+                    0
+                }
+            },
+            BarrelShiftOpCode::ASR => {
+                if 0 < amount && amount < 32 {
+                    self.cpsr.set_C(val.wrapping_shr(amount - 1) & 1 == 1);
+                    val.wrapping_shr(amount)
+                } else {
+                    let bit31 = (val as u32).bit(31);
+                    self.cpsr.set_C(bit31);
+                    if bit31 {
+                        -1
+                    } else {
+                        0
+                    }
+                }
+            }
+            BarrelShiftOpCode::ROR => {
+                match amount {
+                    0 =>
+                    /* RRX */
+                    {
+                        let val = val as u32;
+                        let old_c = self.cpsr.C() as u32;
+                        (val.rotate_right(1) | (old_c << 31)) as i32
+                    }
+                    32 => {
+                        self.cpsr.set_C((val as u32).bit(31));
+                        val
+                    }
+                    _ => {
+                        let result = val.rotate_right(amount);
+                        self.cpsr.set_C((result >> 1) & 1 == 1);
+                        result
+                    }
+                }
+            }
+        }
+    }
+
+    pub fn register_shift(&mut self, reg: usize, shift: ShiftedRegister) -> CpuResult<i32> {
+        let val = self.get_reg(reg) as i32;
+        match shift {
+            ShiftedRegister::ByAmount(amount, shift) => Ok(self.barrel_shift(val, amount, shift)),
+            ShiftedRegister::ByRegister(reg, shift) => {
+                if reg != REG_PC {
+                    Ok(self.barrel_shift(val, self.get_reg(reg) & 0xff, shift))
+                } else {
+                    Err(CpuError::IllegalInstruction)
+                }
+            }
+        }
+    }
+
+    fn alu_sub_flags(a: i32, b: i32, carry: &mut bool, overflow: &mut bool) -> i32 {
+        let res = a.wrapping_sub(b);
+        *carry = res > a;
+        let (_, would_overflow) = a.overflowing_sub(b);
+        *overflow = would_overflow;
+        res
+    }
+
+    fn alu_add_flags(a: i32, b: i32, carry: &mut bool, overflow: &mut bool) -> i32 {
+        let res = a.wrapping_add(b);
+        *carry = res < a;
+        let (_, would_overflow) = a.overflowing_add(b);
+        *overflow = would_overflow;
+        res
+    }
+
+    #[allow(non_snake_case)]
+    pub fn alu(
+        &mut self,
+        opcode: AluOpCode,
+        op1: i32,
+        op2: i32,
+        set_cond_flags: bool,
+    ) -> Option<i32> {
+        use AluOpCode::*;
+
+        let C = self.cpsr.C() as i32;
+
+        let mut carry = self.cpsr.C();
+        let mut overflow = self.cpsr.V();
+
+        let result = match opcode {
+            AND | TST => op1 & op2,
+            EOR | TEQ => op1 ^ op2,
+            SUB | CMP => Self::alu_sub_flags(op1, op2, &mut carry, &mut overflow),
+            RSB => Self::alu_sub_flags(op2, op1, &mut carry, &mut overflow),
+            ADD | CMN => Self::alu_add_flags(op1, op2, &mut carry, &mut overflow),
+            ADC => Self::alu_add_flags(op1, op2.wrapping_add(C), &mut carry, &mut overflow),
+            SBC => Self::alu_sub_flags(op1, op2.wrapping_sub(1 - C), &mut carry, &mut overflow),
+            RSC => Self::alu_sub_flags(op2, op1.wrapping_sub(1 - C), &mut carry, &mut overflow),
+            ORR => op1 | op2,
+            MOV => op2,
+            BIC => op1 & (!op2),
+            MVN => !op2,
+        };
+
+        if set_cond_flags {
+            self.cpsr.set_N(result < 0);
+            self.cpsr.set_Z(result == 0);
+            self.cpsr.set_C(carry);
+            if opcode.is_arithmetic() {
+                self.cpsr.set_V(overflow);
+            }
+        }
+
+        match opcode {
+            TST | TEQ | CMP | CMN => None,
+            _ => Some(result),
+        }
+    }
+}

src/arm7tdmi/arm/display.rs

@@ -1,10 +1,9 @@
 use std::fmt;
 
-use super::{
+use super::{AluOpCode, ArmCond, ArmFormat, ArmHalfwordTransferType, ArmInstruction};
-    ArmCond, ArmFormat, ArmHalfwordTransferType, ArmInstruction, ArmOpCode, ArmRegisterShift,
+use crate::arm7tdmi::{
-    ArmShiftType, ArmShiftedValue,
+    reg_string, Addr, BarrelShiftOpCode, BarrelShifterValue, ShiftedRegister, REG_PC,
 };
-use crate::arm7tdmi::{reg_string, Addr, REG_PC};
 
 impl fmt::Display for ArmCond {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -29,9 +28,9 @@ impl fmt::Display for ArmCond {
     }
 }
 
-impl fmt::Display for ArmOpCode {
+impl fmt::Display for AluOpCode {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        use ArmOpCode::*;
+        use AluOpCode::*;
         match self {
             AND => write!(f, "and"),
             EOR => write!(f, "eor"),
@@ -53,9 +52,9 @@ impl fmt::Display for ArmOpCode {
     }
 }
 
-impl fmt::Display for ArmShiftType {
+impl fmt::Display for BarrelShiftOpCode {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        use ArmShiftType::*;
+        use BarrelShiftOpCode::*;
         match self {
             LSL => write!(f, "lsl"),
             LSR => write!(f, "lsr"),
@@ -76,25 +75,23 @@ impl fmt::Display for ArmHalfwordTransferType {
     }
 }
 
-fn is_shift(shift: &ArmRegisterShift) -> bool {
+fn is_shift(shift: &ShiftedRegister) -> bool {
-    if let ArmRegisterShift::ShiftAmount(val, typ) = shift {
+    if let ShiftedRegister::ByAmount(val, typ) = shift {
-        return !(*val == 0 && *typ == ArmShiftType::LSL);
+        return !(*val == 0 && *typ == BarrelShiftOpCode::LSL);
     }
     true
 }
 
 impl ArmInstruction {
-    fn make_shifted_reg_string(&self, reg: usize, shift: ArmRegisterShift) -> String {
+    fn make_shifted_reg_string(&self, reg: usize, shift: ShiftedRegister) -> String {
         let reg = reg_string(reg).to_string();
         if !is_shift(&shift) {
             return reg;
         }
 
         match shift {
-            ArmRegisterShift::ShiftAmount(imm, typ) => format!("{}, {} #{}", reg, typ, imm),
+            ShiftedRegister::ByAmount(imm, typ) => format!("{}, {} #{}", reg, typ, imm),
-            ArmRegisterShift::ShiftRegister(rs, typ) => {
+            ShiftedRegister::ByRegister(rs, typ) => format!("{}, {} {}", reg, typ, reg_string(rs)),
-                format!("{}, {} {}", reg, typ, reg_string(rs))
-            }
         }
     }
 
@@ -121,7 +118,7 @@ impl ArmInstruction {
     }
 
     fn fmt_data_processing(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        use ArmOpCode::*;
+        use AluOpCode::*;
 
         let opcode = self.opcode().unwrap();
 
@@ -154,11 +151,11 @@ impl ArmInstruction {
 
         let operand2 = self.operand2().unwrap();
         match operand2 {
-            ArmShiftedValue::RotatedImmediate(_, _) => {
+            BarrelShifterValue::RotatedImmediate(_, _) => {
                 let value = operand2.decode_rotated_immediate().unwrap();
                 write!(f, "#{}\t; {:#x}", value, value)
             }
-            ArmShiftedValue::ShiftedRegister {
+            BarrelShifterValue::ShiftedRegister {
                 reg,
                 shift,
                 added: _,
@@ -175,10 +172,10 @@ impl ArmInstruction {
         }
     }
 
-    fn fmt_rn_offset(&self, f: &mut fmt::Formatter, offset: ArmShiftedValue) -> fmt::Result {
+    fn fmt_rn_offset(&self, f: &mut fmt::Formatter, offset: BarrelShifterValue) -> fmt::Result {
         write!(f, "[{Rn}", Rn = reg_string(self.rn()))?;
         let (ofs_string, comment) = match offset {
-            ArmShiftedValue::ImmediateValue(value) => {
+            BarrelShifterValue::ImmediateValue(value) => {
                 let value_for_commnet = if self.rn() == REG_PC {
                     value + (self.pc as i32) + 8 // account for pipelining
                 } else {
@@ -189,7 +186,7 @@ impl ArmInstruction {
                     Some(format!("\t; {:#x}", value_for_commnet)),
                 )
             }
-            ArmShiftedValue::ShiftedRegister {
+            BarrelShifterValue::ShiftedRegister {
                 reg,
                 shift,
                 added: Some(added),
@@ -232,7 +229,7 @@ impl ArmInstruction {
             Rd = reg_string(self.rd()),
         )?;
 
-        self.fmt_rn_offset(f, self.ldr_str_offset().unwrap())
+        self.fmt_rn_offset(f, self.ldr_str_offset())
     }
 
     fn fmt_ldm_stm(&self, f: &mut fmt::Formatter) -> fmt::Result {

src/arm7tdmi/arm/exec.rs

@@ -1,5 +1,6 @@
 use crate::bit::BitIndex;
 
+use crate::arm7tdmi::alu::*;
 use crate::arm7tdmi::bus::Bus;
 use crate::arm7tdmi::cpu::{Core, CpuExecResult, CpuPipelineAction};
 use crate::arm7tdmi::exception::Exception;
@@ -83,114 +84,6 @@ impl Core {
         Ok(CpuPipelineAction::IncPC)
     }
 
-    fn barrel_shift(&mut self, val: i32, amount: u32, shift: ArmShiftType) -> i32 {
-        match shift {
-            ArmShiftType::LSL => {
-                if amount < 32 {
-                    self.cpsr.set_C(val.wrapping_shr(32 - amount) & 1 == 1);
-                } else {
-                    if amount == 32 {
-                        self.cpsr.set_C(val & 1 == 1);
-                    } else {
-                        self.cpsr.set_C(false)
-                    }
-                }
-                val.wrapping_shl(amount)
-            }
-            ArmShiftType::LSR => {
-                if 0 < amount && amount < 32 {
-                    self.cpsr.set_C(val.wrapping_shr(amount - 1) & 1 == 1);
-                } else {
-                    self.cpsr.set_C(false);
-                }
-                (val as u32).wrapping_shr(amount) as i32
-            }
-            ArmShiftType::ASR => {
-                if 0 < amount && amount < 32 {
-                    self.cpsr.set_C(val.wrapping_shr(amount - 1) & 1 == 1);
-                } else {
-                    self.cpsr.set_C(val >> 31 == 1);
-                }
-                val.wrapping_shr(amount)
-            }
-            ArmShiftType::ROR => {
-                let amount = amount % 32;
-                let result = val.rotate_right(amount);
-                self.cpsr.set_C((result >> 1) & 1 == 1);
-                result
-            }
-        }
-    }
-
-    pub fn register_shift(&mut self, reg: usize, shift: ArmRegisterShift) -> CpuResult<i32> {
-        let val = self.get_reg(reg) as i32;
-        match shift {
-            ArmRegisterShift::ShiftAmount(amount, shift) => {
-                Ok(self.barrel_shift(val, amount, shift))
-            }
-            ArmRegisterShift::ShiftRegister(reg, shift) => {
-                if reg != REG_PC {
-                    Ok(self.barrel_shift(val, self.get_reg(reg) & 0xff, shift))
-                } else {
-                    Err(CpuError::IllegalInstruction)
-                }
-            }
-        }
-    }
-
-    fn alu_sub_update_carry(a: i32, b: i32, carry: &mut bool) -> i32 {
-        let res = a.wrapping_sub(b);
-        *carry = res > a;
-        res
-    }
-
-    fn alu_add_update_carry(a: i32, b: i32, carry: &mut bool) -> i32 {
-        let res = a.wrapping_add(b);
-        *carry = res < a;
-        res
-    }
-
-    #[allow(non_snake_case)]
-    pub fn alu(
-        &mut self,
-        opcode: ArmOpCode,
-        op1: i32,
-        op2: i32,
-        set_cond_flags: bool,
-    ) -> Option<i32> {
-        let C = self.cpsr.C() as i32;
-
-        let mut carry = self.cpsr.C();
-        let overflow = self.cpsr.V();
-
-        let result = match opcode {
-            ArmOpCode::AND | ArmOpCode::TST => op1 & op2,
-            ArmOpCode::EOR | ArmOpCode::TEQ => op1 ^ op2,
-            ArmOpCode::SUB | ArmOpCode::CMP => Self::alu_sub_update_carry(op1, op2, &mut carry),
-            ArmOpCode::RSB => Self::alu_sub_update_carry(op2, op1, &mut carry),
-            ArmOpCode::ADD | ArmOpCode::CMN => Self::alu_add_update_carry(op1, op2, &mut carry),
-            ArmOpCode::ADC => Self::alu_add_update_carry(op1, op2.wrapping_add(C), &mut carry),
-            ArmOpCode::SBC => Self::alu_sub_update_carry(op1, op2.wrapping_sub(1 - C), &mut carry),
-            ArmOpCode::RSC => Self::alu_sub_update_carry(op2, op1.wrapping_sub(1 - C), &mut carry),
-            ArmOpCode::ORR => op1 | op2,
-            ArmOpCode::MOV => op2,
-            ArmOpCode::BIC => op1 & (!op2),
-            ArmOpCode::MVN => !op2,
-        };
-
-        if set_cond_flags {
-            self.cpsr.set_N(result < 0);
-            self.cpsr.set_Z(result == 0);
-            self.cpsr.set_C(carry);
-            self.cpsr.set_V(overflow);
-        }
-
-        match opcode {
-            ArmOpCode::TST | ArmOpCode::TEQ | ArmOpCode::CMP | ArmOpCode::CMN => None,
-            _ => Some(result),
-        }
-    }
-
     /// Logical/Arithmetic ALU operations
     ///
     /// Cycles: 1S+x+y (from GBATEK)
@@ -214,20 +107,21 @@ impl Core {
         let rd = insn.rd();
 
         let op2: i32 = match op2 {
-            ArmShiftedValue::RotatedImmediate(immediate, rotate) => {
+            BarrelShifterValue::RotatedImmediate(immediate, rotate) => {
-                Ok(immediate.rotate_right(rotate) as i32)
+                immediate.rotate_right(rotate) as i32
             }
-            ArmShiftedValue::ShiftedRegister {
+            BarrelShifterValue::ShiftedRegister {
                 reg,
                 shift,
                 added: _,
             } => {
                 // +1I
                 self.add_cycle();
-                self.register_shift(reg, shift)
+                let result = self.register_shift(reg, shift)?;
+                result
             }
             _ => unreachable!(),
-        }?;
+        };
 
         let opcode = insn.opcode().unwrap();
         let set_flags = opcode.is_setting_flags() || insn.set_cond_flag();
@@ -241,11 +135,11 @@ impl Core {
         Ok(pipeline_action)
     }
 
-    fn get_rn_offset(&mut self, sval: ArmShiftedValue) -> i32 {
+    fn get_rn_offset(&mut self, sval: BarrelShifterValue) -> i32 {
         // TODO decide if error handling or panic here
         match sval {
-            ArmShiftedValue::ImmediateValue(offset) => offset,
+            BarrelShifterValue::ImmediateValue(offset) => offset,
-            ArmShiftedValue::ShiftedRegister {
+            BarrelShifterValue::ShiftedRegister {
                 reg,
                 shift,
                 added: Some(added),
@@ -284,7 +178,7 @@ impl Core {
             addr = insn.pc + 8; // prefetching
         }
 
-        let offset = self.get_rn_offset(insn.ldr_str_offset().unwrap());
+        let offset = self.get_rn_offset(insn.ldr_str_offset());
 
         let effective_addr = (addr as i32).wrapping_add(offset) as Addr;
         addr = if insn.pre_index_flag() {

src/arm7tdmi/arm/mod.rs

@@ -1,13 +1,13 @@
 pub mod display;
 pub mod exec;
 
+use super::alu::*;
 use crate::arm7tdmi::{Addr, InstructionDecoder, InstructionDecoderError};
 
 use crate::bit::BitIndex;
 use crate::byteorder::{LittleEndian, ReadBytesExt};
 use crate::num::FromPrimitive;
 
-use std::convert::TryFrom;
 use std::io;
 
 #[derive(Debug, PartialEq)]
@@ -90,35 +90,6 @@ pub enum ArmFormat {
     MSR_FLAGS,
 }
 
-#[derive(Debug, Primitive)]
-pub enum ArmOpCode {
-    AND = 0b0000,
-    EOR = 0b0001,
-    SUB = 0b0010,
-    RSB = 0b0011,
-    ADD = 0b0100,
-    ADC = 0b0101,
-    SBC = 0b0110,
-    RSC = 0b0111,
-    TST = 0b1000,
-    TEQ = 0b1001,
-    CMP = 0b1010,
-    CMN = 0b1011,
-    ORR = 0b1100,
-    MOV = 0b1101,
-    BIC = 0b1110,
-    MVN = 0b1111,
-}
-
-impl ArmOpCode {
-    pub fn is_setting_flags(&self) -> bool {
-        match self {
-            ArmOpCode::TST | ArmOpCode::TEQ | ArmOpCode::CMP | ArmOpCode::CMN => true,
-            _ => false,
-        }
-    }
-}
-
 #[derive(Debug, PartialEq, Primitive)]
 pub enum ArmHalfwordTransferType {
     UnsignedHalfwords = 0b01,
@@ -204,59 +175,6 @@ impl InstructionDecoder for ArmInstruction {
     }
 }
 
-#[derive(Debug, PartialEq, Primitive)]
-pub enum ArmShiftType {
-    LSL = 0,
-    LSR = 1,
-    ASR = 2,
-    ROR = 3,
-}
-
-#[derive(Debug, PartialEq)]
-pub enum ArmRegisterShift {
-    ShiftAmount(u32, ArmShiftType),
-    ShiftRegister(usize, ArmShiftType),
-}
-
-impl TryFrom<u32> for ArmRegisterShift {
-    type Error = ArmDecodeErrorKind;
-
-    fn try_from(v: u32) -> Result<Self, Self::Error> {
-        let typ = match ArmShiftType::from_u8(v.bit_range(5..7) as u8) {
-            Some(s) => Ok(s),
-            _ => Err(InvalidShiftType(v.bit_range(5..7))),
-        }?;
-        if v.bit(4) {
-            let rs = v.bit_range(8..12) as usize;
-            Ok(ArmRegisterShift::ShiftRegister(rs, typ))
-        } else {
-            let amount = v.bit_range(7..12) as u32;
-            Ok(ArmRegisterShift::ShiftAmount(amount, typ))
-        }
-    }
-}
-
-#[derive(Debug, PartialEq)]
-pub enum ArmShiftedValue {
-    ImmediateValue(i32),
-    RotatedImmediate(u32, u32),
-    ShiftedRegister {
-        reg: usize,
-        shift: ArmRegisterShift,
-        added: Option<bool>,
-    },
-}
-
-impl ArmShiftedValue {
-    /// Decode operand2 as an immediate value
-    pub fn decode_rotated_immediate(&self) -> Option<i32> {
-        if let ArmShiftedValue::RotatedImmediate(immediate, rotate) = self {
-            return Some(immediate.rotate_right(*rotate) as i32);
-        }
-        None
-    }
-}
-
 impl ArmInstruction {
     fn make_decode_error(&self, kind: ArmDecodeErrorKind) -> ArmDecodeError {
         ArmDecodeError {
@@ -298,8 +216,8 @@ impl ArmInstruction {
         self.raw.bit_range(16..20) as usize
     }
 
-    pub fn opcode(&self) -> Option<ArmOpCode> {
+    pub fn opcode(&self) -> Option<AluOpCode> {
-        ArmOpCode::from_u32(self.raw.bit_range(21..25))
+        AluOpCode::from_u32(self.raw.bit_range(21..25))
     }
 
     pub fn branch_offset(&self) -> i32 {
@@ -363,28 +281,27 @@ impl ArmInstruction {
     }
 
     /// gets offset used by ldr/str instructions
-    pub fn ldr_str_offset(&self) -> Result<ArmShiftedValue, ArmDecodeError> {
+    pub fn ldr_str_offset(&self) -> BarrelShifterValue {
         let ofs = self.raw.bit_range(0..12);
         if self.raw.bit(25) {
             let rm = ofs & 0xf;
-            let shift =
+            let shift = ShiftedRegister::from(ofs);
-                ArmRegisterShift::try_from(ofs).map_err(|kind| self.make_decode_error(kind))?;
+            BarrelShifterValue::ShiftedRegister {
-            Ok(ArmShiftedValue::ShiftedRegister {
                 reg: rm as usize,
                 shift: shift,
                 added: Some(self.add_offset_flag()),
-            })
+            }
         } else {
             let ofs = if self.add_offset_flag() {
                 ofs as i32
             } else {
                 -(ofs as i32)
             };
-            Ok(ArmShiftedValue::ImmediateValue(ofs))
+            BarrelShifterValue::ImmediateValue(ofs)
         }
     }
 
-    pub fn ldr_str_hs_offset(&self) -> Result<ArmShiftedValue, ArmDecodeError> {
+    pub fn ldr_str_hs_offset(&self) -> Result<BarrelShifterValue, ArmDecodeError> {
         match self.fmt {
             ArmFormat::LDR_STR_HS_IMM => {
                 let offset8 = (self.raw.bit_range(8..12) << 4) + self.raw.bit_range(0..4);
@@ -393,28 +310,27 @@ impl ArmInstruction {
                 } else {
                     -(offset8 as i32)
                 };
-                Ok(ArmShiftedValue::ImmediateValue(offset8))
+                Ok(BarrelShifterValue::ImmediateValue(offset8))
             }
-            ArmFormat::LDR_STR_HS_REG => Ok(ArmShiftedValue::ShiftedRegister {
+            ArmFormat::LDR_STR_HS_REG => Ok(BarrelShifterValue::ShiftedRegister {
                 reg: (self.raw & 0xf) as usize,
-                shift: ArmRegisterShift::ShiftAmount(0, ArmShiftType::LSL),
+                shift: ShiftedRegister::ByAmount(0, BarrelShiftOpCode::LSL),
                 added: Some(self.add_offset_flag()),
             }),
             _ => Err(self.make_decode_error(DecodedPartDoesNotBelongToInstruction)),
         }
     }
 
-    pub fn operand2(&self) -> Result<ArmShiftedValue, ArmDecodeError> {
+    pub fn operand2(&self) -> Result<BarrelShifterValue, ArmDecodeError> {
         let op2 = self.raw.bit_range(0..12);
         if self.raw.bit(25) {
             let immediate = op2 & 0xff;
             let rotate = 2 * op2.bit_range(8..12);
-            Ok(ArmShiftedValue::RotatedImmediate(immediate, rotate))
+            Ok(BarrelShifterValue::RotatedImmediate(immediate, rotate))
         } else {
             let reg = op2 & 0xf;
-            let shift =
+            let shift = ShiftedRegister::from(op2); // TODO error handling
-                ArmRegisterShift::try_from(op2).map_err(|kind| self.make_decode_error(kind))?; // TODO error handling
+            Ok(BarrelShifterValue::ShiftedRegister {
-            Ok(ArmShiftedValue::ShiftedRegister {
                 reg: reg as usize,
                 shift: shift,
                 added: None,
@@ -530,11 +446,11 @@ mod tests {
         assert_eq!(decoded.rn(), 5);
         assert_eq!(
             decoded.ldr_str_offset(),
-            Ok(ArmShiftedValue::ShiftedRegister {
+            BarrelShifterValue::ShiftedRegister {
                 reg: 6,
-                shift: ArmRegisterShift::ShiftAmount(5, ArmShiftType::LSL),
+                shift: ShiftedRegister::ByAmount(5, BarrelShiftOpCode::LSL),
                 added: Some(false)
-            })
+            }
         );
 
         assert_eq!(format!("{}", decoded), "ldreq\tr2, [r5, -r6, lsl #5]");
@@ -545,13 +461,12 @@ mod tests {
         core.gpr[6] = 1;
         core.gpr[2] = 0;
 
+        #[rustfmt::skip]
         let bytes = vec![
-            /* 00h: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+            /* 00h: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-            0x00, 0x00, 0x00, 0x00, /* 10h: */ 0x00, 0x00, 0x00, 0x00, 0x37, 0x13, 0x00, 0x00,
+            /* 10h: */ 0x00, 0x00, 0x00, 0x00, 0x37, 0x13, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-            0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 20h: */ 0x00, 0x00, 0x00, 0x00,
+            /* 20h: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-            0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+            /* 30h: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-            /* 30h: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-            0x00, 0x00, 0x00, 0x00,
         ];
         let mut mem = BoxedMemory::new(bytes.into_boxed_slice());
 
@@ -575,11 +490,11 @@ mod tests {
         assert_eq!(decoded.rn(), 4);
         assert_eq!(
             decoded.ldr_str_offset(),
-            Ok(ArmShiftedValue::ShiftedRegister {
+            BarrelShifterValue::ShiftedRegister {
                 reg: 7,
-                shift: ArmRegisterShift::ShiftAmount(8, ArmShiftType::ASR),
+                shift: ShiftedRegister::ByAmount(8, BarrelShiftOpCode::ASR),
                 added: Some(false)
-            })
+            }
         );
 
         assert_eq!(format!("{}", decoded), "strteq\tr2, [r4], -r7, asr #8");
@@ -590,13 +505,12 @@ mod tests {
         core.gpr[7] = 1;
         core.gpr[2] = 0xabababab;
 
+        #[rustfmt::skip]
         let bytes = vec![
-            /* 00h: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+            /* 00h: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-            0x00, 0x00, 0x00, 0x00, /* 10h: */ 0x00, 0x00, 0x00, 0x00, 0x37, 0x13, 0x00, 0x00,
+            /* 10h: */ 0x00, 0x00, 0x00, 0x00, 0x37, 0x13, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-            0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 20h: */ 0x00, 0x00, 0x00, 0x00,
+            /* 20h: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-            0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+            /* 30h: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-            /* 30h: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-            0x00, 0x00, 0x00, 0x00,
         ];
         let mut mem = BoxedMemory::new(bytes.into_boxed_slice());
 
@@ -618,9 +532,12 @@ mod tests {
         core.set_reg(4, 0x12345678);
         core.set_reg(REG_SP, 0);
 
+        #[rustfmt::skip]
         let bytes = vec![
-            /*  0: */ 0xaa, 0xbb, 0xcc, 0xdd, /*  4: */ 0xaa, 0xbb, 0xcc, 0xdd,
+            /*  0: */ 0xaa, 0xbb, 0xcc, 0xdd,
-            /*  8: */ 0xaa, 0xbb, 0xcc, 0xdd, /*  c: */ 0xaa, 0xbb, 0xcc, 0xdd,
+            /*  4: */ 0xaa, 0xbb, 0xcc, 0xdd,
+            /*  8: */ 0xaa, 0xbb, 0xcc, 0xdd,
+            /*  c: */ 0xaa, 0xbb, 0xcc, 0xdd,
             /* 10: */ 0xaa, 0xbb, 0xcc, 0xdd,
         ];
         let mut mem = BoxedMemory::new(bytes.into_boxed_slice());

src/arm7tdmi/mod.rs

@@ -10,6 +10,8 @@ use thumb::{ThumbDecodeError, ThumbInstruction};
 
 pub mod cpu;
 pub use cpu::*;
+pub mod alu;
+pub use alu::*;
 pub mod bus;
 pub use bus::*;
 pub mod exception;

src/arm7tdmi/thumb/display.rs

@@ -38,8 +38,8 @@ impl ThumbInstruction {
 
     fn fmt_thumb_alu_ops(&self, f: &mut fmt::Formatter) -> fmt::Result {
         let (op, shft) = self.alu_opcode();
-        if let Some(ArmRegisterShift::ShiftRegister(_, shftOp)) = shft {
+        if let Some(ShiftedRegister::ByRegister(_, op)) = shft {
-            write!(f, "{}", shftOp)?;
+            write!(f, "{}", op)?;
         } else {
             write!(f, "{}", op)?;
         }
@@ -115,7 +115,6 @@ impl ThumbInstruction {
                     (false, true) => "ldrh",
                     (true, false) => "ldsb",
                     (true, true) => "ldsh",
-                    _ => panic!("invalid flags"),
                 }
             },
             Rd = reg_string(self.rd()),

src/arm7tdmi/thumb/exec.rs

@@ -1,4 +1,3 @@
-use crate::arm7tdmi::arm::exec::*;
 use crate::arm7tdmi::bus::Bus;
 use crate::arm7tdmi::cpu::{Core, CpuExecResult, CpuPipelineAction};
 use crate::arm7tdmi::*;
@@ -25,7 +24,7 @@ impl Core {
         let result = self
             .register_shift(
                 insn.rs(),
-                ArmRegisterShift::ShiftAmount(insn.offset5() as u8 as u32, insn.format1_op()),
+                ShiftedRegister::ByAmount(insn.offset5() as u8 as u32, insn.format1_op()),
             )
             .unwrap();
         self.gpr[insn.rd()] = result as u32;
@@ -41,9 +40,9 @@ impl Core {
             self.get_reg(insn.rn()) as i32
         };
         let arm_alu_op = if insn.is_subtract() {
-            ArmOpCode::SUB
+            AluOpCode::SUB
         } else {
-            ArmOpCode::ADD
+            AluOpCode::ADD
         };
 
         let result = self.alu(arm_alu_op, op1, op2, true);
@@ -59,7 +58,7 @@ impl Core {
         _bus: &mut Bus,
         insn: ThumbInstruction,
     ) -> CpuExecResult {
-        let arm_alu_op: ArmOpCode = insn.format3_op().into();
+        let arm_alu_op: AluOpCode = insn.format3_op().into();
         let op1 = self.get_reg(insn.rd()) as i32;
         let op2 = insn.offset8() as u8 as i32;
         let result = self.alu(arm_alu_op, op1, op2, true);
@@ -139,10 +138,10 @@ impl Core {
             } else {
                 insn.rs()
             };
-            let arm_alu_op: ArmOpCode = insn.format5_op().into();
+            let arm_alu_op: AluOpCode = insn.format5_op().into();
             let op1 = self.get_reg(dst_reg) as i32;
             let op2 = self.get_reg(src_reg) as i32;
-            let result = self.alu(arm_alu_op, op1, op2, true);
+            let result = self.alu(arm_alu_op, op1, op2, false);
             if let Some(result) = result {
                 self.set_reg(dst_reg, result as u32);
                 if dst_reg == REG_PC {
@@ -235,7 +234,6 @@ impl Core {
                 let val = self.load_16(addr, bus) as i16 as i32 as u32;
                 self.gpr[rd] = val;
             }
-            _ => panic!("invalid flags"),
         }
 
         Ok(CpuPipelineAction::IncPC)
@@ -277,7 +275,7 @@ impl Core {
         self.do_exec_thumb_ldr_str_with_addr(bus, insn, addr)
     }
 
-    fn exec_thumb_load_address(&mut self, bus: &mut Bus, insn: ThumbInstruction) -> CpuExecResult {
+    fn exec_thumb_load_address(&mut self, _bus: &mut Bus, insn: ThumbInstruction) -> CpuExecResult {
         let result = if insn.flag(ThumbInstruction::FLAG_SP) {
             self.gpr[REG_SP] + (insn.word8() as Addr)
         } else {
@@ -307,7 +305,7 @@ impl Core {
     fn exec_thumb_add_sp(&mut self, _bus: &mut Bus, insn: ThumbInstruction) -> CpuExecResult {
         let op1 = self.gpr[REG_SP] as i32;
         let op2 = insn.sword7();
-        let arm_alu_op = ArmOpCode::ADD;
+        let arm_alu_op = AluOpCode::ADD;
 
         let result = self.alu(arm_alu_op, op1, op2, false);
         if let Some(result) = result {
@@ -352,7 +350,6 @@ impl Core {
 
         let is_load = insn.is_load();
         let rb = insn.rb();
-        let mut pipeline_action = CpuPipelineAction::IncPC;
 
         let mut addr = self.gpr[rb];
         let rlist = insn.register_list();
@@ -370,7 +367,7 @@ impl Core {
             }
         }
 
-        Ok(pipeline_action)
+        Ok(CpuPipelineAction::IncPC)
     }
 
     fn exec_thumb_branch_with_cond(

src/arm7tdmi/thumb/mod.rs

@@ -4,6 +4,7 @@ use crate::bit::BitIndex;
 use crate::byteorder::{LittleEndian, ReadBytesExt};
 use crate::num::FromPrimitive;
 
+use super::alu::*;
 use super::arm::*;
 use super::{Addr, InstructionDecoder, InstructionDecoderError};
 
@@ -42,7 +43,7 @@ pub enum ThumbFormat {
     AddSub,
     /// Format 3
     DataProcessImm,
-    /// Belongs to Format 4, but decoded seperatly because ArmOpcode doesn't have MUL
+    /// Belongs to Format 4, but decoded seperatly because AluOpCode doesn't have MUL
     Mul,
     /// Format 4
     AluOps,
@@ -163,13 +164,13 @@ pub enum OpFormat3 {
     SUB = 3,
 }
 
-impl From<OpFormat3> for ArmOpCode {
+impl From<OpFormat3> for AluOpCode {
-    fn from(op: OpFormat3) -> ArmOpCode {
+    fn from(op: OpFormat3) -> AluOpCode {
         match op {
-            OpFormat3::MOV => ArmOpCode::MOV,
+            OpFormat3::MOV => AluOpCode::MOV,
-            OpFormat3::CMP => ArmOpCode::CMP,
+            OpFormat3::CMP => AluOpCode::CMP,
-            OpFormat3::ADD => ArmOpCode::ADD,
+            OpFormat3::ADD => AluOpCode::ADD,
-            OpFormat3::SUB => ArmOpCode::SUB,
+            OpFormat3::SUB => AluOpCode::SUB,
         }
     }
 }
@@ -182,12 +183,12 @@ pub enum OpFormat5 {
     BX = 3,
 }
 
-impl From<OpFormat5> for ArmOpCode {
+impl From<OpFormat5> for AluOpCode {
-    fn from(op: OpFormat5) -> ArmOpCode {
+    fn from(op: OpFormat5) -> AluOpCode {
         match op {
-            OpFormat5::ADD => ArmOpCode::ADD,
+            OpFormat5::ADD => AluOpCode::ADD,
-            OpFormat5::CMP => ArmOpCode::CMP,
+            OpFormat5::CMP => AluOpCode::CMP,
-            OpFormat5::MOV => ArmOpCode::MOV,
+            OpFormat5::MOV => AluOpCode::MOV,
             OpFormat5::BX => panic!("this should not be called if op = BX"),
         }
     }
@@ -229,8 +230,8 @@ impl ThumbInstruction {
         self.raw.bit_range(6..9) as usize
     }
 
-    pub fn format1_op(&self) -> ArmShiftType {
+    pub fn format1_op(&self) -> BarrelShiftOpCode {
-        ArmShiftType::from_u8(self.raw.bit_range(11..13) as u8).unwrap()
+        BarrelShiftOpCode::from_u8(self.raw.bit_range(11..13) as u8).unwrap()
     }
 
     pub fn format3_op(&self) -> OpFormat3 {
@@ -241,38 +242,38 @@ impl ThumbInstruction {
         OpFormat5::from_u8(self.raw.bit_range(8..10) as u8).unwrap()
     }
 
-    pub fn alu_opcode(&self) -> (ArmOpCode, Option<ArmRegisterShift>) {
+    pub fn alu_opcode(&self) -> (AluOpCode, Option<ShiftedRegister>) {
         match self.raw.bit_range(6..10) {
             0b0010 => (
-                ArmOpCode::MOV,
+                AluOpCode::MOV,
-                Some(ArmRegisterShift::ShiftRegister(
+                Some(ShiftedRegister::ByRegister(
                     self.rs(),
-                    ArmShiftType::LSL,
+                    BarrelShiftOpCode::LSL,
                 )),
             ),
             0b0011 => (
-                ArmOpCode::MOV,
+                AluOpCode::MOV,
-                Some(ArmRegisterShift::ShiftRegister(
+                Some(ShiftedRegister::ByRegister(
                     self.rs(),
-                    ArmShiftType::LSR,
+                    BarrelShiftOpCode::LSR,
                 )),
             ),
             0b0100 => (
-                ArmOpCode::MOV,
+                AluOpCode::MOV,
-                Some(ArmRegisterShift::ShiftRegister(
+                Some(ShiftedRegister::ByRegister(
                     self.rs(),
-                    ArmShiftType::ASR,
+                    BarrelShiftOpCode::ASR,
                 )),
             ),
             0b0111 => (
-                ArmOpCode::MOV,
+                AluOpCode::MOV,
-                Some(ArmRegisterShift::ShiftRegister(
+                Some(ShiftedRegister::ByRegister(
                     self.rs(),
-                    ArmShiftType::ROR,
+                    BarrelShiftOpCode::ROR,
                 )),
             ),
             0b1101 => panic!("tried to decode MUL"),
-            op => (ArmOpCode::from_u16(op).unwrap(), None),
+            op => (AluOpCode::from_u16(op).unwrap(), None),
         }
     }
 
@@ -383,9 +384,13 @@ mod tests {
         // ldr r0, [pc, #4]
         let insn = ThumbInstruction::decode(0x4801, 0x6).unwrap();
 
+        #[rustfmt::skip]
         let bytes = vec![
-            /* 0: */ 0x00, 0x00, /* 2: */ 0x00, 0x00, /* 4: */ 0x00, 0x00,
+            /* 0: */ 0x00, 0x00, 
-            /* 6: <pc> */ 0x00, 0x00, /* 8: */ 0x00, 0x00, 0x00, 0x00,
+            /* 2: */ 0x00, 0x00,
+            /* 4: */ 0x00, 0x00,
+            /* 6: <pc> */ 0x00, 0x00,
+            /* 8: */ 0x00, 0x00, 0x00, 0x00,
             /* c: */ 0x78, 0x56, 0x34, 0x12,
         ];
         let mut mem = BoxedMemory::new(bytes.into_boxed_slice());
@@ -413,10 +418,13 @@ mod tests {
         core.set_reg(1, 0x4);
         core.set_reg(4, 0xc);
 
+        #[rustfmt::skip]
         let bytes = vec![
-            /*  0: */ 0xaa, 0xbb, 0xcc, 0xdd, /*  4: */ 0xaa, 0xbb, 0xcc, 0xdd,
+            /* 00h: */ 0xaa, 0xbb, 0xcc, 0xdd,
-            /*  8: */ 0xaa, 0xbb, 0xcc, 0xdd, /*  c: */ 0xaa, 0xbb, 0xcc, 0xdd,
+            /* 04h: */ 0xaa, 0xbb, 0xcc, 0xdd,
-            /* 10: */ 0xaa, 0xbb, 0xcc, 0xdd,
+            /* 08h: */ 0xaa, 0xbb, 0xcc, 0xdd,
+            /* 0ch: */ 0xaa, 0xbb, 0xcc, 0xdd,
+            /* 10h: */ 0xaa, 0xbb, 0xcc, 0xdd,
         ];
         let mut mem = BoxedMemory::new(bytes.into_boxed_slice());
 
@@ -438,14 +446,14 @@ mod tests {
     #[test]
     fn format8() {
         let mut core = Core::new();
+        #[rustfmt::skip]
         let bytes = vec![
-            /*  0: */ 0xaa, 0xbb, 0xcc, 0xdd, 0xaa, 0xbb, 0xcc, 0xdd,
+            /* 00h: */ 0xaa, 0xbb, 0xcc, 0xdd, 0xaa, 0xbb, 0xcc, 0xdd,
-            /*  8: */ 0xaa, 0xbb, 0xcc, 0xdd, 0xaa, 0xbb, 0xcc, 0xdd,
+            /* 08h: */ 0xaa, 0xbb, 0xcc, 0xdd, 0xaa, 0xbb, 0xcc, 0xdd,
-            /* 10: */ 0xaa, 0xbb, 0xcc, 0xdd, 0xaa, 0xbb, 0xcc, 0xdd,
+            /* 10h: */ 0xaa, 0xbb, 0xcc, 0xdd, 0xaa, 0xbb, 0xcc, 0xdd,
         ];
         let mut mem = BoxedMemory::new(bytes.into_boxed_slice());
 
-        
         core.gpr[4] = 0x12345678;
         core.gpr[3] = 0x2;
         core.gpr[0] = 0x4;