[ELF] - Do not allow -r to eat comdats.

[grimar]

Summary

This is PR33052, "Bug 33052 - -r eats comdats ".

To fix it I stop removing group section from out when -r is given
and fixing SHT_GROUP content when writing it just like we do some
other fixup, e.g. for Rel[a]. (it needs fix for section indices that ar in group).

[ruiu]

In general, I think we pass through all input sections to an output when the -r option is given. But in this patch you deduplicates comdat groups. Do you need that?

[grimar]

In D33485#763648, @ruiu wrote:

In general, I think we pass through all input sections to an output when the -r option is given. But in this patch you deduplicates comdat groups. Do you need that?

I was need it for relocatable-eh-frame.s

It has next code:

.section .foo,"aG",@progbits,bar,comdat
.cfi_startproc
.cfi_endproc

And uses this file twice with -r:

# RUN: ld.lld -r %t.o %t.o -o %t

Content inside a single object is something like .group (SHT_GROUP) + .foo sections.
Now if we do not do deduplication then 2 .group input sections will be merged into single output .group and
have content like [flag, section index, flag, section index] which is just broken. .foo's will be merged too.
We probably, like Rafael mentioned, can have 2 group sections with same signature and do emit separate sections for .foo's as well,
but deduplication seems much easier solution and itself works as some cheap output optimization.

[grimar]

• Addressed review comments.

[ruiu]

ELF/InputFiles.cpp

306 ↗

