[X86] Fix PR23271 - RIP-relative decoding bug in disassembler.

[dougk]

Summary

REX.b and EVEX.x are not decoded when determining whether the addressing mode is RIP-relative.

[dougk]

Just mask the unwanted bits.

[chandlerc]

Looks good with the test case changed as below.

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

327 ↗

(On Diff #25695)

XFAIL doesn't work this way...

You only write one XFAIL per test file, and it fails the entire file.

I think you want to write the CHECKs as they are today, and as FIXME:s for what they should be.

[m4b]

Besides inline comments, this looks good to me, as typical uses of RIP relative addressing seem to disassemble correctly now.

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

314 ↗

(On Diff #25695)

I'm getting a #UD on this instruction.

Well, technically on: 62 11 5c 40 58 3d ff 7f ff ff, or vaddps -32769(%rip), %zmm20, %zmm15

I've attached both LLDB and GDB's output (LLDB doesn't use this patch yet, so it doesn't disassemble past the instruction).

Also GDB disassembles it differently from this patch, as it says the RIP relative immediate is -8001, but by my count it should be -32769 (which llvm-mc disassembles it to using this patch).

Note: I monkey-patched a C binary with the instruction, so it's not a proper compiled object, but it should at least execute/segfault; but I get an illegal instruction error on that exact instruction, so I think it's not the patching that's the problem.

Unfortunately I can't seem to find the source or a binary download for the xed program on Intel's website or anywhere, otherwise I'd try to get a "second opinion". Hopefully something else is going on, otherwise we might actually all have disassembler bugs to repair :)

vaddps.txt4 KBDownload

[m4b]

Ignore previous comment; I see now that the instruction #UD's because they haven't made/released processors which implement EVEX prefixes yet, so all's right with the world ;)

Carry on!

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

case 2:
     break;
   case 4:
   case 8:
     insn->eaBaseBase = (insn->addressSize == 4 ? EA_BASE_EAX : EA_BASE_RAX);
 
     switch (mod) {
     case 0x0:
       insn->eaDisplacement = EA_DISP_NONE; /* readSIB may override this */
-      switch (rm) {
-      case 0x14:
-      case 0x4:
-      case 0xc:   /* in case REXW.b is set */
+      // In determining whether RIP-relative mode is used (rm=5),
+      // or whether a SIB byte is present (rm=4),
+      // the extension bits (REX.b and EVEX.x) are ignored.
+      switch (rm & 7) {
+      case 0x4: // SIB byte is present
         insn->eaBase = (insn->addressSize == 4 ?
                         EA_BASE_sib : EA_BASE_sib64);
         if (readSIB(insn) || readDisplacement(insn))
           return -1;
         break;
-      case 0x5:
+      case 0x5: // RIP-relative
         insn->eaBase = EA_BASE_NONE;
         insn->eaDisplacement = EA_DISP_32;
         if (readDisplacement(insn))
           return -1;
         break;
       default:
         insn->eaBase = (EABase)(insn->eaBaseBase + rm);
         break;
       }
       break;
     case 0x1:
       insn->displacementSize = 1;
       /* FALLTHROUGH */
     case 0x2:
       insn->eaDisplacement = (mod == 0x1 ? EA_DISP_8 : EA_DISP_32);
-      switch (rm) {
-      case 0x14:
-      case 0x4:
-      case 0xc:   /* in case REXW.b is set */
+      switch (rm & 7) {
+      case 0x4: // SIB byte is present
         insn->eaBase = EA_BASE_sib;
         if (readSIB(insn) || readDisplacement(insn))
           return -1;
         break;
       default:
         insn->eaBase = (EABase)(insn->eaBaseBase + rm);
         if (readDisplacement(insn))
           return -1;

llvm/trunk/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: 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.
+
+# Try all combinations of EVEX.x and REX.b:
+# CHECK: vaddps 287453952(%rip), %zmm20, %zmm15
+0x62 0x11 0x5c 0x40 0x58 0x3d 0x00 0x33 0x22 0x11
+# CHECK: vaddps 287453952(%rip), %zmm20, %zmm15
+0x62 0x31 0x5c 0x40 0x58 0x3d 0x00 0x33 0x22 0x11
+# CHECK: vaddps 287453952(%rip), %zmm20, %zmm15
+0x62 0x51 0x5c 0x40 0x58 0x3d 0x00 0x33 0x22 0x11
+# CHECK: vaddps 287453952(%rip), %zmm20, %zmm15
+0x62 0x71 0x5c 0x40 0x58 0x3d 0x00 0x33 0x22 0x11
+
+# Known bugs: these use a SIB byte. The index register is incorrectly
+# printed as an xmm register. Indeed there are "gather" load instructions
+# taking a vector of indices, but ONLY those instructions can do that.
+# The CHECK lines test the current incorrect output; FIXME is desired.
+# CHECK: vaddps (%r10,%xmm9), %zmm20, %zmm15
+# FIXME: vaddps (%r10,%r9), %zmm20, %zmm15
+0x62 0x11 0x5c 0x40 0x58 0x3c 0x0a
+
+# CHECK: vaddps (%rdx,%xmm9), %zmm20, %zmm15
+# FIXME: vaddps (%rdx,%r9), %zmm20, %zmm15
+0x62 0x31 0x5c 0x40 0x58 0x3c 0x0a
+
+# CHECK: vaddps (%r10,%xmm1), %zmm20, %zmm15
+# FIXME: vaddps (%r10,%rcx), %zmm20, %zmm15
+0x62 0x51 0x5c 0x40 0x58 0x3c 0x0a
+
+# CHECK: vaddps (%rdx,%xmm1), %zmm20, %zmm15
+# FIXME: vaddps (%rdx,%rcx), %zmm20, %zmm15
+0x62 0x71 0x5c 0x40 0x58 0x3c 0x0a