This is an archive of the discontinued LLVM Phabricator instance.

Differential D38626

Invalid used of 'w' suffix on push and pop using 64-bit register
ClosedPublic

Authored by avt77 on Oct 6 2017, 7:27 AM.

Details

Reviewers
RKSimon
echristo
davide
coby
dtemirbulatov
• rafael
Commits
rL316898: Invalid used of 'w' suffix on push and pop using 64-bit register.
Summary

This patch fixes PR32806:

The following instruction accesses a 64-bit register, but the mnemonic indicates that it is using a 16-bit register:

  Bytes:            67 49 5e
  Output:         popw %r14
  Expected:      popq %r14

Diff Detail

Repository
rL LLVM

Event Timeline

avt77 created this revision.Oct 6 2017, 7:27 AM
RKSimon added a subscriber: llvm-commits.Oct 6 2017, 7:53 AM
craig.topper added a subscriber: craig.topper.Oct 13 2017, 9:26 AM
craig.topper added inline comments.
lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp
968 ↗(On Diff #117995)

Why are you forcing the XS attribute?

avt77 added inline comments.Oct 17 2017, 5:20 AM
lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp
968 ↗(On Diff #117995)

I thought we should transform ADSIZE in XS here. It seems I was wrong: everything works w/o such a transformation. I'll update the patch asap.

avt77 updated this revision to Diff 119305.Oct 17 2017, 6:14 AM

I removed unnecessary attribute usage.

avt77 mentioned this in D39093: Certain VEX instructions ignore the W-bit that shouldn'.Oct 23 2017, 12:48 AM
RKSimon added inline comments.Oct 24 2017, 8:01 AM
lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp
968 ↗(On Diff #117995)

Can't this be done as just:

attrMask |= ATTR_REXW;
attrMask &= ~ATTR_ADSIZE;
avt77 updated this revision to Diff 120213.Oct 25 2017, 2:13 AM

The fix was refactored accordingly to Simon's suggest.
Could somebody give me LGTM?

RKSimon added inline comments.Oct 25 2017, 5:27 AM
test/MC/Disassembler/X86/x86-64.txt
494 ↗(On Diff #120213)

Can you commit this test now showing the incorrect popw/pushw and then rebase the patch?

avt77 updated this revision to Diff 120567.Oct 27 2017, 3:30 AM

The patch was re-based to show the change in the test.

RKSimon accepted this revision.Oct 27 2017, 4:00 AM

LGTM - thanks

This revision is now accepted and ready to land.Oct 27 2017, 4:00 AM
Closed by commit rL316898: Invalid used of 'w' suffix on push and pop using 64-bit register. (authored by avt77). · Explain WhyOct 30 2017, 5:02 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
trunk/
lib/
Target/
X86/
Disassembler/
4 lines
test/
MC/
Disassembler/
X86/
6 lines

Diff 120801

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

Show First 20 Lines • Show All 943 Lines • ▼ Show 20 Lines case 0x66:
attrMask |= ATTR_OPSIZE; attrMask |= ATTR_OPSIZE;
break; break;
case 0x67: case 0x67:
attrMask |= ATTR_ADSIZE; attrMask |= ATTR_ADSIZE;
break; break;
} }
} }
if (insn->rexPrefix & 0x08) if (insn->rexPrefix & 0x08) {
attrMask |= ATTR_REXW; attrMask |= ATTR_REXW;
attrMask &= ~ATTR_ADSIZE;
}
/* /*
* JCXZ/JECXZ need special handling for 16-bit mode because the meaning * JCXZ/JECXZ need special handling for 16-bit mode because the meaning
* of the AdSize prefix is inverted w.r.t. 32-bit mode. * of the AdSize prefix is inverted w.r.t. 32-bit mode.
*/ */
if (insn->mode == MODE_16BIT && insn->opcodeType == ONEBYTE && if (insn->mode == MODE_16BIT && insn->opcodeType == ONEBYTE &&
insn->opcode == 0xE3) insn->opcode == 0xE3)
attrMask ^= ATTR_ADSIZE; attrMask ^= ATTR_ADSIZE;
▲ Show 20 Lines • Show All 949 Lines • Show Last 20 Lines

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

Show First 20 Lines • Show All 473 Lines • ▼ Show 20 Lines
0x67 0x8f 0xea 0x68 0x12 0x0c 0x24 0xef 0xcd 0xab 0x89 0x67 0x8f 0xea 0x68 0x12 0x0c 0x24 0xef 0xcd 0xab 0x89
# CHECK: nopq -559038737(%rbx,%rcx,8) # CHECK: nopq -559038737(%rbx,%rcx,8)
0x48 0x0f 0x1f 0x84 0xcb 0xef 0xbe 0xad 0xde 0x48 0x0f 0x1f 0x84 0xcb 0xef 0xbe 0xad 0xde
# CHECK: nopq %rax # CHECK: nopq %rax
0x48 0x0f 0x1f 0xC0 0x48 0x0f 0x1f 0xC0
# TODO: wrong dissassembler with 0x67 prefix: expected popq # CHECK: popq %r14
# CHECK: popw %r14
0x67 0x49 0x5e 0x67 0x49 0x5e
# TODO: wrong dissassembler with 0x67 prefix: expected pushq # CHECK: pushq %r14
# CHECK: pushw %r14
0x67 0x49 0x56 0x67 0x49 0x56
# CHECK: xchgw %di, %ax # CHECK: xchgw %di, %ax
0x66 0x3e 0x97 0x66 0x3e 0x97
# CHECK: movq %rdx, %cs # CHECK: movq %rdx, %cs
0x4c 0x8e 0xca 0x4c 0x8e 0xca
Show All 22 Lines