(On Diff #100217)

This needs an explanation.

322 ↗

(On Diff #100217)

Format

ELF/InputSection.cpp

302 ↗

(On Diff #100217)

Avoid ELFT::Word` in general. You can use read32 instead. You can also remove ELFT from template parameter list if you use Config->Wordsize.

701 ↗

(On Diff #100217)

Do not use ELFT::Word.

ELF/InputSection.h

324 ↗

(On Diff #100217)

I prefer more specific name -- like copyShtGroup, as Group sounds too generic.

ELF/OutputSections.cpp

129–141 ↗

(On Diff #100217)

This is too much details to write in this funciton.

276 ↗

(On Diff #100217)

This looks a bit tricky. We should turn off SHF_COMPRESSED when we decompress sections.

[grimar]

• Addressed review comments.

ELF/InputFiles.cpp

306 ↗

(On Diff #100217)

Done.

322 ↗

(On Diff #100217)

It is just formatting in this way :(

ELF/InputSection.cpp

302 ↗

(On Diff #100217)

Done. (Not sure why did you mention Config->Wordsize though, all values here are 4 bytes I believe).

701 ↗

(On Diff #100217)

Done.

ELF/InputSection.h

324 ↗

(On Diff #100217)

Done.

ELF/OutputSections.cpp

129–141 ↗

(On Diff #100217)

Done.

276 ↗

(On Diff #100217)

May be. This patch just removes SHF_GROUP here.

[ruiu]

ELF/InputFiles.cpp

322 ↗

(On Diff #100217)

I think break after } is somewhat irregular. Move break before }.

306–309 ↗

(On Diff #100381)

Move it after case SHT_GROUP

ELF/InputSection.cpp

304 ↗

(On Diff #100381)

You can remove this temporary variable.

ELF/OutputSections.cpp

122–123 ↗

(On Diff #100381)

It took for a while to understand this code. What was confusing is that the first section is not special. It is just that when you pass -r, we create an output section for each input section, so there's always only one section in an output file.

Please add

assert(Config->Relocatable && Sec->Sections.size == 1);

remove First variable and use Sec->Sections[0] directly. This should improve readability.

276 ↗

(On Diff #100217)

Please rebase.

[grimar]

• Addressed review comments.

ELF/InputFiles.cpp

306–309 ↗

(On Diff #100381)

Done.

ELF/InputSection.cpp

304 ↗

(On Diff #100381)

Right. Done.

ELF/OutputSections.cpp

122–123 ↗

(On Diff #100381)

Done.

[ruiu]

LGTM

ELF/InputSection.cpp

305 ↗

(On Diff #100428)

endianness::native is alarming although it is actually correct. It is better to write

*To++ = From[0];

lld/trunk/ELF/InputFiles.cpp

for (size_t I = 0, E = ObjSections.size(); I < E; I++) {
     // if -r is given, we'll let the final link discard such sections.
     // This is compatible with GNU.
     if ((Sec.sh_flags & SHF_EXCLUDE) && !Config->Relocatable) {
       this->Sections[I] = &InputSection::Discarded;
       continue;
     }
 
     switch (Sec.sh_type) {
-    case SHT_GROUP:
+    case SHT_GROUP: {
+      // We discard comdat sections usually. When -r we should not do that. We
+      // still do deduplication in this case to simplify implementation, because
+      // otherwise merging group sections together would requre additional
+      // regeneration of its contents.
+      bool New = ComdatGroups
+                     .insert(CachedHashStringRef(
+                         getShtGroupSignature(ObjSections, Sec)))
+                     .second;
+      if (New && Config->Relocatable)
+        this->Sections[I] = createInputSection(Sec, SectionStringTable);
+      else
         this->Sections[I] = &InputSection::Discarded;
-      if (ComdatGroups
-              .insert(
-                  CachedHashStringRef(getShtGroupSignature(ObjSections, Sec)))
-              .second)
+      if (New)
         continue;
+
       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:

lld/trunk/ELF/InputSection.h

public:
   uint32_t Class[2] = {0, 0};
 
   // Called by ICF to merge two input sections.
   void replace(InputSection *Other);
 
 private:
   template <class ELFT, class RelTy>
   void copyRelocations(uint8_t *Buf, llvm::ArrayRef<RelTy> Rels);
+
+  void copyShtGroup(uint8_t *Buf);
 };
 
 // The list of all input sections.
 extern std::vector<InputSectionBase *> InputSections;
 
 } // namespace elf
 
 std::string toString(const elf::InputSectionBase *);
 } // namespace lld
 
 #endif

lld/trunk/ELF/InputSection.cpp

bool InputSection::classof(const SectionBase *S) {
   return S->kind() == SectionBase::Regular ||
          S->kind() == SectionBase::Synthetic;
 }
 
 bool InputSectionBase::classof(const SectionBase *S) {
   return S->kind() != Output;
 }
 
+void InputSection::copyShtGroup(uint8_t *Buf) {
+  assert(this->Type == SHT_GROUP);
+
+  ArrayRef<uint32_t> From = getDataAs<uint32_t>();
+  uint32_t *To = reinterpret_cast<uint32_t *>(Buf);
+
+  // First entry is a flag word, we leave it unchanged.
+  *To++ = From[0];
+
+  // Here we adjust indices of sections that belong to group as it
+  // might change during linking.
+  ArrayRef<InputSectionBase *> Sections = this->File->getSections();
+  for (uint32_t Val : From.slice(1)) {
+    uint32_t Index = read32(&Val, Config->Endianness);
+    write32(To++, Sections[Index]->OutSec->SectionIndex, Config->Endianness);
+  }
+}
+
 InputSectionBase *InputSection::getRelocatedSection() {
   assert(this->Type == SHT_RELA || this->Type == SHT_REL);
   ArrayRef<InputSectionBase *> Sections = this->File->getSections();
   return Sections[this->Info];
 }
 
 // This is used for -r and --emit-relocs. We can't use memcpy to copy
 // relocations because we need to update symbol table offset and section index
if (this->Type == SHT_RELA) {
     return;
   }
   if (this->Type == SHT_REL) {
     copyRelocations<ELFT>(Buf + OutSecOff,
                           this->template getDataAs<typename ELFT::Rel>());
     return;
   }
 
+  // If -r is given, linker should keep SHT_GROUP sections. We should fixup
+  // them, see copyShtGroup().
+  if (this->Type == SHT_GROUP) {
+    copyShtGroup(Buf + OutSecOff);
+    return;
+  }
+
   // Copy section contents from source object file to output file
   // and then apply relocations.
   memcpy(Buf + OutSecOff, Data.data(), Data.size());
   uint8_t *BufEnd = Buf + OutSecOff + Data.size();
   this->relocate<ELFT>(Buf, BufEnd);
 }
 
 void InputSection::replace(InputSection *Other) {

lld/trunk/ELF/OutputSections.cpp

template <class ELFT> void OutputSection::maybeCompress() {
   if (Error E = zlib::compress(toStringRef(Buf), CompressedData))
     fatal("compress failed: " + llvm::toString(std::move(E)));
 
   // Update section headers.
   Size = sizeof(Elf_Chdr) + CompressedData.size();
   Flags |= SHF_COMPRESSED;
 }
 
+template <class ELFT> static void finalizeShtGroup(OutputSection *Sec) {
+  // sh_link field for SHT_GROUP sections should contain the section index of
+  // the symbol table.
+  Sec->Link = InX::SymTab->OutSec->SectionIndex;
+
+  // sh_link then contain index of an entry in symbol table section which
+  // provides signature of the section group.
+  elf::ObjectFile<ELFT> *Obj = Sec->Sections[0]->getFile<ELFT>();
+  assert(Config->Relocatable && Sec->Sections.size() == 1);
+  ArrayRef<SymbolBody *> Symbols = Obj->getSymbols();
+  Sec->Info = InX::SymTab->getSymbolIndex(Symbols[Sec->Sections[0]->Info - 1]);
+}
+
 template <class ELFT> void OutputSection::finalize() {
   if ((this->Flags & SHF_LINK_ORDER) && !this->Sections.empty()) {
     std::sort(Sections.begin(), Sections.end(), compareByFilePosition);
     assignOffsets();
 
     // We must preserve the link order dependency of sections with the
     // SHF_LINK_ORDER flag. The dependency is indicated by the sh_link field. We
     // need to translate the InputSection sh_link to the OutputSection sh_link,
     // all InputSections in the OutputSection have the same dependency.
     if (auto *D = this->Sections.front()->getLinkOrderDep())
       this->Link = D->OutSec->SectionIndex;
   }
 
   uint32_t Type = this->Type;
+  if (Type == SHT_GROUP) {
+    finalizeShtGroup<ELFT>(this);
+    return;
+  }
+
   if (!Config->CopyRelocs || (Type != SHT_RELA && Type != SHT_REL))
     return;
 
   InputSection *First = Sections[0];
   if (isa<SyntheticSection>(First))
     return;
 
   this->Link = InX::SymTab->OutSec->SectionIndex;

 void OutputSectionFactory::addInputSec(InputSectionBase *IS,
                                        StringRef OutsecName,
                                        OutputSection *&Sec) {
   if (!IS->Live) {
     reportDiscarded(IS);
     return;
   }
 
-  uint64_t Flags = IS->Flags & ~(uint64_t)SHF_GROUP;
+  uint64_t Flags = IS->Flags;
+  if (!Config->Relocatable)
+    Flags &= ~(uint64_t)SHF_GROUP;
+
   if (Sec) {
     if (getIncompatibleFlags(Sec->Flags) != getIncompatibleFlags(IS->Flags))
       error("incompatible section flags for " + Sec->Name +
             "\n>>> " + toString(IS) + ": 0x" + utohexstr(IS->Flags) +
             "\n>>> output section " + Sec->Name + ": 0x" +
             utohexstr(Sec->Flags));
     if (Sec->Type != IS->Type) {
       if (canMergeToProgbits(Sec->Type) && canMergeToProgbits(IS->Type))

lld/trunk/test/ELF/relocatable-comdat.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: ld.lld -r %t.o %t.o -o %t
+# RUN: llvm-readobj -elf-section-groups -sections %t | FileCheck %s
+
+# CHECK:        Name: .text.bar
+# CHECK-NEXT:   Type: SHT_PROGBITS
+# CHECK-NEXT:   Flags [
+# CHECK-NEXT:     SHF_ALLOC
+# CHECK-NEXT:     SHF_EXECINSTR
+# CHECK-NEXT:     SHF_GROUP
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Address:
+# CHECK-NEXT:   Offset:
+# CHECK-NEXT:   Size: 8
+# CHECK:      Section {
+# CHECK-NEXT:   Index: 4
+# CHECK-NEXT:   Name: .text.foo
+# CHECK-NEXT:   Type: SHT_PROGBITS
+# CHECK-NEXT:   Flags [
+# CHECK-NEXT:     SHF_ALLOC
+# CHECK-NEXT:     SHF_EXECINSTR
+# CHECK-NEXT:     SHF_GROUP
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Address:
+# CHECK-NEXT:   Offset:
+# CHECK-NEXT:   Size: 4
+
+# CHECK:       Groups {
+# CHECK-NEXT:    Group {
+# CHECK-NEXT:      Name: .group
+# CHECK-NEXT:      Index: 2
+# CHECK-NEXT:      Type: COMDAT
+# CHECK-NEXT:      Signature: abc
+# CHECK-NEXT:      Section(s) in group [
+# CHECK-NEXT:        .text.bar
+# CHECK-NEXT:        .text.foo
+# CHECK-NEXT:      ]
+# CHECK-NEXT:    }
+# CHECK-NEXT:  }
+
+.section .text.bar,"axG",@progbits,abc,comdat
+.quad 42
+.section .text.foo,"axG",@progbits,abc,comdat
+.long 42