Page MenuHomePhabricator

[ELF] Loose condition for relocation with a symbol
ClosedPublic

Authored by NikolaPrica on Jul 8 2019, 5:55 AM.

Details

Summary

Deleted code was introduced as a work around for a bug in the gold linker (http://sourceware.org/PR16794). Test case that was given as a reason for this part of code, the one on previous link, now works for the gold.
This condition is too strict and when a code is compiled with debug info it forces generation of numerous relocations with symbol for architectures that do not have relocation addend. This was discovered with older version of the compiler and with this patch it reduced the size of a big ARM-32 debug image by 33% . It contained ~68M of relocations symbols out of total ~71M symbols (96% of symbols table was generated for relocations with symbol).

Diff Detail

Repository
rL LLVM

Event Timeline

NikolaPrica created this revision.Jul 8 2019, 5:55 AM
MaskRay accepted this revision.Jul 8 2019, 10:41 PM

I have tried the reproduce on http://sourceware.org/PR16794 with binutils 2.26 (2016) and it works:

% /tmp/p/binutils-2.26/Release/gold/ld-new @response.txt
% ./a.out 
xabcde
abcde

I cannot test the gold shipped with binutils 2.25 (2014) because it doesn't support R_386_GOT32X

% /tmp/p/binutils-2.25/Release/gold/ld-new @response.txt
/tmp/p/binutils-2.25/Release/gold/ld-new: error: usr/lib/i386-linux-gnu/crti.o: unsupported reloc 43 against global symbol __gmon_start__
/tmp/p/binutils-2.25/Release/gold/ld-new: error: usr/lib/gcc/x86_64-linux-gnu/7/32/crtbeginS.o: unsupported reloc 43 against global symbol _ITM_deregisterTMCloneTable
/tmp/p/binutils-2.25/Release/gold/ld-new: error: usr/lib/gcc/x86_64-linux-gnu/7/32/crtbeginS.o: unsupported reloc 43 against global symbol _ITM_registerTMCloneTable
/tmp/p/binutils-2.25/Release/gold/ld-new: error: usr/lib/gcc/x86_64-linux-gnu/7/32/crtbeginS.o: unsupported reloc 43 against global symbol __cxa_finalize
usr/lib/i386-linux-gnu/crti.o(.init+0x11): error: unsupported reloc 43
usr/lib/gcc/x86_64-linux-gnu/7/32/crtbeginS.o(.text+0x1d): error: unsupported reloc 43
usr/lib/gcc/x86_64-linux-gnu/7/32/crtbeginS.o(.text+0x70): error: unsupported reloc 43
usr/lib/gcc/x86_64-linux-gnu/7/32/crtbeginS.o(.text+0xad): error: unsupported reloc 43

The status is good enough. The bug probably only existed in some pre-2014 version of gold. Maintaining the workaround harms code size, so LG.

This revision is now accepted and ready to land.Jul 8 2019, 10:41 PM

BTW, lld/test/ELF/mips-got-string.s needs updating.

ninja check-lld-elf

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2019, 4:18 AM
Herald added a subscriber: jrtc27. · View Herald Transcript

@MaskRay Thanks for the heads up and review!

This patch is reverted. Bug in gold compiler is not solved after all. We tested this functionality without optimizations enabled. With the "-O2" flag bug is still present.

This patch is reverted. Bug in gold compiler is not solved after all. We tested this functionality without optimizations enabled. With the "-O2" flag bug is still present.

@NikolaPrica Do you mean with the linker option -O2, i.e. the compiler driver option -Wl,-O2, gold can produce incorrect result? If that is the case, I don't think we should revert the patch. ld.bfd and gold default to -O0 (lld defaults to -O1), it is extremely rare for a project to specify -Wl,-O2.

Do you mean with the linker option -O2, i.e. the compiler driver option -Wl,-O2, gold can produce incorrect result? If that is the case, I don't think we should revert the patch. ld.bfd and gold default to -O0 (lld defaults to -O1), it is extremely rare for a project to specify -Wl,-O2.

Yes. That is the case. With -O1 it works. The thing is that this patch breaks sanitizer-x86_64-linux and sanitizer-x86_64-linux-android build bots. On these build bots default linker is gold and apparently it is using -O2 which is causing failure.