Page MenuHomePhabricator

[X86] Have indirect calls take 64-bit operands in 64-bit modes
Needs ReviewPublic

Authored by hvdijk on Sat, Nov 21, 1:58 PM.

Details

Summary

The build bots caught two additional pre-existing problems exposed by the test change part of my change https://reviews.llvm.org/D91339, when expensive checks are enabled. This fixes one of them.

X86 has CALL64r and CALL32r opcodes, where CALL64r takes a 64-bit register, and CALL32r takes a 32-bit register. CALL64r can only be used in 64-bit mode, CALL32r can only be used in 32-bit mode. LLVM would assume that after picking the appropriate CALLr opcode, a pointer-sized register would be a valid operand, but in x32 mode, a 64-bit mode, pointers are 32 bits. In this mode, it is invalid to directly pass a pointer to CALL64r, it needs to be extended to 64 bits first.

Diff Detail

Unit TestsFailed

TimeTest
410 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

hvdijk created this revision.Sat, Nov 21, 1:58 PM
hvdijk requested review of this revision.Sat, Nov 21, 1:58 PM
hvdijk updated this revision to Diff 306876.Sat, Nov 21, 2:02 PM

Forgot to run clang-format-diff before uploading, updated now.

craig.topper added inline comments.Sat, Nov 21, 2:32 PM
llvm/test/CodeGen/X86/pic.ll
103

Why a regular expression? Shouldn't it be fixed for x32?

hvdijk added inline comments.Sat, Nov 21, 2:55 PM
llvm/test/CodeGen/X86/pic.ll
103

It's a good question. I wanted to say that either would be okay since the high bits of all addresses are known to be zero, but actually that is not at all the case at all, it must be callq, and this patch is wrong. The instruction would be okay if it were valid, but it's not: the result of CALL32r passes LLVM's own consistency tests, but is subsequently rejected by the assembler, something not covered by the tests. I will redo this patch to use the other approach, the zero extending. Apologies for the noise and for the breakage.

hvdijk updated this revision to Diff 306887.Sat, Nov 21, 5:02 PM
hvdijk retitled this revision from [X86] Have indirect calls take pointer-sized operands to [X86] Have indirect calls take 64-bit operands in 64-bit modes.
hvdijk edited the summary of this revision. (Show Details)

Is there a test case?

llvm/lib/Target/X86/X86FastISel.cpp
1092

SUBREG_TO_REG is sort of an assertion that the upper 32 bits of the register are 0. This would require the producing instruction to definitely be a 32-bit operation and not some 64-bit operation that was just truncated to 32-bits. Is that really guaranteed here?

hvdijk updated this revision to Diff 306896.Sun, Nov 22, 1:58 AM

Add MOV32rr to guarantee high bits will be zero.

llvm/lib/Target/X86/X86FastISel.cpp
1092

It took a bit of effort to find a test case to verify, but you're right, it's not. Even if getRegForValue will create a new instruction, and all hardware instructions that set a 32-bit register will implicitly zero the high 32 bits, getRegForValue need not create a hardware instruction, it may create a pseudo-instructions such as a subreg copy.