[PATCH] D36573: [ELF] - Treat .gnu.linkonce sections as COMDAT

[Rafael Avila de Espindola via llvm-commits]

I am not a fan of this change.

Correct if I am wrong, but in all software that lld was ever tested in,
only one case showed up: a single file in glibc. The only reason it
shows up is because it is an old assembly file that has never been
updated.

Linkonce was an incomplete design and we really should push users to
switch to comdats.

Cheers,
Rafael

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

> grimar created this revision.
> Herald added a subscriber: emaste.
>
> This is request from PR31586 comment.
>
> .gnu.linkonce were invented before regular COMDAT sections
> and should work in the same way (http://www.airs.com/blog/archives/52).
>
> Previously we dropped such sections.
> Patch makes them COMDATS.
>
> It based on logic of https://reviews.llvm.org/D28430?id=83485 which
> was changed in later diff.
>
>
> https://reviews.llvm.org/D36573
>
> Files:
>   ELF/InputFiles.cpp
>   ELF/InputFiles.h
>   test/ELF/comdat-linkonce2.s
>
>
> Index: test/ELF/comdat-linkonce2.s
> ===================================================================
> --- test/ELF/comdat-linkonce2.s
> +++ test/ELF/comdat-linkonce2.s
> @@ -0,0 +1,11 @@
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
> +# RUN: ld.lld %t.o -o %t
> +
> +.section .gnu.linkonce.t.zed
> +.globl zed
> +zed:
> +
> +.text
> +.globl _start
> +_start:
> + .quad zed
> Index: ELF/InputFiles.h
> ===================================================================
> --- ELF/InputFiles.h
> +++ ELF/InputFiles.h
> @@ -206,7 +206,9 @@
>    void initializeSymbols();
>    void initializeDwarfLine();
>    InputSectionBase *getRelocTarget(const Elf_Shdr &Sec);
> -  InputSectionBase *createInputSection(const Elf_Shdr &Sec);
> +  InputSectionBase *
> +  createInputSection(llvm::DenseSet<llvm::CachedHashStringRef> &ComdatGroups,
> +                     const Elf_Shdr &Sec);
>    StringRef getSectionName(const Elf_Shdr &Sec);
>  
>    bool shouldMerge(const Elf_Shdr &Sec);
> Index: ELF/InputFiles.cpp
> ===================================================================
> --- ELF/InputFiles.cpp
> +++ ELF/InputFiles.cpp
> @@ -305,7 +305,7 @@
>        // object files, we want to pass through basically everything.
>        if (IsNew) {
>          if (Config->Relocatable)
> -          this->Sections[I] = createInputSection(Sec);
> +          this->Sections[I] = createInputSection(ComdatGroups, Sec);
>          continue;
>        }
>  
> @@ -329,7 +329,7 @@
>      case SHT_NULL:
>        break;
>      default:
> -      this->Sections[I] = createInputSection(Sec);
> +      this->Sections[I] = createInputSection(ComdatGroups, Sec);
>      }
>  
>      // .ARM.exidx sections have a reverse dependency on the InputSection they
> @@ -372,7 +372,9 @@
>  }
>  
>  template <class ELFT>
> -InputSectionBase *ObjFile<ELFT>::createInputSection(const Elf_Shdr &Sec) {
> +InputSectionBase *
> +ObjFile<ELFT>::createInputSection(DenseSet<CachedHashStringRef> &ComdatGroups,
> +                                  const Elf_Shdr &Sec) {
>    StringRef Name = getSectionName(Sec);
>  
>    switch (Sec.sh_type) {
> @@ -493,10 +495,15 @@
>  
>    // The linkonce feature is a sort of proto-comdat. Some glibc i386 object
>    // files contain definitions of symbol "__x86.get_pc_thunk.bx" in linkonce
> -  // sections. Drop those sections to avoid duplicate symbol errors.
> +  // sections. Section usually named as '.gnu.linkonce.TYPE.NAME', where NAME
> +  // is a symbol name this section contains and also a name of comdat group.
> +  // Usually NAME does not contain dots ('.'), but case above is special.
> +  // Since this is temporary hack, we handle only ".gnu.linkonce.t.*" sections
> +  // as minimal case for simplicity.
>    // FIXME: This is glibc PR20543, we should remove this hack once that has been
>    // fixed for a while.
> -  if (Name.startswith(".gnu.linkonce."))
> +  if (Name.startswith(".gnu.linkonce.t.") &&
> +      !ComdatGroups.insert(CachedHashStringRef(Name.drop_front(16))).second)
>      return &InputSection::Discarded;
>  
>    // The linker merges EH (exception handling) frames and creates a
>
>
> Index: test/ELF/comdat-linkonce2.s
> ===================================================================
> --- test/ELF/comdat-linkonce2.s
> +++ test/ELF/comdat-linkonce2.s
> @@ -0,0 +1,11 @@
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
> +# RUN: ld.lld %t.o -o %t
> +
> +.section .gnu.linkonce.t.zed
> +.globl zed
> +zed:
> +
> +.text
> +.globl _start
> +_start:
> + .quad zed
> Index: ELF/InputFiles.h
> ===================================================================
> --- ELF/InputFiles.h
> +++ ELF/InputFiles.h
> @@ -206,7 +206,9 @@
>    void initializeSymbols();
>    void initializeDwarfLine();
>    InputSectionBase *getRelocTarget(const Elf_Shdr &Sec);
> -  InputSectionBase *createInputSection(const Elf_Shdr &Sec);
> +  InputSectionBase *
> +  createInputSection(llvm::DenseSet<llvm::CachedHashStringRef> &ComdatGroups,
> +                     const Elf_Shdr &Sec);
>    StringRef getSectionName(const Elf_Shdr &Sec);
>  
>    bool shouldMerge(const Elf_Shdr &Sec);
> Index: ELF/InputFiles.cpp
> ===================================================================
> --- ELF/InputFiles.cpp
> +++ ELF/InputFiles.cpp
> @@ -305,7 +305,7 @@
>        // object files, we want to pass through basically everything.
>        if (IsNew) {
>          if (Config->Relocatable)
> -          this->Sections[I] = createInputSection(Sec);
> +          this->Sections[I] = createInputSection(ComdatGroups, Sec);
>          continue;
>        }
>  
> @@ -329,7 +329,7 @@
>      case SHT_NULL:
>        break;
>      default:
> -      this->Sections[I] = createInputSection(Sec);
> +      this->Sections[I] = createInputSection(ComdatGroups, Sec);
>      }
>  
>      // .ARM.exidx sections have a reverse dependency on the InputSection they
> @@ -372,7 +372,9 @@
>  }
>  
>  template <class ELFT>
> -InputSectionBase *ObjFile<ELFT>::createInputSection(const Elf_Shdr &Sec) {
> +InputSectionBase *
> +ObjFile<ELFT>::createInputSection(DenseSet<CachedHashStringRef> &ComdatGroups,
> +                                  const Elf_Shdr &Sec) {
>    StringRef Name = getSectionName(Sec);
>  
>    switch (Sec.sh_type) {
> @@ -493,10 +495,15 @@
>  
>    // The linkonce feature is a sort of proto-comdat. Some glibc i386 object
>    // files contain definitions of symbol "__x86.get_pc_thunk.bx" in linkonce
> -  // sections. Drop those sections to avoid duplicate symbol errors.
> +  // sections. Section usually named as '.gnu.linkonce.TYPE.NAME', where NAME
> +  // is a symbol name this section contains and also a name of comdat group.
> +  // Usually NAME does not contain dots ('.'), but case above is special.
> +  // Since this is temporary hack, we handle only ".gnu.linkonce.t.*" sections
> +  // as minimal case for simplicity.
>    // FIXME: This is glibc PR20543, we should remove this hack once that has been
>    // fixed for a while.
> -  if (Name.startswith(".gnu.linkonce."))
> +  if (Name.startswith(".gnu.linkonce.t.") &&
> +      !ComdatGroups.insert(CachedHashStringRef(Name.drop_front(16))).second)
>      return &InputSection::Discarded;
>  
>    // The linker merges EH (exception handling) frames and creates a