pusha/popa/push-imm memory sizes

5 files changed, 75 insertions, 15 deletions

diff --git a/CHANGELOG b/CHANGELOG
index b42570b..3af20cc 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -11,9 +11,8 @@
   (RegSpec::xmm, RegSpec::q, RegSpec::d, RegSpec::st, etc)
 * for uarch-specific decoding, there is now a feature bit for Intel Key Locker. this corrects an
   issue where Key Locker instructions would decode under AMD-specific decoders.
-* push-immediate, pushf, popf, enter, leave, and xlat now all report a correct memory
-  access size, fixing the prior behavior of reporting no memory access size at
-  all
+* push-immediate, pushf, popf, pusha, popa, enter, leave, and xlat now all report a correct memory
+  access size, fixing the prior behavior of reporting no memory access size at all
 * table load/store instructions (lgdt, lidt, lldt, sgdt, sidt, sldt) have correct (mode-dependent)
   memory access sizes, rather than incorrectly varying on operand-size overrides.
 * 64-bit mode: mov seg-to-reg uses 32-bit GPRs for the destination rather than 16-bit.
diff --git a/src/protected_mode/mod.rs b/src/protected_mode/mod.rs
index 5ecfe01..9fd396c 100644
--- a/src/protected_mode/mod.rs
+++ b/src/protected_mode/mod.rs
@@ -1102,8 +1102,11 @@ pub enum Opcode {
     LEA,
     NOP,
     PREFETCHNTA,
+    /// this variant was named incorrectly and will change to `PREFETCHT0` in the future.
     PREFETCH0,
+    /// this variant was named incorrectly and will change to `PREFETCHT1` in the future.
     PREFETCH1,
+    /// this variant was named incorrectly and will change to `PREFETCHT2` in the future.
     PREFETCH2,
 //    XCHG,
     POPF,
@@ -3792,7 +3795,8 @@ enum OperandCase {
     G_E_xmm_Ib,
     AL_Ibs,
     AX_Ivd,
-    Ivs,
+    PushIbs,
+    PushIvs,
     ModRM_0x83,
     Ed_G_xmm,
     G_Ed_xmm,
@@ -3964,11 +3968,16 @@ enum OperandCase {
 #[repr(u16)]
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
 enum OperandCode {
-    Ivs = OperandCodeBuilder::new().operand_case(OperandCase::Ivs).bits(),
     I_3 = OperandCodeBuilder::new().operand_case(OperandCase::I_3).bits(),
     Nothing = OperandCodeBuilder::new().operand_case(OperandCase::Nothing).bits(),
     Ib = OperandCodeBuilder::new().operand_case(OperandCase::Ib).bits(),
     Ibs = OperandCodeBuilder::new().only_imm().operand_case(OperandCase::Ibs).bits(),
+    // `push` of an immediate means we have to set a memory size, but `Ibs` is also the operand
+    // form for short relative (conditional) branches. to avoid the conditional in that relatively
+    // hot path, push is a different operand code so we can shove a `mem_size = 8` into the
+    // instruction later.
+    PushIbs = OperandCodeBuilder::new().operand_case(OperandCase::PushIbs).bits(),
+    PushIvs = OperandCodeBuilder::new().operand_case(OperandCase::PushIvs).bits(),
     Jvds = OperandCodeBuilder::new().only_imm().operand_case(OperandCase::Jvds).bits(),
     Yv_Xv = OperandCodeBuilder::new().operand_case(OperandCase::Yv_Xv).bits(),
 
@@ -4445,9 +4454,9 @@ const OPCODES: [OpcodeRecord; 256] = [
     OpcodeRecord::new(Interpretation::Prefix, OperandCode::Nothing),
     OpcodeRecord::new(Interpretation::Prefix, OperandCode::Nothing),
     OpcodeRecord::new(Interpretation::Prefix, OperandCode::Nothing),
-    OpcodeRecord::new(Interpretation::Instruction(Opcode::PUSH), OperandCode::Ivs),
+    OpcodeRecord::new(Interpretation::Instruction(Opcode::PUSH), OperandCode::PushIvs),
     OpcodeRecord::new(Interpretation::Instruction(Opcode::IMUL), OperandCode::Gv_Ev_Iv),
-    OpcodeRecord::new(Interpretation::Instruction(Opcode::PUSH), OperandCode::Ibs),
+    OpcodeRecord::new(Interpretation::Instruction(Opcode::PUSH), OperandCode::PushIbs),
     OpcodeRecord::new(Interpretation::Instruction(Opcode::IMUL), OperandCode::Gv_Ev_Ib),
     OpcodeRecord::new(Interpretation::Instruction(Opcode::INS), OperandCode::Yb_DX),
     OpcodeRecord::new(Interpretation::Instruction(Opcode::INS), OperandCode::Yv_DX),
@@ -5871,6 +5880,18 @@ fn read_operands<
         OperandCase::Internal | OperandCase::Gv_M |
         OperandCase::Ibs | OperandCase::Jvds => {
         }
+        OperandCase::PushIbs => {
+            instruction.mem_size = 8;
+            instruction.imm = read_imm_signed(words, 1)? as u32;
+            instruction.operands[0] = OperandSpec::ImmI8;
+            sink.record(
+                words.offset() as u32 * 8 - 8,
+                words.offset() as u32 * 8 - 1,
+                InnerDescription::Number("1-byte immediate", instruction.imm as i64)
+                    .with_id(words.offset() as u32 * 8)
+            );
+            instruction.operand_count = 1;
+        }
         OperandCase::SingleMMMOper => {
             instruction.operands[0] = mem_oper;
             instruction.operand_count = 1;
@@ -6377,7 +6398,9 @@ fn read_operands<
                     .with_id(words.offset() as u32 * 8 - bank as u8 as u32 * 8 + 1)
             );
         }
-        OperandCase::Ivs => {
+        OperandCase::PushIvs => {
+            instruction.mem_size = 4;
+
             if !instruction.prefixes.operand_size() {
                 instruction.imm = read_imm_unsigned(words, 4)?;
                 instruction.operands[0] = OperandSpec::ImmI32;
@@ -6449,6 +6472,10 @@ fn read_operands<
                 instruction.mem_size = 4;
             } else if instruction.opcode == Opcode::XLAT {
                 instruction.mem_size = 1;
+            } else if instruction.opcode == Opcode::PUSHA {
+                instruction.mem_size = 4 * 8;
+            } else if instruction.opcode == Opcode::POPA {
+                instruction.mem_size = 4 * 8;
             }
             instruction.operands[0] = OperandSpec::Nothing;
             instruction.operand_count = 0;
@@ -8956,11 +8983,12 @@ fn read_operands<
         OperandCase::AbsFar => {
             instruction.operands[0] = OperandSpec::AbsoluteFarAddress;
             instruction.operand_count = 1;
-            instruction.mem_size = 0;
             // read segment
             let addr_size = if instruction.prefixes.operand_size() {
+                instruction.mem_size = 2 + 2;
                 2
             } else {
+                instruction.mem_size = 4 + 2;
                 4
             };
             instruction.imm = read_num(words, addr_size)?;
diff --git a/src/real_mode/mod.rs b/src/real_mode/mod.rs
index 89b1162..a985d91 100644
--- a/src/real_mode/mod.rs
+++ b/src/real_mode/mod.rs
@@ -3822,7 +3822,8 @@ enum OperandCase {
     G_E_xmm_Ib,
     AL_Ibs,
     AX_Ivd,
-    Ivs,
+    PushIbs,
+    PushIvs,
     ModRM_0x83,
     Ed_G_xmm,
     G_Ed_xmm,
@@ -3994,11 +3995,16 @@ enum OperandCase {
 #[repr(u16)]
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
 enum OperandCode {
-    Ivs = OperandCodeBuilder::new().operand_case(OperandCase::Ivs).bits(),
     I_3 = OperandCodeBuilder::new().operand_case(OperandCase::I_3).bits(),
     Nothing = OperandCodeBuilder::new().operand_case(OperandCase::Nothing).bits(),
     Ib = OperandCodeBuilder::new().operand_case(OperandCase::Ib).bits(),
     Ibs = OperandCodeBuilder::new().only_imm().operand_case(OperandCase::Ibs).bits(),
+    // `push` of an immediate means we have to set a memory size, but `Ibs` is also the operand
+    // form for short relative (conditional) branches. to avoid the conditional in that relatively
+    // hot path, push is a different operand code so we can shove a `mem_size = 8` into the
+    // instruction later.
+    PushIbs = OperandCodeBuilder::new().operand_case(OperandCase::PushIbs).bits(),
+    PushIvs = OperandCodeBuilder::new().operand_case(OperandCase::PushIvs).bits(),
     Jvds = OperandCodeBuilder::new().only_imm().operand_case(OperandCase::Jvds).bits(),
     Yv_Xv = OperandCodeBuilder::new().operand_case(OperandCase::Yv_Xv).bits(),
 
@@ -4475,9 +4481,9 @@ const OPCODES: [OpcodeRecord; 256] = [
     OpcodeRecord::new(Interpretation::Prefix, OperandCode::Nothing),
     OpcodeRecord::new(Interpretation::Prefix, OperandCode::Nothing),
     OpcodeRecord::new(Interpretation::Prefix, OperandCode::Nothing),
-    OpcodeRecord::new(Interpretation::Instruction(Opcode::PUSH), OperandCode::Ivs),
+    OpcodeRecord::new(Interpretation::Instruction(Opcode::PUSH), OperandCode::PushIvs),
     OpcodeRecord::new(Interpretation::Instruction(Opcode::IMUL), OperandCode::Gv_Ev_Iv),
-    OpcodeRecord::new(Interpretation::Instruction(Opcode::PUSH), OperandCode::Ibs),
+    OpcodeRecord::new(Interpretation::Instruction(Opcode::PUSH), OperandCode::PushIbs),
     OpcodeRecord::new(Interpretation::Instruction(Opcode::IMUL), OperandCode::Gv_Ev_Ib),
     OpcodeRecord::new(Interpretation::Instruction(Opcode::INS), OperandCode::Yb_DX),
     OpcodeRecord::new(Interpretation::Instruction(Opcode::INS), OperandCode::Yv_DX),
@@ -5902,6 +5908,18 @@ fn read_operands<
         OperandCase::Internal | OperandCase::Gv_M |
         OperandCase::Ibs | OperandCase::Jvds => {
         }
+        OperandCase::PushIbs => {
+            instruction.mem_size = 8;
+            instruction.imm = read_imm_signed(words, 1)? as u32;
+            instruction.operands[0] = OperandSpec::ImmI8;
+            sink.record(
+                words.offset() as u32 * 8 - 8,
+                words.offset() as u32 * 8 - 1,
+                InnerDescription::Number("1-byte immediate", instruction.imm as i64)
+                    .with_id(words.offset() as u32 * 8)
+            );
+            instruction.operand_count = 1;
+        }
         OperandCase::SingleMMMOper => {
             instruction.operands[0] = mem_oper;
             instruction.operand_count = 1;
@@ -6414,7 +6432,9 @@ fn read_operands<
                     .with_id(words.offset() as u32 * 8 - bank as u8 as u32 * 8 + 1)
             );
         }
-        OperandCase::Ivs => {
+        OperandCase::PushIvs => {
+            instruction.mem_size = 4;
+
             if instruction.prefixes.operand_size() {
                 instruction.imm = read_imm_unsigned(words, 4)?;
                 instruction.operands[0] = OperandSpec::ImmI32;
@@ -6486,6 +6506,10 @@ fn read_operands<
                 instruction.mem_size = 2;
             } else if instruction.opcode == Opcode::XLAT {
                 instruction.mem_size = 1;
+            } else if instruction.opcode == Opcode::PUSHA {
+                instruction.mem_size = 2 * 8;
+            } else if instruction.opcode == Opcode::POPA {
+                instruction.mem_size = 2 * 8;
             }
             instruction.operands[0] = OperandSpec::Nothing;
             instruction.operand_count = 0;
@@ -8998,11 +9022,12 @@ fn read_operands<
         OperandCase::AbsFar => {
             instruction.operands[0] = OperandSpec::AbsoluteFarAddress;
             instruction.operand_count = 1;
-            instruction.mem_size = 0;
             // read segment
             let addr_size = if !instruction.prefixes.operand_size() {
+                instruction.mem_size = 2 + 2;
                 2
             } else {
+                instruction.mem_size = 4 + 2;
                 4
             };
             instruction.imm = read_num(words, addr_size)?;
diff --git a/test/protected_mode/operand.rs b/test/protected_mode/operand.rs
index d90e6ba..0c970de 100644
--- a/test/protected_mode/operand.rs
+++ b/test/protected_mode/operand.rs
@@ -75,4 +75,8 @@ fn test_implied_memory_width() {
     assert_eq!(mem_size_of(&[0x0f, 0xa8]), Some(4));
     // pop gs
     assert_eq!(mem_size_of(&[0x0f, 0xa9]), Some(4));
+
+    // callf 0xcf23:0x2d62
+    assert_eq!(mem_size_of(&[0x9a, 0x62, 0x2d, 0x00, 0x00, 0x23, 0xcf]), Some(6));
+    assert_eq!(mem_size_of(&[0x66, 0x9a, 0x62, 0x2d, 0x23, 0xcf]), Some(4));
 }
diff --git a/test/real_mode/operand.rs b/test/real_mode/operand.rs
index 9b37f36..fb2ce39 100644
--- a/test/real_mode/operand.rs
+++ b/test/real_mode/operand.rs
@@ -48,4 +48,8 @@ fn test_implied_memory_width() {
     // "vmaskmovdqu xmm0, xmm1"
     assert_eq!(mem_size_of(&[0xc4, 0xe1, 0x79, 0xf7, 0xc1]), Some(16));
     assert_eq!(mem_size_of(&[0x67, 0xc4, 0xe1, 0x79, 0xf7, 0xc1]), Some(16));
+
+    // callf 0xcf23:0x2d62
+    assert_eq!(mem_size_of(&[0x9a, 0x62, 0x2d, 0x23, 0xcf]), Some(4));
+    assert_eq!(mem_size_of(&[0x66, 0x9a, 0x62, 0x2d, 0x00, 0x00, 0x23, 0xcf]), Some(6));
 }