The issues with X86 prefixes

[avt77]

Summary

The current version of LLVM X86 disassembler incorrectly interprets some possible sets of x86 prefixes (see PR7709, 17697, 19251, 21640, etc.). This patch is the first step to resolve those issues. In particulary this patch could close the first two PRs from the above and should be able to close some others soon.

[avt77]

I found one issue in the current version of the patch that's why I'm asking to postpone its review until the next version: hope to publish it in some hours or tomorrow.

[avt77]

I fixed the last minute issue raised in the first version.

[craig.topper]

lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp

384 ↗

(On Diff #111344)

Don't leave around commented out code

394 ↗

(On Diff #111344)

Remove commented out code

421 ↗

(On Diff #111344)

First '&&' should be on the previous line.

[avt77]

I did fixes raised by Craig.

[craig.topper]

lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp

295 ↗

(On Diff #112141)

What about 32-bit mode?

306 ↗

(On Diff #112141)

wee->we

[avt77]

lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp

295 ↗

(On Diff #112141)

I was sure that SSE2, etc. work on 64-bit CPUs only but it seems I was wrong: Wikipedia says:

The following IA-32 CPUs support SSE2:
Intel NetBurst-based CPUs (Pentium 4, Xeon, Celeron, Pentium D, Celeron D)
Intel Pentium M and Celeron M
Intel Atom
Transmeta Efficeon
VIA C7

OK, I'll fix it.

[avt77]

Craig's issues fixed.

[craig.topper]

Please add a 32-bit mode test as well.

lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp

993 ↗

(On Diff #112323)

Is this 64-bit mode checks till needed?

[avt77]

lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp

993 ↗

(On Diff #112323)

In fact we need

... else if (insn->mode != MODE_16BIT ....

I'll do it. Tnx.

[avt77]

32-bit tests added and the condition fixed properly.

[craig.topper]

LGTM

[RKSimon]

Reopening after reversion at rL311987

[RKSimon]

@echristo Please can you post the regressions you mentioned in rL311987 ?

[echristo]

The two I had were 0xf2666d and 0xf3666d that were broken with the earlier patch that I mentioned in my reversion email following up on the original.

[avt77]

D37262 covers all issues from this review plus some others that's why we don't need D36788 any more.

llvm/trunk/lib/Target/X86/Disassembler/X86DisassemblerDecoder.h

struct InternalInstruction {
   DisassemblerMode mode;
   // The start of the instruction, usable with the reader
   uint64_t startLocation;
   // The length of the instruction, in bytes
   size_t length;
 
   // Prefix state
 
+  // TODO: we're able to get rid off the following 2 arrays
   // 1 if the prefix byte corresponding to the entry is present; 0 if not
   uint8_t prefixPresent[0x100];
   // contains the location (for use with the reader) of the prefix byte
   uint64_t prefixLocations[0x100];
+  // The possible mandatory prefix
+  uint8_t mandatory_prefix;
   // The value of the vector extension prefix(EVEX/VEX/XOP), if present
   uint8_t vectorExtensionPrefix[4];
   // The type of the vector extension prefix
   VectorExtensionType vectorExtensionType;
   // The value of the REX prefix, if present
   uint8_t rexPrefix;
   // The location where a mandatory prefix would have to be (i.e., right before
   // the opcode, or right before the REX prefix if one is present).

llvm/trunk/lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp

static void dbgprintf(struct InternalInstruction* insn,
 
   va_start(ap, format);
   (void)vsnprintf(buffer, sizeof(buffer), format, ap);
   va_end(ap);
 
   insn->dlog(insn->dlogArg, buffer);
 }
 
+static bool isREX(struct InternalInstruction *insn, uint8_t prefix) {
+  if (insn->mode == MODE_64BIT)
+    return prefix >= 0x40 && prefix <= 0x4f;
+  return false;
+}
+
 /*
  * setPrefixPresent - Marks that a particular prefix is present at a particular
  *   location.
  *
  * @param insn      - The instruction to be marked as having the prefix.
  * @param prefix    - The prefix that is present.
  * @param location  - The location where the prefix is located (in the address
  *                    space of the instruction's reader).
  */
 static void setPrefixPresent(struct InternalInstruction* insn,
                                     uint8_t prefix,
                                     uint64_t location)
 {
+  uint8_t nextByte;
+  switch (prefix) {
+  case 0xf2:
+  case 0xf3:
+    if (lookAtByte(insn, &nextByte))
+      break;
+    // TODO:
+    //  1. There could be several 0x66
+    //  2. if (nextByte == 0x66) and nextNextByte != 0x0f then
+    //      it's not mandatory prefix
+    //  3. if (nextByte >= 0x40 && nextByte <= 0x4f) it's REX and we need
+    //     0x0f exactly after it to be mandatory prefix
+    if (isREX(insn, nextByte) || nextByte == 0x0f || nextByte == 0x66) {
+      // The last of 0xf2 /0xf3 is mandatory prefix
+      insn->mandatory_prefix = prefix;
+      insn->necessaryPrefixLocation = location;
+      break;
+    }
+    break;
+  case 0x66:
+    if (lookAtByte(insn, &nextByte))
+      break;
+    // 0x66 can't overwrite existing mandatory prefix and should be ignored
+    if (!insn->mandatory_prefix &&
+        (nextByte == 0x0f || isREX(insn, nextByte))) {
+      insn->mandatory_prefix = prefix;
+      insn->necessaryPrefixLocation = location;
+    }
+    break;
+  }
+  if (!insn->mandatory_prefix)
+    insn->necessaryPrefixLocation = location;
   insn->prefixPresent[prefix] = 1;
   insn->prefixLocations[prefix] = location;
 }
 
 /*
  * isPrefixAtLocation - Queries an instruction to determine whether a prefix is
  *   present at a given location.
  *
while (isPrefix) {
     /* If we fail reading prefixes, just stop here and let the opcode reader deal with it */
     if (consumeByte(insn, &byte))
       break;
 
     /*
      * If the byte is a LOCK/REP/REPNE prefix and not a part of the opcode, then
      * break and let it be disassembled as a normal "instruction".
      */
-    if (insn->readerCursor - 1 == insn->startLocation && byte == 0xf0)
+    if (insn->readerCursor - 1 == insn->startLocation && byte == 0xf0) // LOCK
       break;
 
-    if (insn->readerCursor - 1 == insn->startLocation
-        && (byte == 0xf2 || byte == 0xf3)
-        && !lookAtByte(insn, &nextByte))
-    {
+    if ((byte == 0xf2 || byte == 0xf3) && !lookAtByte(insn, &nextByte)) {
       /*
        * If the byte is 0xf2 or 0xf3, and any of the following conditions are
        * met:
        * - it is followed by a LOCK (0xf0) prefix
        * - it is followed by an xchg instruction
        * then it should be disassembled as a xacquire/xrelease not repne/rep.
        */
-      if ((byte == 0xf2 || byte == 0xf3) &&
-          ((nextByte == 0xf0) ||
+      if (((nextByte == 0xf0) ||
            ((nextByte & 0xfe) == 0x86 || (nextByte & 0xf8) == 0x90)))
         insn->xAcquireRelease = true;
       /*
        * Also if the byte is 0xf3, and the following condition is met:
        * - it is followed by a "mov mem, reg" (opcode 0x88/0x89) or
        *                       "mov mem, imm" (opcode 0xc6/0xc7) instructions.
        * then it should be disassembled as an xrelease not rep.
        */
       if (byte == 0xf3 &&
           (nextByte == 0x88 || nextByte == 0x89 ||
            nextByte == 0xc6 || nextByte == 0xc7))
         insn->xAcquireRelease = true;
       if (insn->mode == MODE_64BIT && (nextByte & 0xf0) == 0x40) {
         if (consumeByte(insn, &nextByte))
           return -1;
         if (lookAtByte(insn, &nextByte))
           return -1;
         unconsumeByte(insn);
       }
-      if (nextByte != 0x0f && nextByte != 0x90)
+      // If the current byte can't be mandatory prefix then it's a simple repeat
+      // prefix and should be elaborated as a separated instr
+      if (nextByte != 0x0f &&
+          // We can have f2 f3 f2 66 f2 0f (in any order) as a valid set
+          // of prefixes with the last one as a mandatory prefix (SSE/SSE2...)
+          nextByte != 0xf2 && nextByte != 0xf3 && nextByte != 0x66 &&
+          nextByte != 0x90 && !isREX(insn, nextByte))
         break;
     }
 
     switch (byte) {
     case 0xf0:  /* LOCK */
     case 0xf2:  /* REPNE/REPNZ */
     case 0xf3:  /* REP or REPE/REPZ */
       if (prefixGroups[0])
case 0x65:  /* GS segment override */
         return -1;
       }
       if (prefixGroups[1])
         dbgprintf(insn, "Redundant Group 2 prefix");
       prefixGroups[1] = true;
       setPrefixPresent(insn, byte, prefixLocation);
       break;
     case 0x66:  /* Operand-size override */
+      setPrefixPresent(insn, byte, prefixLocation);
+      if (!insn->mandatory_prefix) {
         if (prefixGroups[2])
           dbgprintf(insn, "Redundant Group 3 prefix");
         prefixGroups[2] = true;
+      }
       hasOpSize = true;
-      setPrefixPresent(insn, byte, prefixLocation);
       break;
     case 0x67:  /* Address-size override */
       if (prefixGroups[3])
         dbgprintf(insn, "Redundant Group 4 prefix");
       prefixGroups[3] = true;
       hasAdSize = true;
       setPrefixPresent(insn, byte, prefixLocation);
       break;
if (insn->mode == MODE_64BIT) {
           dbgprintf(insn, "Redundant REX prefix");
           return -1;
         }
 
         insn->rexPrefix = byte;
         insn->necessaryPrefixLocation = insn->readerCursor - 2;
 
         dbgprintf(insn, "Found REX prefix 0x%hhx", byte);
-      } else {
+      } else
         unconsumeByte(insn);
-        insn->necessaryPrefixLocation = insn->readerCursor - 1;
-      }
     } else {
       unconsumeByte(insn);
       insn->necessaryPrefixLocation = insn->readerCursor - 1;
     }
   }
 
   if (insn->mode == MODE_16BIT) {
     insn->registerSize       = (hasOpSize ? 4 : 2);
if (insn->vectorExtensionType == TYPE_EVEX) {
         break;
       }
 
       if (lFromXOP3of3(insn->vectorExtensionPrefix[2]))
         attrMask |= ATTR_VEXL;
     } else {
       return -1;
     }
+  } else if (insn->mode != MODE_16BIT && !insn->mandatory_prefix) {
+    // If we don't have mandatory prefix we should use "standard" prefixes here
+    if (insn->prefixPresent[0x66])
+      attrMask |= ATTR_OPSIZE;
+    if (insn->prefixPresent[0x67])
+      attrMask |= ATTR_ADSIZE;
+    if (insn->prefixPresent[0xf2])
+      attrMask |= ATTR_XD;
+    if (insn->prefixPresent[0xf3])
+      attrMask |= ATTR_XS;
   } else {
-    if (insn->mode != MODE_16BIT && isPrefixAtLocation(insn, 0x66, insn->necessaryPrefixLocation))
+    if (insn->mode != MODE_16BIT &&
+        isPrefixAtLocation(insn, 0x66, insn->necessaryPrefixLocation))
       attrMask |= ATTR_OPSIZE;
     else if (isPrefixAtLocation(insn, 0x67, insn->necessaryPrefixLocation))
       attrMask |= ATTR_ADSIZE;
     else if (isPrefixAtLocation(insn, 0xf3, insn->necessaryPrefixLocation))
       attrMask |= ATTR_XS;
     else if (isPrefixAtLocation(insn, 0xf2, insn->necessaryPrefixLocation))
       attrMask |= ATTR_XD;
   }

llvm/trunk/test/MC/Disassembler/X86/prefixes-i386.s

+// RUN: llvm-mc -disassemble %s -triple=i386-apple-darwin9
+
+// CHECK:  movl    %fs:24, %eax
+0x64 0xa1 0x18 0x00 0x00 0x00 # mov eax, dword ptr fs:[18h]
+
+# CHECK: rep
+# CHECK-NEXT: insb    %dx, %es:(%rdi)
+0xf3 0x6c #rep ins
+# CHECK: rep
+# CHECK-NEXT: insl    %dx, %es:(%rdi)
+0xf3 0x6d #rep ins
+# CHECK: rep
+# CHECK-NEXT: movsb   (%rsi), %es:(%rdi)
+0xf3 0xa4 #rep movs
+# CHECK: rep
+# CHECK-NEXT: movsl   (%rsi), %es:(%rdi)
+0xf3 0xa5 #rep movs
+# CHECK: rep
+# CHECK-NEXT: outsb   (%rsi), %dx
+0xf3 0x6e #rep outs
+# CHECK: rep
+# CHECK-NEXT: outsl   (%rsi), %dx
+0xf3 0x6f #rep outs
+# CHECK: rep
+# CHECK-NEXT: lodsb   (%rsi), %al
+0xf3 0xac #rep lods
+# CHECK: rep
+# CHECK-NEXT: lodsl   (%rsi), %eax
+0xf3 0xad #rep lods
+# CHECK: rep
+# CHECK-NEXT: stosb   %al, %es:(%rdi)
+0xf3 0xaa #rep stos
+# CHECK: rep
+# CHECK-NEXT: stosl   %eax, %es:(%rdi)
+0xf3 0xab #rep stos
+# CHECK: rep
+# CHECK-NEXT: cmpsb   %es:(%rdi), (%rsi)
+0xf3 0xa6 #rep cmps
+# CHECK: rep
+# CHECK-NEXT: cmpsl   %es:(%rdi), (%rsi)
+0xf3 0xa7 #repe cmps
+# CHECK: rep
+# CHECK-NEXT: scasb   %es:(%rdi), %al
+0xf3 0xae #repe scas
+# CHECK: rep
+# CHECK-NEXT: scasl   %es:(%rdi), %eax
+0xf3 0xaf #repe scas
+# CHECK: repne
+# CHECK-NEXT: cmpsb   %es:(%rdi), (%rsi)
+0xf2 0xa6 #repne cmps
+# CHECK: repne
+# CHECK-NEXT: cmpsl   %es:(%rdi), (%rsi)
+0xf2 0xa7 #repne cmps
+# CHECK: repne
+# CHECK-NEXT: scasb   %es:(%rdi), %al
+0xf2 0xae #repne scas
+# CHECK: repne
+# CHECK-NEXT: scasl   %es:(%rdi), %eax
+0xf2 0xaf #repne scas
+
+// CHECK: mulsd   %xmm7, %xmm7
+0x66 0xF3 0xF2 0x0F 0x59 0xFF
+// CHECK: mulss   %xmm7, %xmm7
+0x66 0xF2 0xF3 0x0F 0x59 0xFF
+// CHECK: mulpd   %xmm7, %xmm7
+0x66 0x0F 0x59 0xFF
+// CHECK: mulsd   %xmm7, %xmm7
+0xf2 0x66 0x0f 0x59 0xff

llvm/trunk/test/MC/Disassembler/X86/prefixes-x86_64.s

+// RUN: llvm-mc -disassemble %s -triple=x86_64-apple-darwin9
+
+// CHECK: mulsd   %xmm7, %xmm7
+0x66 0xF3 0xF2 0x0F 0x59 0xFF
+// CHECK: mulss   %xmm7, %xmm7
+0x66 0xF2 0xF3 0x0F 0x59 0xFF
+// CHECK: mulpd   %xmm7, %xmm7
+0x66 0x0F 0x59 0xFF
+// CHECK: mulsd   %xmm7, %xmm7
+0xf2 0x66 0x0f 0x59 0xff

llvm/trunk/test/MC/Disassembler/X86/prefixes.txt

 # RUN: llvm-mc --disassemble %s -triple=x86_64 | FileCheck %s
 
+# CHECK: rep
+# CHECK-NEXT: insb    %dx, %es:(%rdi)
+0xf3 0x6c #rep ins
+# CHECK: rep
+# CHECK-NEXT: insl    %dx, %es:(%rdi)
+0xf3 0x6d #rep ins
+# CHECK: rep
+# CHECK-NEXT: movsb   (%rsi), %es:(%rdi)
+0xf3 0xa4 #rep movs
+# CHECK: rep
+# CHECK-NEXT: movsl   (%rsi), %es:(%rdi)
+0xf3 0xa5 #rep movs
+# CHECK: rep
+# CHECK-NEXT: outsb   (%rsi), %dx
+0xf3 0x6e #rep outs
+# CHECK: rep
+# CHECK-NEXT: outsl   (%rsi), %dx
+0xf3 0x6f #rep outs
+# CHECK: rep
+# CHECK-NEXT: lodsb   (%rsi), %al
+0xf3 0xac #rep lods
+# CHECK: rep
+# CHECK-NEXT: lodsl   (%rsi), %eax
+0xf3 0xad #rep lods
+# CHECK: rep
+# CHECK-NEXT: stosb   %al, %es:(%rdi)
+0xf3 0xaa #rep stos
+# CHECK: rep
+# CHECK-NEXT: stosl   %eax, %es:(%rdi)
+0xf3 0xab #rep stos
+# CHECK: rep
+# CHECK-NEXT: cmpsb   %es:(%rdi), (%rsi)
+0xf3 0xa6 #rep cmps
+# CHECK: rep
+# CHECK-NEXT: cmpsl   %es:(%rdi), (%rsi)
+0xf3 0xa7 #repe cmps
+# CHECK: rep
+# CHECK-NEXT: scasb   %es:(%rdi), %al
+0xf3 0xae #repe scas
+# CHECK: rep
+# CHECK-NEXT: scasl   %es:(%rdi), %eax
+0xf3 0xaf #repe scas
+# CHECK: repne
+# CHECK-NEXT: cmpsb   %es:(%rdi), (%rsi)
+0xf2 0xa6 #repne cmps
+# CHECK: repne
+# CHECK-NEXT: cmpsl   %es:(%rdi), (%rsi)
+0xf2 0xa7 #repne cmps
+# CHECK: repne
+# CHECK-NEXT: scasb   %es:(%rdi), %al
+0xf2 0xae #repne scas
+# CHECK: repne
+# CHECK-NEXT: scasl   %es:(%rdi), %eax
+0xf2 0xaf #repne scas
+
 # CHECK: lock
 # CHECK-NEXT: orl $16, %fs:776
 0xf0 0x64 0x83 0x0c 0x25 0x08 0x03 0x00 0x00 0x10
 
 # CHECK: movq %fs:768, %rdi
 0x64 0x48 0x8b 0x3c 0x25 0x00 0x03 0x00 0x00
 
 # CHECK: rep

 0xf0 0x90
 
 # Test that immediate is printed correctly within opsize prefix
 # CHECK: addw    $-12, %ax
 0x66,0x83,0xc0,0xf4
 
 # Test that multiple redundant prefixes work (redundant, but valid x86).
 # CHECK: rep
-# CHECK-NEXT: rep
 # CHECK-NEXT: stosq
 0xf3 0xf3 0x48 0xab
 
 
 # Test that we can disassembler control registers above CR8
 # CHECK: movq %cr15, %rax
 0x44 0x0f 0x20 0xf8
 # CHECK: movq %dr15, %rax