[llvm-mc] fix 64-bit mode call disassembly to ignore opcode size prefix

[m4b]

Summary

This is a potential fix for disassembling unusual instruction sequences in 64-bit mode w.r.t the CALL instruction. It might be desirable to move the check somewhere else, etc., but it essentially mimics the special case handling with JCXZ in 16-bit mode.

The current behavior accepts the opcode size prefix and causes the call's immediate to stop disassembling after 2 bytes. When debugging sequences of instructions with this pattern, the disassembler output becomes extremely unreliable and essentially useless (if you jump midway into what lldb thinks is a unified instruction, you'll lose %rip). Thus the choice of ignoring the prefix and consuming all 4 bytes when disassembling a 64-bit mode binary.

Note: in Vol. 2A 3-99 the Intel spec states that CALL rel16 is N.S. N.S. is defined as:

Indicates an instruction syntax that requires an address override prefix in 64-bit mode
and is not supported. Using an address override prefix in 64-bit mode
may result in model-specific execution behavior. (Vol. 2A 3-7)

Since 0x66 is an operand override prefix we should be OK (although we may want to warn about 0x67 prefixes to 0xe8). On the CPUs I have tested with, they all ignore the 0x66 prefix in 64-bit mode.

[dougk]

code LGTM but "ignore opcode size" -> "ignore operand size" and even though Intel defines "N.S.", I'd spell out "Not Supported" in the comment so that it can reasonably stand alone.

[dougk]

JMP has the same issue. No reason not to try and fix both please.

[m4b]

fix for ignoring opcode prefixes for jmp 0xe9

Unfortunately all of the jcc instructions which use rel16 are broken as well; it doesn't seem very pretty to add a bunch of cases, but not sure what else to do without doing some refactoring or digging into other potential places to correctly disassemble these kinds of edge cases.

what do you think?

• added n.s. -> not supported;
• quick fix for jmp e9; using switch now, and e9 requires displacement and immediate explicitly set, xoring opsize alone will not suffice for correct disassembly;
• jmp 0xe9 x86-64 unit tests added;

[m4b]

Slightly more robust version, switching on all the relevant opcodes I could find for call/jmp/jcc.

Should probably do an audit of some sort to determine the extent of bad f64 subscript opcode table disassembly.

I'm still uncomfortable reassigning insn->immediateSize and insn->displacementSize in the getID() method, but not sure where else to do this. Removing the ATTR_OPSIZE does not affect jmp and jcc instructions unfortunately, as it does for call.

May need to fix this in the releavnt .td files or perhaps in the readImmediate() method at some point.

Added more unit tests, specifically for jzz cases with 0x66 prefix.

• current revision switches on relevant call/jmp/jcc instructions, and avoids breaking lea, et. al.
• added unit tests for jzz 0x66 cases and removed whitespace;

[craig.topper]

lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp

994 ↗

