[PATCH] D120592: [X86] Fix handling of Address-Size override prefix

[Kan Shengchen via Phabricator via llvm-commits]

skan added a comment.

In D120592#3348274 <https://reviews.llvm.org/D120592#3348274>, @craig.topper wrote:

> In D120592#3348249 <https://reviews.llvm.org/D120592#3348249>, @skan wrote:
>
>> In D120592#3348246 <https://reviews.llvm.org/D120592#3348246>, @craig.topper wrote:
>>
>>> In D120592#3348216 <https://reviews.llvm.org/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 at 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120592/new/

https://reviews.llvm.org/D120592