Merge pull request #5183 from Sonicadvance1/35

Frontend: Only decode REX if it is at the correct location

FEXCore/Source/Interface/Core/Frontend.cpp

@@ -848,6 +848,7 @@ bool Decoder::DecodeInstructionImpl(uint64_t PC) {
       switch (EscapeOp) {
       case 0x0F:
         [[unlikely]] { // 3DNow!
+          DecodeREXIfValid(-2);
           // 3DNow! Instruction Encoding: 0F 0F [ModRM] [SIB] [Displacement] [Opcode]
           // Decode ModRM
           uint8_t ModRMByte = ReadByte();
@@ -872,6 +873,7 @@ bool Decoder::DecodeInstructionImpl(uint64_t PC) {
           break;
         }
       case 0x38: { // F38 Table!
+        DecodeREXIfValid(-2);
         constexpr uint16_t PF_38_NONE = 0;
         constexpr uint16_t PF_38_66 = (1U << 0);
         constexpr uint16_t PF_38_F2 = (1U << 1);
@@ -897,11 +899,11 @@ bool Decoder::DecodeInstructionImpl(uint64_t PC) {
           DecodeInst->Flags &= ~DecodeFlags::FLAG_OPERAND_SIZE;
           DecodeFlags::PopOpAddrIf(&DecodeInst->Flags, DecodeFlags::FLAG_OPERAND_SIZE_LAST);
         }
-
         return NormalOpHeader(&FEXCore::X86Tables::H0F38TableOps[LocalOp], LocalOp);
         break;
       }
       case 0x3A: { // F3A Table!
+        DecodeREXIfValid(-2);
         constexpr uint16_t PF_3A_NONE = 0;
         constexpr uint16_t PF_3A_66 = (1 << 0);
         constexpr uint16_t PF_3A_REX = (1 << 1);
@@ -931,6 +933,7 @@ bool Decoder::DecodeInstructionImpl(uint64_t PC) {
           bool NoOverlay = (FEXCore::X86Tables::SecondBaseOps[EscapeOp].Flags & InstFlags::FLAGS_NO_OVERLAY) != 0;
           bool NoOverlay66 = (FEXCore::X86Tables::SecondBaseOps[EscapeOp].Flags & InstFlags::FLAGS_NO_OVERLAY66) != 0;
 
+          DecodeREXIfValid(-2);
           if (NoOverlay) { // This section of the table ignores prefix extention
             return NormalOpHeader(&FEXCore::X86Tables::SecondBaseOps[EscapeOp], EscapeOp);
           } else if (LastEscapePrefix == 0xF3) { // REP
@@ -1010,29 +1013,9 @@ bool Decoder::DecodeInstructionImpl(uint64_t PC) {
         }
 
         if (Info->Type == FEXCore::X86Tables::TYPE_REX_PREFIX) {
-          DecodeInst->Flags |= DecodeFlags::FLAG_REX_PREFIX;
-
-          // Widening displacement
-          if (Op & 0b1000) {
-            DecodeInst->Flags |= DecodeFlags::FLAG_REX_WIDENING;
-            DecodeFlags::PushOpAddr(&DecodeInst->Flags, DecodeFlags::FLAG_WIDENING_SIZE_LAST);
-          }
-
-          // XGPR_B bit set
-          if (Op & 0b0001) {
-            DecodeInst->Flags |= DecodeFlags::FLAG_REX_XGPR_B;
-          }
-
-          // XGPR_X bit set
-          if (Op & 0b0010) {
-            DecodeInst->Flags |= DecodeFlags::FLAG_REX_XGPR_X;
-          }
-
-          // XGPR_R bit set
-          if (Op & 0b0100) {
-            DecodeInst->Flags |= DecodeFlags::FLAG_REX_XGPR_R;
-          }
+          DecodeInst->REXIndex = InstructionSize;
         } else {
+          DecodeREXIfValid();
           return NormalOpHeader(Info, Op);
         }
 
@@ -1048,6 +1031,37 @@ bool Decoder::DecodeInstructionImpl(uint64_t PC) {
   return true;
 }
 
+void Decoder::DecodeREXIfValid(int8_t ExpectedOffset) {
+  LOGMAN_THROW_A_FMT(ExpectedOffset < 0, "Expecting an negative offset for the REX offset!");
+  const int8_t REXIndex = InstructionSize + ExpectedOffset;
+
+  if (DecodeInst->REXIndex != 0 && DecodeInst->REXIndex == REXIndex) {
+    const uint8_t Op = Instruction[REXIndex - 1];
+    DecodeInst->Flags |= DecodeFlags::FLAG_REX_PREFIX;
+
+    // Widening displacement
+    if (Op & 0b1000) {
+      DecodeInst->Flags |= DecodeFlags::FLAG_REX_WIDENING;
+      DecodeFlags::PushOpAddr(&DecodeInst->Flags, DecodeFlags::FLAG_WIDENING_SIZE_LAST);
+    }
+
+    // XGPR_B bit set
+    if (Op & 0b0001) {
+      DecodeInst->Flags |= DecodeFlags::FLAG_REX_XGPR_B;
+    }
+
+    // XGPR_X bit set
+    if (Op & 0b0010) {
+      DecodeInst->Flags |= DecodeFlags::FLAG_REX_XGPR_X;
+    }
+
+    // XGPR_R bit set
+    if (Op & 0b0100) {
+      DecodeInst->Flags |= DecodeFlags::FLAG_REX_XGPR_R;
+    }
+  }
+}
+
 Decoder::DecodedBlockStatus Decoder::DecodeInstruction(uint64_t PC) {
   // Will be set if DecodeInstructionImpl tries to read non-executable memory
   HitNonExecutableRange = false;

FEXCore/Source/Interface/Core/Frontend.h

@@ -112,6 +112,8 @@ private:
   bool NormalOp(const FEXCore::X86Tables::X86InstInfo* Info, uint16_t Op, DecodedHeader Options = {});
   bool NormalOpHeader(const FEXCore::X86Tables::X86InstInfo* Info, uint16_t Op);
 
+  void DecodeREXIfValid(int8_t ExpectedOffset = -1);
+
   static constexpr size_t DefaultDecodedBufferSize = 0x10000;
   FEXCore::X86Tables::DecodedInst* DecodedBuffer {};
   Utils::PoolBufferWithTimedRetirement<FEXCore::X86Tables::DecodedInst*, 5000, 500> PoolObject;

FEXCore/Source/Interface/Core/X86Tables/X86Tables.h

@@ -218,6 +218,7 @@ struct DecodedInst {
   uint8_t ModRM;
   uint8_t SIB;
   uint8_t InstSize;
+  int8_t REXIndex;
 };
 
 union ModRMDecoded {

unittests/ASM/FEX_bugs/REX/0F_38.asm

@@ -0,0 +1,35 @@
+%ifdef CONFIG
+{
+  "RegData": {
+    "RAX": "0x424446484a4c4e50",
+    "RCX": "0x00000000d2af0486",
+    "R8": "0"
+  }
+}
+%endif
+
+mov rax, 0x4142434445464748
+mov rcx, 0x4142434445464748
+
+mov r8, 0
+
+lea rbx, [rel .data]
+jmp .test
+.test:
+
+; adcx rax, [rbx]
+; Real encoding: 0x66, 0x48, 0x0f, 0x38, 0xf6, 0x03
+; Add a dummy REX prefix that enables everything. Really mess up FEX's cumulative usage.
+db 0x4f, 0x66, 0x48, 0x0f, 0x38, 0xf6, 0x03
+
+; crc32 ecx, dword [rbx]
+; Real encoding: 0xf2, 0x0f, 0x38, 0xf1, 0x0b
+; Add a dummy rex encoding with the widening bit set.
+; If FEX parsed this incorrectly, then it converts the crc in to a 64-bit version.
+db 0x48, 0xf2, 0x0f, 0x38, 0xf1, 0x0b
+
+hlt
+
+align 16
+.data:
+dq 0x0102030405060708

unittests/ASM/FEX_bugs/REX/0F_3A.asm

@@ -0,0 +1,30 @@
+%ifdef CONFIG
+{
+  "RegData": {
+    "RAX": "0x0000000055565758",
+    "RCX": "0x5152535455565758",
+    "R8": "0"
+  }
+}
+%endif
+
+mov rax, 0x4142434445464748
+mov rcx, 0x4142434445464748
+mov r8, 0
+movups xmm0, [rel .data]
+jmp .test
+.test:
+
+; pextrd eax, xmm0, 0
+; Real encoding: 0x66, 0x0f, 0x3a, 0x16, 0xc0, 0x00
+; Add a NOP REX encoding. Would convert `eax` to `rax` if decoded incorrectly.
+db 0x4f, 0x66, 0x0f, 0x3a, 0x16, 0xc0, 0x00
+; pextrq rcx, xmm0, 0
+; Real encoding: 0x66, 0x48, 0x0f, 0x3a, 0x16, 0xc1, 0x00
+; Add a NOP REX encoding, should do nothing. Might convert rcx to ecx if only first REX decoded.
+db 0x47, 0x66, 0x48, 0x0f, 0x3a, 0x16, 0xc1, 0x00
+hlt
+
+align 16
+.data:
+dq 0x5152535455565758, 0x6162636465666768

unittests/ASM/FEX_bugs/REX/DDDNow.asm

@@ -0,0 +1,31 @@
+%ifdef CONFIG
+{
+  "RegData": {
+    "RAX": "0x0506070801020304",
+    "MM0": "0x0506070801020304"
+  },
+  "HostFeatures": ["3DNOW"]
+}
+%endif
+
+femms
+mov rax, 0x4142434445464748
+
+mov r8, 0
+
+lea rbx, [rel .data]
+jmp .test
+.test:
+
+; pswapd mm0, [rbx]
+; Real encoding: 0x0f, 0x0f, 0x03, 0xbb
+; Add a NOP REX encoding between a volatile REX and the 3DNow! instruction.
+; FEX accidentally being cumulative will cause rbx to convert to r8.
+db 0x41, 0x40, 0x0f, 0x0f, 0x03, 0xbb
+
+movd rax, mm0
+hlt
+
+align 16
+.data:
+dq 0x0102030405060708

unittests/ASM/FEX_bugs/REX/Primary.asm

@@ -0,0 +1,37 @@
+%ifdef CONFIG
+{
+  "RegData": {
+    "RAX": "0x4142434445464e50",
+    "RCX": "0x000000004a4c4e50",
+    "R8": "0x4142434445464748",
+    "R9": "0x4142434445464748"
+  }
+}
+%endif
+
+; FEX-Emu had a bug where REX was not correctly ignored if it was placed at the wrong location.
+; "Wrong" means it wasn't encoded just before the opcode byte.
+; This can be done for multiple reasons, either padding or anti-emulation.
+mov rax, 0x4142434445464748
+mov rcx, 0x4142434445464748
+mov r8, 0x4142434445464748
+mov r9, 0x4142434445464748
+
+lea rbx, [rel .data]
+jmp .test
+.test:
+
+; add r8w, [rbx]
+; Real encoding: 0x66, 0x44, 0x03, 0x03
+; Swap operand-size override and REX. Converts r8 to rax, and stays a 16-bit operation.
+db 0x44, 0x66, 0x03, 0x03
+
+; add r9, [rbx]
+; Real encoding: 0x4c, 0x03, 0x0b
+; Add extraneous segment-overide between REX prefix and op, changes r9 to rcx, and 64-bit to 32-bit.
+db 0x4c, 0x2e, 0x03, 0x0b
+hlt
+
+align 16
+.data:
+dq 0x0102030405060708

unittests/ASM/FEX_bugs/REX/Primary_2.asm

@@ -0,0 +1,31 @@
+%ifdef CONFIG
+{
+  "RegData": {
+    "RAX": "0x424446484a4c4e50",
+    "RCX": "0x4142434445464748",
+    "R8": "0x4142434445464748",
+    "R9": "0x4142434445464748"
+  }
+}
+%endif
+
+mov rax, 0x4142434445464748
+mov rcx, 0x4142434445464748
+mov r8, 0x4142434445464748
+mov r9, 0x4142434445464748
+
+lea rbx, [rel .data]
+jmp .test
+.test:
+
+; add rax, [rbx]
+; Real encoding: 0x44, 0x03, 0x03
+; Add additional false REX as padding that would convert `rax` to `r8`.
+; FEX treated REX prefixes as cumulative at one point.
+db 0x44, 0x48, 0x03, 0x03
+
+hlt
+
+align 16
+.data:
+dq 0x0102030405060708

unittests/ASM/FEX_bugs/REX/TwoByte.asm

@@ -0,0 +1,33 @@
+%ifdef CONFIG
+{
+  "RegData": {
+    "RAX": "0x828486888a8c8e90",
+    "RBX": "0x000000008a8c8e90",
+    "RCX": "0x4142434445464748",
+    "RDX": "0x0000000045464748",
+    "R8": "1",
+    "R9": "1"
+  }
+}
+%endif
+
+mov rax, 0x4142434445464748
+mov rbx, 0x4142434445464748
+mov rcx, 0x4142434445464748
+mov rdx, 0x4142434445464748
+mov r8, 1
+mov r9, 1
+jmp .test
+.test:
+
+; xadd rax, rcx
+; Real encoding: 0x48, 0x0f, 0xc1, 0xc8
+; For cumulative decode errors, add a REX with all bits set. Will convert rax to r8, and rcx to r9.
+db 0x4f, 0x48, 0x0f, 0xc1, 0xc8
+
+; xadd ebx, edx
+; Real encoding: 0x0f, 0xc1, 0xd3
+; Add a nop-prefix pad between the opcode and full REX.
+db 0x4f, 0x2e, 0x0f, 0xc1, 0xd3
+
+hlt