[X86] Preserve redundant Address-Size override prefix

[Amir]

Summary

Print and emit redundant Address-Size override prefix if it's set on the
instruction.

[skan]

I think this patch is totally wrong. We need address size override prefix only when the instruction has a memory operand.The address-size override prefix (67H) allows programs to switch between 16- and 32-bit addressing. Either size can be the default; the prefix selects the non-default size. Using this prefix and/or other undefined opcodes when operands for the instruction do not reside in memory is reserved; such use may cause unpredictable behavior. Whether a instruction need ASZ should be included in the definition of the instruction in tablgen.

[craig.topper]

In D120592#3348216, @skan wrote:

I think this patch is totally wrong. We need address size override prefix only when the instruction has a memory operand.The address-size override prefix (67H) allows programs to switch between 16- and 32-bit addressing. Either size can be the default; the prefix selects the non-default size. Using this prefix and/or other undefined opcodes when operands for the instruction do not reside in memory is reserved; such use may cause unpredictable behavior. Whether a instruction need ASZ should be included in the definition of the instruction in tablgen.

I believe example test case is supported by gas and callq does use address size. What syntax should be used instead?

[skan]

In D120592#3348246, @craig.topper wrote:

In D120592#3348216, @skan wrote:

I think this patch is totally wrong. We need address size override prefix only when the instruction has a memory operand.The address-size override prefix (67H) allows programs to switch between 16- and 32-bit addressing. Either size can be the default; the prefix selects the non-default size. Using this prefix and/or other undefined opcodes when operands for the instruction do not reside in memory is reserved; such use may cause unpredictable behavior. Whether a instruction need ASZ should be included in the definition of the instruction in tablgen.

I believe example test case is supported by gas and callq does use address size. What syntax should be used instead?

This test case is misleading. LLVM-MC can pass the test case w/o this patch b/c an expicit prefix is added. However, this callq does not need a ASZ at all.

[craig.topper]

In D120592#3348249, @skan wrote:

In D120592#3348246, @craig.topper wrote:

In D120592#3348216, @skan wrote:

I think this patch is totally wrong. We need address size override prefix only when the instruction has a memory operand.The address-size override prefix (67H) allows programs to switch between 16- and 32-bit addressing. Either size can be the default; the prefix selects the non-default size. Using this prefix and/or other undefined opcodes when operands for the instruction do not reside in memory is reserved; such use may cause unpredictable behavior. Whether a instruction need ASZ should be included in the definition of the instruction in tablgen.

I believe example test case is supported by gas and callq does use address size. What syntax should be used instead?

This test case is misleading. LLVM-MC can pass the test case w/o this patch b/c an expicit prefix is added. However, this callq does not need a ASZ at all.

There is a legimate use for a redundant addr32 on a call instruction. See this code in lld

// Convert call/jmp instructions.
if (modRm == 0x15) {
  // ABI says we can convert "call *foo@GOTPCREL(%rip)" to "nop; call foo".
  // Instead we convert to "addr32 call foo" where addr32 is an instruction
  // prefix. That makes result expression to be a single instruction.
  loc[-2] = 0x67; // addr32 prefix
  loc[-1] = 0xe8; // call
  write32le(loc, val);
  return;
}

[skan]

In D120592#3348274, @craig.topper wrote:

In D120592#3348249, @skan wrote:

In D120592#3348246, @craig.topper wrote:

In D120592#3348216, @skan wrote:

I think this patch is totally wrong. We need address size override prefix only when the instruction has a memory operand.The address-size override prefix (67H) allows programs to switch between 16- and 32-bit addressing. Either size can be the default; the prefix selects the non-default size. Using this prefix and/or other undefined opcodes when operands for the instruction do not reside in memory is reserved; such use may cause unpredictable behavior. Whether a instruction need ASZ should be included in the definition of the instruction in tablgen.

I believe example test case is supported by gas and callq does use address size. What syntax should be used instead?

This test case is misleading. LLVM-MC can pass the test case w/o this patch b/c an expicit prefix is added. However, this callq does not need a ASZ at all.

There is a legimate use for a redundant addr32 on a call instruction. See this code in lld

// Convert call/jmp instructions.
if (modRm == 0x15) {
  // ABI says we can convert "call *foo@GOTPCREL(%rip)" to "nop; call foo".
  // Instead we convert to "addr32 call foo" where addr32 is an instruction
  // prefix. That makes result expression to be a single instruction.
  loc[-2] = 0x67; // addr32 prefix
  loc[-1] = 0xe8; // call
  write32le(loc, val);
  return;
}

