[ELF] - Treat .gnu.linkonce sections as COMDAT

[grimar]

Summary

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.

[ruiu]

If I understand https://bugs.llvm.org/show_bug.cgi?id=31586 correctly, this is adding more code to a hack that will be removed in near future. I don't think we should do that unless it is absolutely necessary, especially because this patch touches not a single place but many places in the source code.

[smeenai]

@ruiu, my understanding was that this change would make it less of a hack. It's not terribly difficult to work around as it stands though (you can define your own version of the symbol which got dropped, and in practice that symbol tends to only contain a few instructions); it would be nicer to have the linker handle it, but it's understandable if you think this isn't worthwhile. Thanks for the attempt, @grimar!

[grimar]

Ok.

[ruiu]

ELF/InputFiles.cpp

496–502

".gnu.linkonce.t" is not the only ".gnu.linkonce" section. There are other sections such as ".gnu.linkonce.r". Why can you ignore them but only handle ".gnu.linkonce.t"? It seems something is missing.

[grimar]

ELF/InputFiles.cpp

496–502

In all descriptions I found it was said that we should expect
.gnu.linkonce.NAME or .gnu.linkonce.TYPE.NAME, for example:

(http://www.airs.com/blog/archives/52)
"Any section whose name starts with “.gnu.linkonce.” is a COMDAT section. The associated string is simply the section name itself. Thus the inline function f1 would be put into the section “.gnu.linkonce.f1”"

In that case all we would need here is to take the string NAME which follows last ..
But in real life situation is different and I did not find exact documentation about naming.

gold linker has comment saying that for .gnu.linkonce.t.__i686.get_pc_thunk.bx
__i686.get_pc_thunk.bx should be used as function name. That corresponds
to documentation probably. But at the same time it says that for case of .gnu.linkonce.d.rel.ro.local
the name should not be rel.ro.local, but local. So we generally can not simply take whole
string following the last . for taking comdat group name.

And because this patch was a hack to fix exact user request, I decided to support case
that is simple, clear for me and enough for fixing request.

But anyways, this patch was abandoned, do you want to resurrect it ?

ELF/InputFiles.h

public:
   StringRef SourceFile;
 
 private:
   void
   initializeSections(llvm::DenseSet<llvm::CachedHashStringRef> &ComdatGroups);
   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);
   SymbolBody *createSymbolBody(const Elf_Sym *Sym);
 
   // .shstrtab contents.
   StringRef SectionStringTable;
 

ELF/InputFiles.cpp

case SHT_GROUP: {
 
       // If it is a new section group, we want to keep group members.
       // Group leader sections, which contain indices of group members, are
       // discarded because they are useless beyond this point. The only
       // exception is the -r option because in order to produce re-linkable
       // 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;
       }
 
       // Otherwise, discard group members.
       for (uint32_t SecIndex : getShtGroupEntries(Sec)) {
         if (SecIndex >= Size)
           fatal(toString(this) +
                 ": invalid section index in group: " + Twine(SecIndex));
         this->Sections[SecIndex] = &InputSection::Discarded;
       }
       break;
     }
     case SHT_SYMTAB:
       this->initSymtab(ObjSections, &Sec);
       break;
     case SHT_SYMTAB_SHNDX:
       this->SymtabSHNDX =
           check(Obj.getSHNDXTable(Sec, ObjSections), toString(this));
       break;
     case SHT_STRTAB:
     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
     // have a SHF_LINK_ORDER dependency, this is identified by the sh_link.
     if (Sec.sh_flags & SHF_LINK_ORDER) {
       if (Sec.sh_link >= this->Sections.size())
         fatal(toString(this) + ": invalid sh_link index: " +
               Twine(Sec.sh_link));

 InputSectionBase *toRegularSection(MergeInputSection *Sec) {
   auto *Ret = make<InputSection>(Sec->Flags, Sec->Type, Sec->Alignment,
                                  Sec->Data, Sec->Name);
   Ret->File = Sec->File;
   return Ret;
 }
 
 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) {
   case SHT_ARM_ATTRIBUTES:
     // FIXME: ARM meta-data section. Retain the first attribute section
     // we see. The eglibc ARM dynamic loaders require the presence of an
     // attribute section for dlopen to work.
     // In a full implementation we would merge all attribute sections.
ObjFile<ELFT>::createInputSection(DenseSet<CachedHashStringRef> &ComdatGroups,
   // can use the InputSection to access the data.
   if (Config->GdbIndex &&
       (Name == ".debug_gnu_pubnames" || Name == ".debug_gnu_pubtypes")) {
     auto *Ret = make<InputSection>(this, &Sec, Name);
     Script->discard({Ret});
     return Ret;
   }
 
   // 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
   // .eh_frame_hdr section for runtime. So we handle them with a special
   // class. For relocatable outputs, they are just passed through.
   if (Name == ".eh_frame" && !Config->Relocatable)
     return make<EhInputSection>(this, &Sec, Name);
 

test/ELF/comdat-linkonce2.s

+# 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