Force SHF_MERGE optimizations with -r

[• espindola]

Summary

This is an alternative to D43942 that handles sh_entsize.

[gbreynoo]

I disagree with forcing -O1 on a user when using -r. -O0 can be useful for debugging and we have tool features that rely on no optimisations. From the user perspective it seems arbitrary that these two flags don't work together, there should be a warning at least. This appears like a gap in implementation regarding combining SHF_MERGE sections by name and sh_entsize. I'm currently looking into implementing this.

[ruiu]

Instead of changing the global configuration of -O, how about returning true from ObjFile::shouldMerge if Config->Relocatable is true?

[• espindola]

In D44917#1049267, @gbreynoo wrote:

I disagree with forcing -O1 on a user when using -r. -O0 can be useful for debugging and we have tool features that rely on no optimisations.

Those tools are broken according to the ELF spec. Please fix them. Also, they are lld only as we are the first to offer a -O0 mode.

[• espindola]

Address review comments.

[ruiu]

LGTM

[gbreynoo]

Our proprietary linker has always offered an equivalent of -O0, at this point we cannot reasonably fix these tools.

[gbreynoo]

Other arguments:

• The inclusion of -O0 in LLD suggests there is a use case, why should this not work with -r?
• -O0 is very useful when linking broken input, often you don't have the source for object files and can't easily make a fix.

[ruiu]

I don't intend to break things that work with your proprietary linker, but the similarity of lld's -O0 and your proprietary linker's feature is a coincidence. I think we reserve the right to do anything that we think make the linker faster for -O0, and it may or may not merge sections. If merging section is faster than not doing that (and that could happen if merging section reduces data to be written significantly), we may turn section merging on for -O0. So it is perhaps a bit too dangerous to depend on the current precise behavior of lld.

[• espindola]

328640

ELF/InputFiles.cpp

ObjFile<ELFT>::getShtGroupEntries(const Elf_Shdr &Sec) {
   ArrayRef<Elf_Word> Entries =
       CHECK(Obj.template getSectionContentsAsArray<Elf_Word>(&Sec), this);
   if (Entries.empty() || Entries[0] != GRP_COMDAT)
     fatal(toString(this) + ": unsupported SHT_GROUP format");
   return Entries.slice(1);
 }
 
 template <class ELFT> bool ObjFile<ELFT>::shouldMerge(const Elf_Shdr &Sec) {
-  // We don't merge sections if -O0 (default is -O1). This makes sometimes
-  // the linker significantly faster, although the output will be bigger.
-  if (Config->Optimize == 0)
+  // On a regular link we don't merge sections if -O0 (default is -O1). This
+  // sometimes makes the linker significantly faster, although the output will
+  // be bigger.
+  //
+  // Doing the same for -r would create a problem as it would combine sections
+  // with different sh_entsize. One option would be to just copy every SHF_MERGE
+  // section as is to the output. While this would produce a valid ELF file with
+  // usable SHF_MERGE sections, tools like (llvm-)?dwarfdump get confused when
+  // they see two .debug_str. We could have separate logic for combining
+  // SHF_MERGE sections based both on their name and sh_entsize, but that seems
+  // to be more trouble than it is worth. Instead, we just use the regular (-O1)
+  // logic for -r.
+  if (Config->Optimize == 0 && !Config->Relocatable)
     return false;
 
   // A mergeable section with size 0 is useless because they don't have
   // any data to merge. A mergeable string section with size 0 can be
   // argued as invalid because it doesn't end with a null character.
   // We'll avoid a mess by handling them as if they were non-mergeable.
   if (Sec.sh_size == 0)
     return false;

test/ELF/merge-reloc-O0.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: ld.lld %t.o -r -o %t2.o -O0
+# RUN: llvm-readobj -s -section-data %t2.o | FileCheck %s
+
+# We combine just the sections with the same name and sh_entsize.
+
+# CHECK:      Name: .foo
+# CHECK-NEXT: Type: SHT_PROGBITS
+# CHECK-NEXT: Flags [
+# CHECK-NEXT:   SHF_ALLOC
+# CHECK-NEXT:   SHF_MERGE
+# CHECK-NEXT: ]
+# CHECK-NEXT: Address:
+# CHECK-NEXT: Offset:
+# CHECK-NEXT: Size: 16
+# CHECK-NEXT: Link:
+# CHECK-NEXT: Info:
+# CHECK-NEXT: AddressAlignment: 8
+# CHECK-NEXT: EntrySize: 8
+# CHECK-NEXT: SectionData (
+# CHECK-NEXT:   0000: 41000000 00000000 42000000 00000000
+# CHECK-NEXT: )
+
+# CHECK:      Name: .foo
+# CHECK-NEXT: Type: SHT_PROGBITS
+# CHECK-NEXT: Flags [
+# CHECK-NEXT:   SHF_ALLOC
+# CHECK-NEXT:   SHF_MERGE
+# CHECK-NEXT: ]
+# CHECK-NEXT: Address:
+# CHECK-NEXT: Offset:
+# CHECK-NEXT: Size: 8
+# CHECK-NEXT: Link:
+# CHECK-NEXT: Info:
+# CHECK-NEXT: AddressAlignment: 4
+# CHECK-NEXT: EntrySize: 4
+# CHECK-NEXT: SectionData (
+# CHECK-NEXT:   0000: 41000000 42000000
+# CHECK-NEXT: )
+
+        .section .foo, "aM",@progbits,8,unique,0
+        .quad 0x41
+        .section .foo, "aM",@progbits,8,unique,1
+        .quad 0x42
+        .section .foo, "aM",@progbits,4,unique,2
+        .long 0x41
+        .long 0x42