[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

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

1013

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

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

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;
 
+  /*
+   * CALL/JMP 64-bit mode fix to ignore opcode size prefix when disassembling
+   * and consume all 4 bytes of the immediate/displacement instead;
+   * NOTE: intel spec states CALL rel16/JMP rel16  is Not Supported in 64-bit mode
+   */
+
+  if (insn->mode == MODE_64BIT && insn->opcodeType == ONEBYTE && isPrefixAtLocation(insn, 0x66, insn->necessaryPrefixLocation)){
+    switch (insn->opcode){
+    case 0xE8:
+      attrMask ^= ATTR_OPSIZE;
+      break;
+    case 0xE9:
+      attrMask ^= ATTR_OPSIZE;
+      insn->immediateSize = 4;
+      insn->displacementSize = 4;
+      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) {
     /*
      * The tables can't distinquish between cases where the W-bit is used to
      * select register size and cases where its a required part of the opcode.
      */
     if ((insn->vectorExtensionType == TYPE_EVEX &&
          wFromEVEX3of4(insn->vectorExtensionPrefix[2])) ||
         (insn->vectorExtensionType == TYPE_VEX_3B &&
          wFromVEX3of3(insn->vectorExtensionPrefix[2])) ||
         (insn->vectorExtensionType == TYPE_XOP &&
          wFromXOP3of3(insn->vectorExtensionPrefix[2]))) {
 
       uint16_t instructionIDWithREXW;
       if (getIDWithAttrMask(&instructionIDWithREXW,

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

 # CHECK: movq 1515870810, %rax
 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