Decode the correct number of operands for CMP/CMN (immediate/register)

This also incidentally fixes an issue where MOV/MVN instructions with a
shifted operand could be decoded as if they had 3 operands.

3 files changed, 65 insertions, 22 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 8229e4d..1bb05d3 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -17,6 +17,8 @@ several fixes from @Grond66:
 * ARMv7: reject unconditional instructions with op1=1111xxxx. these were
   decoded as CDP, MCR, or MRC when the bit pattern is actually undefined.
 * ARMv7: fix inverted reserved bit test for some ldr*/str* (immediate) forms.
+* ARMv7: fix several issues with data processing instructions:
+  * CMP/CMN decoded "three operands" when they only have two.
 
 thank you for the patches!
 
diff --git a/src/armv7.rs b/src/armv7.rs
index 8b55e65..2403eaa 100644
--- a/src/armv7.rs
+++ b/src/armv7.rs
@@ -2558,42 +2558,52 @@ impl Decoder<ARMv7> for InstDecoder {
                                 (Rn, Rd, shift_spec, Rm)
                             };
 
-                            if shift_spec & 0xff0 == 0 {
-                                if (0b1101 & opcode) == 0b1101 {
+                            let last_operand = if shift_spec & 0xff0 == 0 {
+                                // No shift, so the operand is just a register
+                                Operand::Reg(Reg::from_u8(Rm))
+                            } else {
+                                Operand::RegShift(RegShift::from_raw(shift_spec))
+                            };
+
+                            match inst.opcode {
+                                Opcode::MOV
+                                |Opcode::MVN => {
                                     if self.should_is_must {
                                         if Rn != 0 {
                                             return Err(DecodeError::Nonconforming);
                                         }
                                     }
-                                    // MOV or MVN
                                     inst.operands = [
                                         Operand::Reg(Reg::from_u8(Rd)),
-                                        Operand::Reg(Reg::from_u8(Rm)),
+                                        last_operand,
                                         Operand::Nothing,
                                         Operand::Nothing
                                     ];
-                                } else {
+                                }
+
+                                Opcode::CMP
+                                |Opcode::CMN => {
+                                    if self.should_is_must {
+                                        if Rd != 0 {
+                                            return Err(DecodeError::Nonconforming);
+                                        }
+                                    }
                                     inst.operands = [
-                                        Operand::Reg(Reg::from_u8(Rd)),
                                         Operand::Reg(Reg::from_u8(Rn)),
-                                        Operand::Reg(Reg::from_u8(Rm)),
+                                        last_operand,
+                                        Operand::Nothing,
                                         Operand::Nothing
                                     ];
                                 }
-                            } else {
-                                if self.should_is_must {
-                                    if opcode == 0b1101 && Rn != 0 {
-                                        // Rn "should" be zero
-                                        return Err(DecodeError::Nonconforming);
-                                    }
-                                }
 
-                                inst.operands = [
-                                    Operand::Reg(Reg::from_u8(Rd)),
-                                    Operand::Reg(Reg::from_u8(Rn)),
-                                    Operand::RegShift(RegShift::from_raw(shift_spec)),
-                                    Operand::Nothing
-                                ];
+                                _ => {
+                                    inst.operands = [
+                                        Operand::Reg(Reg::from_u8(Rd)),
+                                        Operand::Reg(Reg::from_u8(Rn)),
+                                        last_operand,
+                                        Operand::Nothing
+                                    ];
+                                }
                             }
                         } else {
                     //    known 0 because it and bit 5 are not both 1 --v
@@ -2725,7 +2735,24 @@ impl Decoder<ARMv7> for InstDecoder {
                         inst.opcode = Opcode::ADR;
                     }
                     match opcode {
+                        // CMP/CMN (immediate)
+                        0b1010 | 0b1011 => {
+                            // According to A8-368, there are 4 bits right above the immediate that
+                            // are reserved and should be zero
+                            if self.should_is_must && (word >> 12) as u8 & 0b1111 != 0 {
+                                return Err(DecodeError::Nonconforming);
+                            }
+                            // compare has no destination register, only a source.
+                            inst.operands = [
+                                Operand::Reg(Reg::from_u8(Rn)),
+                                Operand::Imm32(imm),
+                                Operand::Nothing,
+                                Operand::Nothing,
+                            ];
+                        }
+                        // MOV (immediate)
                         0b1101 => {
+                            // mov has no source *register*, only the immediate being moved.
                             inst.operands = [
                                 Operand::Reg(Reg::from_u8(Rd)),
                                 Operand::Imm32(imm),
diff --git a/tests/armv7/mod.rs b/tests/armv7/mod.rs
index fc909af..58f7e0c 100644
--- a/tests/armv7/mod.rs
+++ b/tests/armv7/mod.rs
@@ -527,13 +527,14 @@ fn test_decode_arithmetic() {
         Instruction {
             condition: ConditionCode::AL,
             opcode: Opcode::MOV,
-            operands: [Operand::Reg(Reg::from_u8(3)), Operand::Reg(Reg::from_u8(0)), Operand::RegShift(RegShift::from_raw(0x143)), Operand::Nothing],
+            operands: [Operand::Reg(Reg::from_u8(3)), Operand::RegShift(RegShift::from_raw(0x143)), Operand::Nothing, Operand::Nothing],
             s: false,
             thumb_w: false,
             thumb: false,
             wide: false,
         }
     );
+    test_display([0x43, 0x31, 0xa0, 0xe1], "mov r3, r3, asr 2");
     test_decode(
         [0x01, 0x50, 0x43, 0xe2],
         Instruction {
@@ -704,7 +705,6 @@ fn test_decode_mul() {
     );
 }
 
-#[test]
 fn test_register_shift_rotate() {
     test_armv6([0xec, 0x02, 0x00, 0x00], "andeq r0, r0, ip, ror 5");
     test_armv6([0xa0, 0x33, 0x0b, 0x00], "andeq r3, fp, r0, lsr 7");
@@ -730,6 +730,20 @@ fn test_decode_mrc2() {
     test_armv6([0xbc, 0xec, 0xff, 0xfe], "mrc2 p12, 7, lr, c15, c12, 5");
 }
 
+#[test]
+fn test_cmp_immediate_decode() {
+    test_all([0xaf, 0x00, 0x56, 0xe3], "cmps r6, 0xaf");
+    test_all([0xaf, 0x00, 0x76, 0xe3], "cmns r6, 0xaf");
+}
+
+#[test]
+fn test_cmp_register_decode() {
+    test_all([0x01, 0x00, 0x52, 0xe1], "cmps r2, r1");
+    test_all([0x01, 0x03, 0x52, 0xe1], "cmps r2, r1, lsl 6");
+    test_all([0x01, 0x00, 0x72, 0xe1], "cmns r2, r1");
+    test_all([0x01, 0x03, 0x72, 0xe1], "cmns r2, r1, lsl 6");
+}
+
 static INSTRUCTION_BYTES: [u8; 4 * 60] = [
         0x24, 0xc0, 0x9f, 0xe5,
         0x00, 0xb0, 0xa0, 0xe3,