Thanks craig, I agree that we can add a redundant addr32 prefix on callq.

In addition, and GNU objdump also prints the prefix even if it is illegal, which can help developer find their bug.

cat 1.s

addr32 ret

as 1.s -o 1.o && objdump -d 1.o

0000000000000000 <.text>:
   0:   67 c3                   addr32 retq

This change looks good to me. But the test case should be moved into MC and covers ATT/Intel assemble syntax.

[skan]

bolt/test/X86/addr32.s

3–21

[skan]

bolt/test/X86/addr32.s

1

This test should be moved to llvm/test/MC/Disassembler/X86/

[Amir]

@skan, @craig.topper
Thank you for the review! I didn't communicate the intent clearly. There's a BOLT bug report (https://github.com/llvm/llvm-project/issues/54066#issuecomment-1050327548) in which BOLT removes addr32 prefix from call imm. I've root caused it to instruction printer and emitter ignoring the explicit (redundant) prefix. The code is produced by g++ (GCC) 9.2.1 20200225, with -O3 -g -std=gnu++17 -Wl,-Bsymbolic-functions -Wl,--dynamic-list-cpp-new. The linker wasn't mentioned.

[skan]

In D120592#3349375, @Amir wrote:

@skan, @craig.topper
Thank you for the review! I didn't communicate the intent clearly. There's a BOLT bug report (https://github.com/llvm/llvm-project/issues/54066#issuecomment-1050327548) in which BOLT removes addr32 prefix from call imm. I've root caused it to instruction printer and emitter ignoring the explicit (redundant) prefix. The code is produced by g++ (GCC) 9.2.1 20200225, with -O3 -g -std=gnu++17 -Wl,-Bsymbolic-functions -Wl,--dynamic-list-cpp-new. The linker wasn't mentioned.

I didn't apply the patch and rebuild the compiler. But I think the bug can be reproduced w/o BOLT, right?

[Amir]

Fix tests

[Amir]

In D120592#3350106, @skan wrote:

In D120592#3349375, @Amir wrote:

@skan, @craig.topper
Thank you for the review! I didn't communicate the intent clearly. There's a BOLT bug report (https://github.com/llvm/llvm-project/issues/54066#issuecomment-1050327548) in which BOLT removes addr32 prefix from call imm. I've root caused it to instruction printer and emitter ignoring the explicit (redundant) prefix. The code is produced by g++ (GCC) 9.2.1 20200225, with -O3 -g -std=gnu++17 -Wl,-Bsymbolic-functions -Wl,--dynamic-list-cpp-new. The linker wasn't mentioned.

I didn't apply the patch and rebuild the compiler. But I think the bug can be reproduced w/o BOLT, right?

Yes, it reproduces with llvm-mc disasm-asm roundtrip (the prefix is lost).

[skan]

llvm/test/MC/X86/align-branch-hardcode.s

20 ↗

(On Diff #411935)

Why do we got addr32 here w/ hardcode 0x66?

[Amir]

llvm/test/MC/X86/align-branch-hardcode.s

20 ↗

(On Diff #411935)

Looking into it

[Amir]

Rebase

[Amir]

llvm/test/MC/X86/align-branch-hardcode.s

20 ↗

(On Diff #411935)

@skan
The call has 0x67 prefix regardless of .byte 0x66, perhaps due to using ecx register as a memop.
With this diff we start printing the addr32 prefix that wasn't printed before.

[skan]

llvm/test/MC/X86/code16-32-64.s

18–20 ↗

(On Diff #411938)

addr32 should not be printed here b/c 0x67 is not a redundant prefix here. You can check the behaviour of GNU objdump.

[Amir]

Only print address size override prefix in disassembly when it's redundant

[Amir]

@skan
I moved NeedAddressOverride check out of X86MCCodeEmitter.cpp (along with isXXBitMemOperand) into X86MCTargetDesc in order to use it in printInstFlags to test if addr32 is a redundant prefix.

[skan]

In D120592#3353538, @Amir wrote:

@skan
I moved NeedAddressOverride check out of X86MCCodeEmitter.cpp (along with isXXBitMemOperand) into X86MCTargetDesc in order to use it in printInstFlags to test if addr32 is a redundant prefix.

I belive it's the correct direction. However there is still a bug. (see my inline comments)

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

54 ↗

(On Diff #412323)

0x67 is not redundant here. It changes (%rdi) to (%edi). You can not encode
rep addr32 stosq %rax, %es:(%edi) w/ llvm-mc --show-encoding

llvm/test/MC/X86/addr16-32.s

20–23 ↗

(On Diff #412323)

0x67 is not redundant here.

[Amir]

Moved RawFrmSrc, RawFrmDstSrc, RawFrmDst handling from X86MCCodeEmitter::emitPrefixImpl
to X86_MC::needsAddressSizeOverride. This consolidates all checks controlling
the emission of AddressSize override prefix in needsAddressSizeOverride.

The flipside is a prefix order change but it should be benign.

[Amir]

Code cleanup

[craig.topper]

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp

75 ↗

(On Diff #412565)

Missing namespace? Or missing static?

llvm/test/MC/X86/index-operations.s

74 ↗

(On Diff #412565)

Was this a bug before? REX prefix is supposed to be the byte before the opcode.

[Amir]

llvm/test/MC/X86/index-operations.s

74 ↗

(On Diff #412565)

You're right. It was a bug before, but it's still there:
REX prefix emission happened before emitSegmentOverridePrefix and 0x67 prefix, this diff moved 0x67 prefix emission above.
See the line
HasREX = emitOpcodePrefix(MemoryOperand, MI, STI, OS);

[Amir]

static isMemOperand

[skan]

LGTM.

[Amir]

llvm/test/MC/X86/index-operations.s

74 ↗

(On Diff #412565)

Addressed this issue in D120871 (follow-up to this diff)

bolt/test/X86/addr32.s

+# Check that we don't accidentally strip addr32 prefix
+
+# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s -o %t.o
+# RUN: ld.lld %t.o -o %t.exe -nostdlib
+# RUN: llvm-objdump -d %t.exe | FileCheck %s
+# RUN: llvm-bolt %t.exe -o %t.out -lite=0
+# RUN: llvm-objdump -d %t.out | FileCheck %s
+# CHECK: 67 e8 {{.*}} callq {{.*}} <foo>
+
+.globl  _start
+.type _start, @function
+_start:
+.code64
+  addr32 callq foo
+  ret
+  .size _start, .-_start
+
+.globl  foo
+.type foo, @function
+foo:
+  ud2

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp

 
 void X86ATTInstPrinter::printInst(const MCInst *MI, uint64_t Address,
                                   StringRef Annot, const MCSubtargetInfo &STI,
                                   raw_ostream &OS) {
   // If verbose assembly is enabled, we can print some informative comments.
   if (CommentStream)
     HasCustomInstComment = EmitAnyX86InstComments(MI, *CommentStream, MII);
 
-  printInstFlags(MI, OS);
+  printInstFlags(MI, OS, STI);
 
   // Output CALLpcrel32 as "callq" in 64-bit mode.
   // In Intel annotation it's always emitted as "call".
   //
   // TODO: Probably this hack should be redesigned via InstAlias in
   // InstrInfo.td as soon as Requires clause is supported properly
   // for InstAlias.
   if (MI->getOpcode() == X86::CALLpcrel32 &&

llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.h

public:
   void printVPCOMMnemonic(const MCInst *MI, raw_ostream &OS);
   void printVPCMPMnemonic(const MCInst *MI, raw_ostream &OS);
   void printCMPMnemonic(const MCInst *MI, bool IsVCmp, raw_ostream &OS);
   void printRoundingControl(const MCInst *MI, unsigned Op, raw_ostream &O);
   void printPCRelImm(const MCInst *MI, uint64_t Address, unsigned OpNo,
                      raw_ostream &O);
 
 protected:
-  void printInstFlags(const MCInst *MI, raw_ostream &O);
+  void printInstFlags(const MCInst *MI, raw_ostream &O,
+                      const MCSubtargetInfo &STI);
   void printOptionalSegReg(const MCInst *MI, unsigned OpNo, raw_ostream &O);
   void printVKPair(const MCInst *MI, unsigned OpNo, raw_ostream &OS);
 };
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_X86_MCTARGETDESC_X86INSTPRINTERCOMMON_H

llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp

 
 #include "X86InstPrinterCommon.h"
 #include "X86BaseInfo.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstrDesc.h"
 #include "llvm/MC/MCInstrInfo.h"
-#include "llvm/Support/raw_ostream.h"
+#include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/Support/Casting.h"
-#include <cstdint>
+#include "llvm/Support/raw_ostream.h"
 #include <cassert>
+#include <cstdint>
 
 using namespace llvm;
 
 void X86InstPrinterCommon::printCondCode(const MCInst *MI, unsigned Op,
                                          raw_ostream &O) {
   int64_t Imm = MI->getOperand(Op).getImm();
   switch (Imm) {
   default: llvm_unreachable("Invalid condcode argument!");

 void X86InstPrinterCommon::printOptionalSegReg(const MCInst *MI, unsigned OpNo,
                                                raw_ostream &O) {
   if (MI->getOperand(OpNo).getReg()) {
     printOperand(MI, OpNo, O);
     O << ':';
   }
 }
 
-void X86InstPrinterCommon::printInstFlags(const MCInst *MI, raw_ostream &O) {
+void X86InstPrinterCommon::printInstFlags(const MCInst *MI, raw_ostream &O,
+                                          const MCSubtargetInfo &STI) {
   const MCInstrDesc &Desc = MII.get(MI->getOpcode());
   uint64_t TSFlags = Desc.TSFlags;
   unsigned Flags = MI->getFlags();
 
   if ((TSFlags & X86II::LOCK) || (Flags & X86::IP_HAS_LOCK))
     O << "\tlock\t";
 
   if ((TSFlags & X86II::NOTRACK) || (Flags & X86::IP_HAS_NOTRACK))
else if (Flags & X86::IP_USE_VEX3)
     O << "\t{vex3}";
   else if (Flags & X86::IP_USE_EVEX)
     O << "\t{evex}";
 
   if (Flags & X86::IP_USE_DISP8)
     O << "\t{disp8}";
   else if (Flags & X86::IP_USE_DISP32)
     O << "\t{disp32}";
+
+  // Address-Size override prefix
+  if (Flags & X86::IP_HAS_AD_SIZE) {
+    if (STI.hasFeature(X86::Mode16Bit) || STI.hasFeature(X86::Mode64Bit))
+      O << "\taddr32\t";
+    else if (STI.hasFeature(X86::Mode32Bit))
+      O << "\taddr16\t";
+  }
 }
 
 void X86InstPrinterCommon::printVKPair(const MCInst *MI, unsigned OpNo,
                                        raw_ostream &OS) {
   // In assembly listings, a pair is represented by one of its members, any
   // of the two.  Here, we pick k0, k2, k4, k6, but we could as well
   // print K2_K3 as "k3".  It would probably make a lot more sense, if
   // the assembly would look something like:

llvm/lib/Target/X86/MCTargetDesc/X86IntelInstPrinter.cpp

 
 void X86IntelInstPrinter::printRegName(raw_ostream &OS, unsigned RegNo) const {
   OS << getRegisterName(RegNo);
 }
 
 void X86IntelInstPrinter::printInst(const MCInst *MI, uint64_t Address,
                                     StringRef Annot, const MCSubtargetInfo &STI,
                                     raw_ostream &OS) {
-  printInstFlags(MI, OS);
+  printInstFlags(MI, OS, STI);
 
   // In 16-bit mode, print data16 as data32.
   if (MI->getOpcode() == X86::DATA16_PREFIX &&
       STI.getFeatureBits()[X86::Mode16Bit]) {
     OS << "\tdata32";
   } else if (!printAliasInstr(MI, Address, OS) && !printVecCompareInstr(MI, OS))
     printInstruction(MI, Address, OS);
 

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

bool X86MCCodeEmitter::emitPrefixImpl(unsigned &CurOp, const MCInst &MI,
 
   // Emit the address size opcode prefix as needed.
   bool NeedAddressOverride;
   uint64_t AdSize = TSFlags & X86II::AdSizeMask;
   if ((STI.hasFeature(X86::Mode16Bit) && AdSize == X86II::AdSize32) ||
       (STI.hasFeature(X86::Mode32Bit) && AdSize == X86II::AdSize16) ||
       (STI.hasFeature(X86::Mode64Bit) && AdSize == X86II::AdSize32)) {
     NeedAddressOverride = true;
+  } else if (Flags & X86::IP_HAS_AD_SIZE) {
+    NeedAddressOverride = true;
   } else if (MemoryOperand < 0) {
     NeedAddressOverride = false;
   } else if (STI.hasFeature(X86::Mode64Bit)) {
     assert(!is16BitMemOperand(MI, MemoryOperand, STI));
     NeedAddressOverride = is32BitMemOperand(MI, MemoryOperand);
   } else if (STI.hasFeature(X86::Mode32Bit)) {
     assert(!is64BitMemOperand(MI, MemoryOperand));
     NeedAddressOverride = is16BitMemOperand(MI, MemoryOperand, STI);