(On Diff #25397)

Why no opcodeType check here? Can't this alias?

1013 ↗

(On Diff #25397)

The indentation on this line is showing up funny in phabricator

[m4b]

Now checking for ONEBYTE opcodeType for call and jump, as per comment; added some more unit tests and fixed indentation.

[m4b]

FYI, I don't have write privileges, so I assume someone else needs to run arc land if this is ready to be committed and pushed?

Similarly for diff D9514

lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp

994 ↗

(On Diff #25397)

Nice catch! The instructions they would have messed up, like psubsb/vpsubsb, don't have any unit tests, so it wasn't showing up in testing. I'll check for a ONEBYTE opcodeType here, add the unit tests, and fix the identation problems in the next diff.

[m4b]

*ping* this has been sitting quite a while. As I said, I don't have write perms. So either some feedback, a rejection, or landing this commit is probably in order.

[vsk]

Thanks for your work on this!

This passes check-all on x86, but there are a few changes I'd like to see. (1) Please shorten and clean up the comments you added, (2) clang-format -style=llvm your code. I'm happy to commit this for you once that's done.

[m4b]

updated according to vsk suggestions

• shortened comments
• ran clang-format-3.4 -i -style=llvm as per vsk's suggestions

*NOTE*: I noticed there were pretty massive source code formatting changes after clang-format; I'm not sure if this was/is intended?

[dougk]

To prevent clang-format from adjusting whitespace in the entire file, you want to use clang-format-diff.py -

git diff | tools/clang/tools/clang-format/clang-format-diff.py -p1 -style=llvm

[m4b]

Shorter comments and only formatting code changes

• shorter comments
• reformat with clang-format-diff.py as per dougk

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

static int getID(struct InternalInstruction* insn, const void *miiArg) {
   /*
    * JCXZ/JECXZ need special handling for 16-bit mode because the meaning
    * of the AdSize prefix is inverted w.r.t. 32-bit mode.
    */
   if (insn->mode == MODE_16BIT && insn->opcodeType == ONEBYTE &&
       insn->opcode == 0xE3)
     attrMask ^= ATTR_ADSIZE;
 
+  /*
+   * In 64-bit mode all f64 superscripted opcodes ignore opcode size prefix
+   * CALL/JMP/JCC instructions need to ignore 0x66 and consume 4 bytes
+   */
+
+  if (insn->mode == MODE_64BIT &&
+      isPrefixAtLocation(insn, 0x66, insn->necessaryPrefixLocation)) {
+    switch (insn->opcode) {
+    case 0xE8:
+    case 0xE9:
+      if (insn->opcodeType ==
+          ONEBYTE) { // breaks psubsb and other mmx instructions otherwise
+        attrMask ^= ATTR_OPSIZE;
+        insn->immediateSize = 4;
+        insn->displacementSize = 4;
+      }
+      break;
+    case 0x82:
+    case 0x83:
+    case 0x84:
+    case 0x85:
+    case 0x86:
+    case 0x87:
+    case 0x88:
+    case 0x89:
+    case 0x8A:
+    case 0x8B:
+    case 0x8C:
+    case 0x8D:
+    case 0x8E:
+    case 0x8F:
+      if (insn->opcodeType ==
+          TWOBYTE) { // breaks lea and three byte ops otherwise
+        attrMask ^= ATTR_OPSIZE;
+        insn->immediateSize = 4;
+        insn->displacementSize = 4; // otherwise not sign extended
+      }
+      break;
+    }
+  }
+
   if (getIDWithAttrMask(&instructionID, insn, attrMask))
     return -1;
 
   /* The following clauses compensate for limitations of the tables. */
 
   if (insn->mode != MODE_64BIT &&
       insn->vectorExtensionType != TYPE_NO_VEX_XOP) {
     /*

llvm/trunk/test/MC/Disassembler/X86/x86-64.txt

 0x67, 0x48 0xa1 0x5a 0x5a 0x5a 0x5a
 
 # CHECK: movabsq %rax, 6510615555426900570
 0x48 0xa3 0x5a 0x5a 0x5a 0x5a 0x5a 0x5a 0x5a 0x5a
 
 # CHECK: movq %rax, 1515870810
 0x67, 0x48 0xa3 0x5a 0x5a 0x5a 0x5a
 
+# CHECK: callq -32769
+0x66 0xe8 0xff 0x7f 0xff 0xff
+
+# CHECK: callq -32769
+0x66 0x66 0x48 0xe8 0xff 0x7f 0xff 0xff
+
+# CHECK: jmp -32769
+0xe9 0xff 0x7f 0xff 0xff
+
+# CHECK: jmp -32769
+0x66 0xe9 0xff 0x7f 0xff 0xff
+
+# CHECK: jmp -32769
+0x66 0x66 0x48 0xe9 0xff 0x7f 0xff 0xff
+
+# CHECK: jb -32769
+0x0f 0x82 0xff 0x7f 0xff 0xff
+
+# CHECK: jb -32769
+0x66 0x0f 0x82 0xff 0x7f 0xff 0xff
+
+# CHECK: jae -32769
+0x0f 0x83 0xff 0x7f 0xff 0xff
+
+# CHECK: jae -32769
+0x66 0x0f 0x83 0xff 0x7f 0xff 0xff
+
+# CHECK: je -32769
+0x0f 0x84 0xff 0x7f 0xff 0xff
+
+# CHECK: je -32769
+0x66 0x0f 0x84 0xff 0x7f 0xff 0xff
+
+# CHECK: jne -32769
+0x0f 0x85 0xff 0x7f 0xff 0xff
+
+# CHECK: jne -32769
+0x66 0x0f 0x85 0xff 0x7f 0xff 0xff
+
+# CHECK: jbe -32769
+0x0f 0x86 0xff 0x7f 0xff 0xff
+
+# CHECK: jbe -32769
+0x66 0x0f 0x86 0xff 0x7f 0xff 0xff
+
+# CHECK: ja -32769
+0x0f 0x87 0xff 0x7f 0xff 0xff
+
+# CHECK: ja -32769
+0x66 0x0f 0x87 0xff 0x7f 0xff 0xff
+
+# CHECK: js -32769
+0x0f 0x88 0xff 0x7f 0xff 0xff
+
+# CHECK: js -32769
+0x66 0x0f 0x88 0xff 0x7f 0xff 0xff
+
+# CHECK: jns -32769
+0x0f 0x89 0xff 0x7f 0xff 0xff
+
+# CHECK: jns -32769
+0x66 0x0f 0x89 0xff 0x7f 0xff 0xff
+
+# CHECK: jp -32769
+0x0f 0x8a 0xff 0x7f 0xff 0xff
+
+# CHECK: jp -32769
+0x66 0x0f 0x8a 0xff 0x7f 0xff 0xff
+
+# CHECK: jnp -32769
+0x0f 0x8b 0xff 0x7f 0xff 0xff
+
+# CHECK: jnp -32769
+0x66 0x0f 0x8b 0xff 0x7f 0xff 0xff
+
+# CHECK: jl -32769
+0x0f 0x8c 0xff 0x7f 0xff 0xff
+
+# CHECK: jl -32769
+0x66 0x0f 0x8c 0xff 0x7f 0xff 0xff
+
+# CHECK: jge -32769
+0x0f 0x8d 0xff 0x7f 0xff 0xff
+
+# CHECK: jge -32769
+0x66 0x0f 0x8d 0xff 0x7f 0xff 0xff
+
+# CHECK: jle -32769
+0x0f 0x8e 0xff 0x7f 0xff 0xff
+
+# CHECK: jle -32769
+0x66 0x0f 0x8e 0xff 0x7f 0xff 0xff
+
+# CHECK: jg -32769
+0x0f 0x8f 0xff 0x7f 0xff 0xff
+
+# CHECK: jg -32769
+0x66 0x0f 0x8f 0xff 0x7f 0xff 0xff
+
+# CHECK: lcallw *-32769(%rip)
+0x66 0xff 0x1d 0xff 0x7f 0xff 0xff
+
+# CHECK: ljmpw  *-32769(%rip)
+0x66 0xff 0x2d 0xff 0x7f 0xff 0xff
+
+# CHECK: psubsb (%rdx), %mm3
+0x0f 0xe8 0x1a
+
+# CHECK: psubsb (%rdx), %xmm3
+0x66 0x0f 0xe8 0x1a
+
 # CHECK: addq 255(%rip), %rbx
 0x49, 0x03, 0x1d, 0xff, 0x00, 0x00, 0x00
 
 # The following 4 encodings are equivalent, as confirmed by the 'xed64'
 # decoder tool provided by Intel, which we assume to be canonical even
 # if the real silicon does something different. If that should happen,
 # then we'll all have disassembler bugs to repair.