[ELF] Do not merge sections in case of relocatable object generation

[atanasyan]

Summary

Do not merge sections if generate a relocatable object. It makes the code simpler because we do not need to update relocations addend to reflect changes introduced by merging. Instead of that we write such "merge" sections into separate output ones and keep SHF_MERGE / SHF_STRINGS flags and sh_entsize value to be able to perform merging later during a final linking.

[ruiu]

ELF/InputFiles.cpp

181–184

The thing that a relocation could point to a location beyond the end of a section is a bug if we don't update a relocation, and that is fixable. So I found this explanation a bit confusing. It is just that we don't merge strings as a design decision because (1) it makes code simpler and (2) the final link will merge strings anyway so we don't lose almost nothing by not doing this at the moment. Could you clarify these points?

[atanasyan]

• Update comment
• Keep sh_entsize values of "merge" sections to allow merging at final linking

[peter.smith]

I think the implementation looks ok. I have made some suggestions to improve the comment, but I understood what it meant as it was written.

ELF/InputFiles.cpp

180–187

A few suggestions for the comment.
"if generate" -> "if generating"
"update relocations addend" -> "update relocation addends"
"separate output ones" -> "separate OutputSections"

[ruiu]

LGTM

[atanasyan]

Thanks for review and the comment improvement.

ELF/InputFiles.cpp

 }
 
 template <class ELFT>
 bool elf::ObjectFile<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)
     return false;
 
+  // In case of relocatable object generation we do not update any relocation
+  // addends, but as a result of the merging section's size might change
+  // and so we get a relocation points to out of the section border.
+  // To prevent this do not merge sections.
+  if (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;
 
   // Check for sh_entsize. The ELF spec is not clear about the zero

test/ELF/merge.s

 // REQUIRES: x86
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %p/Inputs/merge.s -o %t2.o
 // RUN: ld.lld %t.o %t2.o -o %t
 // RUN: llvm-readobj -s -section-data -t %t | FileCheck %s
 // RUN: llvm-objdump -d %t | FileCheck --check-prefix=DISASM %s
 
+// RUN: ld.lld %t.o %t2.o -r -o %t-rel
+// RUN: llvm-readobj -s -section-data %t-rel | FileCheck --check-prefix=REL %s
+
         .section        .mysec,"aM",@progbits,4
         .align  4
         .global foo
         .hidden foo
         .long   0x10
 foo:
         .long   0x42
 bar:
// DISASM-NEXT:   movl    65835, %eax
 
         movl foo+8, %eax
 // addr(0x42) + 8 =  = 65828 + 8 = 65836
 // DISASM-NEXT:   movl    65836, %eax
 
 //  From the other file:  movl .mysec, %eax
 // addr(0x42) = 65828
 // DISASM-NEXT:   movl    65828, %eax
+
+// REL:      Name: .mysec
+// REL-NEXT: Type: SHT_PROGBITS
+// REL-NEXT:    Flags [
+// REL-NEXT:      SHF_ALLOC
+// REL-NEXT:      SHF_MERGE
+// REL-NEXT:    ]
+// REL-NEXT:    Address: 0x0
+// REL-NEXT:    Offset: 0x40
+// REL-NEXT:    Size: 20
+// REL-NEXT:    Link: 0
+// REL-NEXT:    Info: 0
+// REL-NEXT:    AddressAlignment: 4
+// REL-NEXT:    EntrySize: 0
+// REL-NEXT:    SectionData (
+// REL-NEXT:      0000: 10000000 42000000 42000000 42000000
+// REL-NEXT:      0010: 42000000
+// REL-NEXT:    )