[ELF] Loose condition for relocation with a symbol

[NikolaPrica]

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).

[MaskRay]

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.

[MaskRay]

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

ninja check-lld-elf

[NikolaPrica]

@MaskRay Thanks for the heads up and review!

[NikolaPrica]

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.

[MaskRay]

In D64327#1600769, @NikolaPrica wrote:

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.

[NikolaPrica]

In D64327#1600792, @MaskRay wrote:

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.

llvm/trunk/lib/MC/ELFObjectWriter.cpp

bool ELFObjectWriter::shouldRelocateWithSymbol(const MCAssembler &Asm,
   // that it pointed to another string and subtracting 42 at runtime will
   // produce the wrong value.
   if (Sym->isInSection()) {
     auto &Sec = cast<MCSectionELF>(Sym->getSection());
     unsigned Flags = Sec.getFlags();
     if (Flags & ELF::SHF_MERGE) {
       if (C != 0)
         return true;
-
-      // It looks like gold has a bug (http://sourceware.org/PR16794) and can
-      // only handle section relocations to mergeable sections if using RELA.
-      if (!hasRelocationAddend())
-        return true;
     }
 
     // Most TLS relocations use a got, so they need the symbol. Even those that
     // are just an offset (@tpoff), require a symbol in gold versions before
     // 5efeedf61e4fe720fd3e9a08e6c91c10abb66d42 (2014-09-26) which fixed
     // http://sourceware.org/PR16773.
     if (Flags & ELF::SHF_TLS)
       return true;

llvm/trunk/test/MC/ELF/basic-elf-32.s

 // CHECK:     Name: (0)
 
 // CHECK:     Name: .text
 
 // CHECK:     Name: .rel.text
 
 // CHECK: Relocations [
 // CHECK:   Section {{.*}} .rel.text {
-// CHECK:     0x6  R_386_32   .L.str1
+// CHECK:     0x6  R_386_32   .rodata.str1.1 0x0
 // CHECK:     0xB  R_386_PC32 puts
-// CHECK:     0x12 R_386_32   .L.str2
+// CHECK:     0x12 R_386_32   .rodata.str1.1 0x0
 // CHECK:     0x17 R_386_PC32 puts
 // CHECK:   }
 // CHECK: ]
 
 // CHECK: Symbols [
 
 // CHECK:   Symbol {
 // CHECK:     Name: main

llvm/trunk/test/MC/ELF/compression.s

 // CHECK-GNU-STYLE: Contents of
 
 // Decompress one valid dwarf section just to check that this roundtrips,
 // we use .zdebug_str section for that
 // STR: perfectly compressable data sample *****************************************
 
 // In x86 32 bit named symbols are used for temporary symbols in merge
 // sections, so make sure we handle symbols inside compressed sections
-// 386-SYMBOLS-GNU: Name: .Linfo_string0
+// 386-SYMBOLS-GNU: Name: .zdebug_str
 // 386-SYMBOLS-GNU-NOT: }
 // 386-SYMBOLS-GNU: Section: .zdebug_str
 
 // Now check the zlib style output:
 
 // Don't compress small sections, such as this simple debug_abbrev example
 // CHECK-ZLIB-STYLE: Contents of section .debug_abbrev:
 // CHECK-ZLIB-STYLE-NOT: ZLIB

 // ZLIB-STYLE-FLAGS-NEXT:   Address:
 // ZLIB-STYLE-FLAGS-NEXT:   Offset:
 // ZLIB-STYLE-FLAGS-NEXT:   Size:
 // ZLIB-STYLE-FLAGS-NEXT:   Link:
 // ZLIB-STYLE-FLAGS-NEXT:   Info:
 // ZLIB-STYLE-FLAGS32-NEXT: AddressAlignment: 4
 // ZLIB-STYLE-FLAGS64-NEXT: AddressAlignment: 8
 
-// 386-SYMBOLS-ZLIB: Name: .Linfo_string0
+// 386-SYMBOLS-ZLIB: Name: .debug_str
 // 386-SYMBOLS-ZLIB-NOT: }
 // 386-SYMBOLS-ZLIB: Section: .debug_str
 
   .section  .debug_line,"",@progbits
 
   .section  .debug_abbrev,"",@progbits
 .Lsection_abbrev:
   .byte 1                       # Abbreviation Code

llvm/trunk/test/MC/ELF/relocation-386.s

 // RUN: llvm-mc -filetype=obj -triple i386-pc-linux-gnu %s -relax-relocations=false -o - | llvm-readobj -r  | FileCheck  %s --check-prefix=CHECK --check-prefix=I386
 // RUN: llvm-mc -filetype=obj -triple i386-pc-elfiamcu %s -relax-relocations=false  -o - | llvm-readobj -r  | FileCheck  %s --check-prefix=CHECK --check-prefix=IAMCU
 
 // Test that we produce the correct relocation types and that the relocations
 // correctly point to the section or the symbol.
 
 // IAMCU: Format: ELF32-iamcu
 // I386: Format: ELF32-i386
 // CHECK:      Relocations [
 // CHECK-NEXT:   Section {{.*}} .rel.text {
-// CHECK-NEXT:     0x2          R_386_GOTOFF     .Lfoo 0x0
+// CHECK-NEXT:     0x2          R_386_GOTOFF     .rodata.str1.16 0x0
 // CHECK-NEXT:     0x{{[^ ]+}}  R_386_PLT32      bar2 0x0
 // CHECK-NEXT:     0x{{[^ ]+}}  R_386_GOTPC      _GLOBAL_OFFSET_TABLE_ 0x0
 // Relocation 3 (bar3@GOTOFF) is done with symbol 7 (bss)
 // CHECK-NEXT:     0x{{[^ ]+}}  R_386_GOTOFF     .bss 0x0
 // Relocation 4 (bar2@GOT) is of type R_386_GOT32
 // CHECK-NEXT:     0x{{[^ ]+}}  R_386_GOT32      bar2j 0x0
 
 // Relocation 5 (foo@TLSGD) is of type R_386_TLS_GD

llvm/trunk/test/MC/Mips/elf-relsym.s

 // RUN: llvm-mc -filetype=obj -triple mipsel-unknown-linux %s -o - | llvm-readobj --symbols | FileCheck %s
 
 // Check that the appropriate symbols were created.
 
 // CHECK: Symbols [
 // CHECK:   Symbol {
-// CHECK:     Name: $.str
+// CHECK:     Name: .rodata.cst8
 // CHECK:   }
 // CHECK:   Symbol {
-// CHECK:     Name: $.str1
-// CHECK:   }
-// CHECK:   Symbol {
-// CHECK:     Name: $CPI0_0
-// CHECK:   }
-// CHECK:   Symbol {
-// CHECK:     Name: $CPI0_1
+// CHECK:     Name: .rodata.str1.1
 // CHECK:   }
 // CHECK: ]
 
   .text
   .abicalls
   .section  .mdebug.abi32,"",@progbits
   .file "/home/espindola/llvm/llvm/test/MC/Mips/elf-relsym.ll"
   .section  .rodata.cst8,"aM",@progbits,8

llvm/trunk/test/MC/Mips/xgot.s

 // RUN: llvm-mc -filetype=obj -triple mipsel-unknown-linux  %s -o - | llvm-readobj -r | FileCheck %s
 
 // Check that the appropriate relocations were created.
 // For the xgot case we want to see R_MIPS_[GOT|CALL]_[HI|LO]16.
 
 // CHECK: Relocations [
 // CHECK:   0x0 R_MIPS_HI16 _gp_disp
 // CHECK:   0x4 R_MIPS_LO16 _gp_disp
 // CHECK:   0x14 R_MIPS_GOT_HI16 ext_1
 // CHECK:   0x1C R_MIPS_GOT_LO16 ext_1
 // CHECK:   0x24 R_MIPS_CALL_HI16 printf
 // CHECK:   0x30 R_MIPS_CALL_LO16 printf
-// CHECK:   0x2C R_MIPS_GOT16 $.str
-// CHECK:   0x38 R_MIPS_LO16 $.str
+// CHECK:   0x2C R_MIPS_GOT16 .rodata.str1.1 0x0
+// CHECK:   0x38 R_MIPS_LO16 .rodata.str1.1 0x0
 // CHECK: ]
 
   .text
   .abicalls
   .section  .mdebug.abi32,"",@progbits
   .file "/home/espindola/llvm/llvm/test/MC/Mips/xgot.ll"
   .text
   .globl  fill