Page MenuHomePhabricator

Revert "[X86] Produce R_X86_64_GOTPCRELX for test/binop instructions (MOV32rm/TEST32rm/...) when -Wa,-mrelax-relocations=yes is enabled"
AbandonedPublic

Authored by MaskRay on Nov 23 2020, 7:06 AM.

Details

Summary

After just having wasted a day scratching my head why a UBSan build on another machine now started to fail, after I had already forgotten about this issue again, here's a revert now:

This reverts commit f04d92af94a8d763e91ae38fe35319e426dc466c, which causes
miscompilation, as first reported at
<http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20201026/845488.html>
"Re: [llvm] f04d92a - [X86] Produce R_X86_64_GOTPCRELX for test/binop
instructions (MOV32rm/TEST32rm/...) when -Wa, -mrelax-relocations=yes is
enabled":

At least on Linux x86-64 in combination with lld, this causes miscompilation
when an expression like

  ((long) otherfunction) >> 32

computing the upper 32 bits of a function's address is emitted as

  movl otherfunction at GOTPCREL+4(%rip), %eax

which lld then optimizes as

  lea otherfunction+4(%rip), %eax

computing the lower 32 bits of the function's address + 4.

The simplest reproducer I came up with is

> $ cat test.c
> void otherfunction(void);
> int __attribute__ ((visibility("default"))) test1(void) { return (long) otherfunction; }
> int __attribute__ ((visibility("default"))) test2(void) { return ((long) otherfunction) >> 32; }
>
> $ cat otherfunction.c
> void otherfunction(void) {}
>
> $ cat main.c
> #include <stdio.h>
> int test1(void);
> int test2(void);
> int main() {
>   printf("%08X %08X\n", test1(), test2());
>   return 0;
> }
>
> $ llvm/inst/bin/clang -fpic -fvisibility=hidden -O -c test.c
> $ objdump -dr test.o | grep -F -A3 '<test'
> 0000000000000000 <test1>:
>    0:   8b 05 00 00 00 00       mov    0x0(%rip),%eax        # 6 <test1+0x6>
>                         2: R_X86_64_GOTPCRELX   otherfunction-0x4
>    6:   c3                      retq
>    7:   66 0f 1f 84 00 00 00    nopw   0x0(%rax,%rax,1)
> --
> 0000000000000010 <test2>:
>   10:   8b 05 00 00 00 00       mov    0x0(%rip),%eax        # 16 <test2+0x6>
>                         12: R_X86_64_GOTPCRELX  otherfunction
>   16:   c3                      retq
>
> $ llvm/inst/bin/clang -fpic -fvisibility=hidden -c otherfunction.c
>
> $ llvm/inst/bin/clang -fuse-ld=lld --ld-path=/.../llvm/inst/bin/ld.lld -shared test.o otherfunction.o -o libtest.so
> $ objdump -dR libtest.so | grep -F -A3 '<test'
> 00000000000015d0 <test1>:
>     15d0:       8d 05 1a 00 00 00       lea    0x1a(%rip),%eax        # 15f0 <otherfunction>
>     15d6:       c3                      retq
>     15d7:       66 0f 1f 84 00 00 00    nopw   0x0(%rax,%rax,1)
> --
> 00000000000015e0 <test2>:
>     15e0:       8d 05 0e 00 00 00       lea    0xe(%rip),%eax        # 15f4 <otherfunction+0x4>
>     15e6:       c3                      retq
>     15e7:       cc                      int3
>
> $ llvm/inst/bin/clang main.c libtest.so
> $ LD_LIBRARY_PATH=. ./a.out
> 602065F0 602065F4

where using R_X86_64_GOTPCRELX instead of R_X86_64_GOTPCREL in test2 causes the
second value printed out to be (first value + 4) instead of some 00007F55 or
similar.

Diff Detail

Unit TestsFailed

TimeTest
10 mslinux > Extra Tools Unit Tests.clang-query/_/ClangQueryTests::QueryParserTest.Complete
Note: Google Test filter = QueryParserTest.Complete [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
380 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
20 mswindows > Extra Tools Unit Tests.clang-query/_/ClangQueryTests_exe::QueryParserTest.Complete
Note: Google Test filter = QueryParserTest.Complete [==========] Running 1 test from 1 test case.

Event Timeline

sberg created this revision.Nov 23 2020, 7:06 AM
Herald added a project: Restricted Project. · View Herald Transcript
sberg requested review of this revision.Nov 23 2020, 7:07 AM
MaskRay added a comment.EditedNov 23 2020, 9:57 AM

Thanks for reporting the issue. The problem is that clang produces incorrect GOTPCREL+offset fixup. I will try providing a better fix.

// gcc -fpic -fvisibility=hidden -O -S test.c -o - -fno-asynchronous-unwind-tables
test2:
        movq    otherfunction@GOTPCREL(%rip), %rax
        sarq    $32, %rax
        ret

// gcc -fpic -fvisibility=hidden -O -S test.c -o - -fno-asynchronous-unwind-tables
test2:                                  # @test2
# %bb.0:                                # %entry
        movl    otherfunction@GOTPCREL+4(%rip), %eax
        retq

After just having wasted a day scratching my head why a UBSan build on another machine now started to fail

Can you please provide the ubsan example as well?

MaskRay added a comment.EditedNov 23 2020, 11:36 AM
  • X86MCInstLower.cpp emits movl otherfunction@GOTPCREL+4(%rip), %eax
  • This fixup does not work if both R_X86_64_GOTPCRELX (new integrated assembler behavior, also GNU as's) and LLD are used.

(i.e. before my patch, -fno-integrated-as + LLD can break as well)

otherfunction@GOTPCREL+4 is a bit weird as it emits a GOT entry for otherfunction but accesses the high 4 bytes of the GOT entry. It can work if the linker allows it as in GNU ld.
GNU ld does not relax R_X86_64_GOTPCRELX if the addend if not -4.

I'll try fixing LLD instead.

MaskRay added a comment.EditedNov 23 2020, 1:06 PM

Superseded by D91993

MaskRay commandeered this revision.Nov 30 2020, 8:28 AM
MaskRay edited reviewers, added: sberg; removed: MaskRay.

Obsoleted by D92114

MaskRay abandoned this revision.Nov 30 2020, 8:28 AM