[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 ↗

(On Diff #72973)

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

179 ↗

(On Diff #73425)

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.

lld/trunk/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;
 
+  // Do not merge sections if generating a relocatable object. It makes
+  // the code simpler because we do not need to update relocation addends
+  // to reflect changes introduced by merging. Instead of that we write
+  // such "merge" sections into separate OutputSections and keep SHF_MERGE
+  // / SHF_STRINGS flags and sh_entsize value to be able to perform merging
+  // later during a final linking.
+  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

lld/trunk/ELF/OutputSections.cpp

 
 template <class ELFT>
 void OutputSection<ELFT>::addSection(InputSectionBase<ELFT> *C) {
   assert(C->Live);
   auto *S = cast<InputSection<ELFT>>(C);
   Sections.push_back(S);
   S->OutSec = this;
   this->updateAlignment(S->Alignment);
+  // Keep sh_entsize value of the input section to be able to perform merging
+  // later during a final linking using the generated relocatable object.
+  if (Config->Relocatable && (S->getSectionHdr()->sh_flags & SHF_MERGE))
+    this->Header.sh_entsize = S->getSectionHdr()->sh_entsize;
 }
 
 // This function is called after we sort input sections
 // and scan relocations to setup sections' offsets.
 template <class ELFT> void OutputSection<ELFT>::assignOffsets() {
   uintX_t Off = this->Header.sh_size;
   for (InputSection<ELFT> *S : Sections) {
     Off = alignTo(Off, S->Alignment);
static SectionKey<ELFT::Is64Bits> createKey(InputSectionBase<ELFT> *C,
                                             StringRef OutsecName) {
   const typename ELFT::Shdr *H = C->getSectionHdr();
   typedef typename ELFT::uint uintX_t;
   uintX_t Flags = getOutFlags(C);
 
   // For SHF_MERGE we create different output sections for each alignment.
   // This makes each output section simple and keeps a single level mapping from
   // input to output.
+  // In case of relocatable object generation we do not try to perform merging
+  // and treat SHF_MERGE sections as regular ones, but also create different
+  // output sections for them to allow merging at final linking stage.
   uintX_t Alignment = 0;
-  if (isa<MergeInputSection<ELFT>>(C))
+  if (isa<MergeInputSection<ELFT>>(C) ||
+      (Config->Relocatable && (H->sh_flags & SHF_MERGE)))
     Alignment = std::max(H->sh_addralign, H->sh_entsize);
 
   uint32_t Type = H->sh_type;
   return SectionKey<ELFT::Is64Bits>{OutsecName, Type, Flags, Alignment};
 }
 
 template <class ELFT>
 std::pair<OutputSectionBase<ELFT> *, bool>

lld/trunk/test/ELF/merge-reloc.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: ld.lld %t.o -r -o %t-rel
+# RUN: llvm-readobj -s -section-data %t-rel | FileCheck %s
+
+# When linker generates a relocatable object it should keep "merge"
+# sections as-is: do not merge content, do not join regular and
+# "merge" sections, do not joint "merge" sections with different
+# entry size.
+
+# CHECK:      Section {
+# CHECK:        Index:
+# CHECK:        Name: .data
+# 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: 12
+# CHECK-NEXT:   Link: 0
+# CHECK-NEXT:   Info: 0
+# CHECK-NEXT:   AddressAlignment: 4
+# CHECK-NEXT:   EntrySize: 4
+# CHECK-NEXT:   SectionData (
+# CHECK-NEXT:     0000: 42000000 42000000 42000000
+# CHECK-NEXT:   )
+# CHECK-NEXT: }
+# CHECK:      Section {
+# CHECK:        Index:
+# CHECK:        Name: .data
+# 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: 0
+# CHECK-NEXT:   Info: 0
+# CHECK-NEXT:   AddressAlignment: 8
+# CHECK-NEXT:   EntrySize: 8
+# CHECK-NEXT:   SectionData (
+# CHECK-NEXT:     0000: 42000000 42000000 42000000 42000000
+# CHECK-NEXT:   )
+# CHECK-NEXT: }
+# CHECK:      Section {
+# CHECK:        Index:
+# CHECK:        Name: .data
+# CHECK-NEXT:   Type: SHT_PROGBITS
+# CHECK-NEXT:   Flags [
+# CHECK-NEXT:     SHF_ALLOC
+# CHECK-NEXT:     SHF_WRITE
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Address:
+# CHECK-NEXT:   Offset:
+# CHECK-NEXT:   Size: 16
+# CHECK-NEXT:   Link: 0
+# CHECK-NEXT:   Info: 0
+# CHECK-NEXT:   AddressAlignment: 1
+# CHECK-NEXT:   EntrySize: 0
+# CHECK-NEXT:   SectionData (
+# CHECK-NEXT:     0000: 42000000 42000000 42000000 42000000
+# CHECK-NEXT:   )
+# CHECK-NEXT: }
+
+        .section        .data.1,"aM",@progbits,4
+        .align  4
+        .global foo
+foo:
+        .long   0x42
+        .long   0x42
+        .long   0x42
+
+        .section        .data.2,"aM",@progbits,8
+        .align  8
+        .global bar
+bar:
+        .long   0x42
+        .long   0x42
+        .long   0x42
+        .long   0x42
+
+        .data
+        .global gar
+zed:
+        .long   0x42
+        .long   0x42
+        .long   0x42
+        .long   0x42