[PATCH] D39959: [ELF] - Allow merging of strings sections for -relocatable output.

[Rafael Avila de Espindola via llvm-commits]

What is it that prevent us from just using elf::mergeSections for -r?

If I remember correctly, we don't want to merge input strings when that
would reduce what the final link can do (--gc-sections for example). But
with SHF_MERGE, they should always be fine to merge.

Cheers,
Rafael


George Rimar via Phabricator <reviews at reviews.llvm.org> writes:

> grimar created this revision.
> Herald added subscribers: arichardson, aprantl, emaste.
>
> This is PR35223.
>
> r317406 stopped merging SHF_MERGE sections when -r is given.
> Previously we used Alignment, Flags and section Name together to
> generate a complex key for merging sections. For -r case all 3 were used,
> but for regular case we merged sections by name only for a long time
> and r317406 changed behavior to merge sections by name for all outputs,
> that allowed to simplify code and sped up linker a bit.
>
> But above broke -relocatable output in sence that tools like dwarfdump (both gnu
> and llvm one) expects to see single .debug_str section in output.
> After the r317406 we started producing unique output section for each input
> and tools are upset.
>
> I suggest to allow relocatable output to merge SHF_MERGE strings sections.
> In case when input sections has different sh_entsize, I think we can just drop
> the SHF_MERGE flag and merge them. That looks consistent with what ld.bfd
> do and should not be an issue, because I believe normally string sections
> always have sh_entsize = 1. llvm-mc even ignores any values explicitly set for .debug_str
> sh_entsize and renders 1 to output, though gas allows different values.
>
>
> https://reviews.llvm.org/D39959
>
> Files:
>   ELF/LinkerScript.cpp
>   test/ELF/relocatable-mergeable.s
>
> Index: test/ELF/relocatable-mergeable.s
> ===================================================================
> --- test/ELF/relocatable-mergeable.s
> +++ test/ELF/relocatable-mergeable.s
> @@ -0,0 +1,67 @@
> +# REQUIRES: x86
> +
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1
> +# RUN: ld.lld %t1 -o %t2 -r
> +# RUN: llvm-readobj -sections %t2 | FileCheck %s
> +
> +## Check we merged 2 .strings sections together and dropped SHF_MERGE flag
> +## because they have different sh_entsize. Check we created separate output
> +## section for each non-string mergeable input and keeped SHF_MERGE.
> +
> +# CHECK:      Section {
> +# CHECK:        Index:
> +# CHECK:        Name: .strings
> +# CHECK-NEXT:   Type: SHT_PROGBITS
> +# CHECK-NEXT:   Flags [
> +# CHECK-NEXT:     SHF_STRINGS
> +# CHECK-NEXT:   ]
> +# CHECK-NEXT:   Address:
> +# CHECK-NEXT:   Offset:
> +# CHECK-NEXT:   Size: 8
> +# CHECK-NEXT:   Link:
> +# CHECK-NEXT:   Info:
> +# CHECK-NEXT:   AddressAlignment:
> +# CHECK-NEXT:   EntrySize: 3
> +# CHECK-NEXT: }
> +# CHECK-NEXT: Section {
> +# CHECK-NEXT:   Index:
> +# CHECK-NEXT:   Name: .nonstrings
> +# CHECK-NEXT:   Type: SHT_PROGBITS
> +# CHECK-NEXT:   Flags [
> +# CHECK-NEXT:     SHF_MERGE
> +# CHECK-NEXT:   ]
> +# CHECK-NEXT:   Address:
> +# CHECK-NEXT:   Offset:
> +# CHECK-NEXT:   Size: 4
> +# CHECK-NEXT:   Link:
> +# CHECK-NEXT:   Info:
> +# CHECK-NEXT:   AddressAlignment:
> +# CHECK-NEXT:   EntrySize: 2
> +# CHECK-NEXT: }
> +# CHECK-NEXT: Section {
> +# CHECK-NEXT:   Index:
> +# CHECK-NEXT:   Name: .nonstrings
> +# CHECK-NEXT:   Type: SHT_PROGBITS
> +# CHECK-NEXT:   Flags [
> +# CHECK-NEXT:     SHF_MERGE
> +# CHECK-NEXT:   ]
> +# CHECK-NEXT:   Address:
> +# CHECK-NEXT:   Offset:
> +# CHECK-NEXT:   Size: 4
> +# CHECK-NEXT:   Link:
> +# CHECK-NEXT:   Info:
> +# CHECK-NEXT:   AddressAlignment:
> +# CHECK-NEXT:   EntrySize: 4
> +# CHECK-NEXT: }
> +
> +.section .strings,"MS", at progbits,1,unique,10
> +.asciz "foo"
> +
> +.section .nonstrings,"M", at progbits,2,unique,20
> +.long 11
> +
> +.section .strings,"MS", at progbits,3,unique,30
> +.asciz "bar"
> +
> +.section .nonstrings,"M", at progbits,4,unique,40
> +.long 22
> Index: ELF/LinkerScript.cpp
> ===================================================================
> --- ELF/LinkerScript.cpp
> +++ ELF/LinkerScript.cpp
> @@ -427,6 +427,16 @@
>    return nullptr;
>  }
>  
> +static void addMergeableStringSection(OutputSection *Sec,
> +                                      InputSectionBase *IS) {
> +  assert(Config->Relocatable);
> +  if (Sec->Entsize != IS->Entsize)
> +    Sec->Flags &= ~SHF_MERGE;
> +  if (!(Sec->Flags & SHF_MERGE))
> +    IS->Flags &= ~SHF_MERGE;
> +  Sec->addSection(cast<InputSection>(IS));
> +}
> +
>  static OutputSection *createSection(InputSectionBase *IS,
>                                      StringRef OutsecName) {
>    OutputSection *Sec = Script->createOutputSection(OutsecName, "<internal>");
> @@ -466,11 +476,23 @@
>      return Out->RelocationSection;
>    }
>  
> -  // When control reaches here, mergeable sections have already been
> -  // merged except the -r case. If that's the case, we do not combine them
> -  // and let final link to handle this optimization.
> -  if (Config->Relocatable && (IS->Flags & SHF_MERGE))
> -    return createSection(IS, OutsecName);
> +  // When control reaches here, mergeable sections have already been merged
> +  // except the -r case. If that's the case, we do not really want to care about
> +  // merging sections, leaving this task for final link. But unfortunately we
> +  // still need to do that for .debug_str sections, because different tools
> +  // like dwarfdump expects to see single instance in the output. To handle this
> +  // case we merge string sections and just drop SHF_MERGE optimization flag if
> +  // entry sizes of input and output sections are different. That normally
> +  // should not happen as entry size usually is always 1 for strings sections.
> +  if (Config->Relocatable && (IS->Flags & SHF_MERGE)) {
> +    OutputSection *&Sec = Map[OutsecName];
> +    if (!Sec || !(IS->Flags & SHF_STRINGS)) {
> +      Sec = createSection(IS, OutsecName);
> +      return Sec;
> +    }
> +    addMergeableStringSection(Sec, IS);
> +    return nullptr;
> +  }
>  
>    //  The ELF spec just says
>    // ----------------------------------------------------------------