[ELF] Refine section group --gc-sections rules to not discard .debug_types

[MaskRay]

Summary

clang/gcc -fdebug-type-sections places .debug_types and
.rela.debug_types in a section group, with a signature symbol which
represents the type signature. The section group is for deduplication
purposes.

After D70146, we will discard such section groups. Refine the rule so
that we will retain the group if no member has the SHF_ALLOC flag.

GNU ld has a similar rule to retain the group if all members have the
SEC_DEBUGGING flag. We try to be more general for future-proof purposes:
if other non-SHF_ALLOC sections have deduplication needs, they may be
placed in a section group.

[MaskRay]

GNU ld's logic is here https://sourceware.org/git/?p=binutils-gdb.git;a=blobdiff;f=bfd/elflink.c;h=73d2360602c5d6032aca28df46ebd43aa3703aac;hp=66cf1ed89003ade7761f71dcb7feb4f6457cf35f;hb=3c758495736df25d141a4ea13c7aba24f5bb9753;hpb=bba33ab1e0f7d2ebd8f8435f92ed12e2a3c558a4

The patch tries to be a bit more general.

[grimar]

The logic looks OK to me. A suggestion about the code is inline.

lld/ELF/InputFiles.cpp

653–654

(while you are here)
secion -> section

663

I'd suggest adding a helper, because initializeSections becoming too large probably.
Something like:

template <typename ELFT>
static std::vector<InputSectionBase *>
getGroupSections(ArrayRef<typename ELFT::Word> entries,
                 ArrayRef<InputSectionBase *> sections) {
  std::vector<InputSectionBase *> ret;
  for (uint32_t i : entries)
    if (i < sections.size())
      if (InputSectionBase *s = sections[i])
        if (s != &InputSection::discarded)
          ret.push_back(s);
  return ret;
}

Then you can simplify a bit:

for (ArrayRef<Elf_Word> entries : selectedGroups) {
  bool hasAlloc = false;
  std::vector<InputSectionBase *> sections =
      getGroupSections<ELFT>(entries.slice(1), this->sections);
  for (InputSectionBase *s : sections) {
    if ((s->flags & SHF_ALLOC) == 0)
      continue;
    hasAlloc = true;
    break;
  }

  // If no member has the SHF_ALLOC flag, retain the whole group. This rule
  // retains .debug_types and .rela.debug_types.
  if (!hasAlloc)
    continue;

  InputSectionBase *head;
  InputSectionBase *prev = nullptr;
  for (InputSectionBase *s : sections) {
    if (prev)
      prev->nextInSectionGroup = s;
    else
      head = s;
    prev = s;
  }
  if (prev)
    prev->nextInSectionGroup = head;
}

I'd probably consider moving out the whole code related to groups into it's own method/helper though.
(not in this patch)

[MaskRay]

Split off section group code

lld/ELF/InputFiles.cpp

663

I will move out the whole section group code. The split-off function is not too long, so we don't need the helper function (getGroupSections).

[MaskRay]

Delete a redundant comment

[peter.smith]

Assuming George is happy with them, changes look good to me,

[MaskRay]

@grimar Does this look good to you? Rui is OOO. (This is somewhat important for our internal use cases but I want an official approval before committing it, to make sure there is no open questions.)

[grimar]

LGTM.

lld/ELF/InputFiles.cpp

511

I'd probably simplify:

if (InputSectionBase *s = sections[index])
  if (s != &InputSection::discarded && s->flags & SHF_ALLOC)
    hasAlloc = true;

Up to you.

[MaskRay]

lld/ELF/InputFiles.cpp

511

Good idea. Applying.

[MaskRay]

Simplify code

[ruiu]

lld/ELF/InputFiles.cpp

500

Could you add a function comment to explain what the section group is and what this function is supposed to do?

[MaskRay]

lld/ELF/InputFiles.cpp

500

How about

--- i/lld/ELF/InputFiles.cpp
+++ w/lld/ELF/InputFiles.cpp
@@ -498,4 +498,5 @@ static void addDependentLibrary(StringRef specifier, const InputFile *f) {
 }
 
+// Check if members of the section group can be discarded.
 template <class ELFT>
 static void handleSectionGroup(ArrayRef<InputSectionBase *> sections,

? The most useful comment is below

// If any member has the SHF_ALLOC flag, the whole group is subject to garbage
// collection. See the comment in markLive(). This rule retains .debug_types
// and .rela.debug_types.

So the function comment probably should not repeat that.

[ruiu]

lld/ELF/InputFiles.cpp

500

It's more like

Record the membership of a section group so that in the garbage collection pass,
section group members are kept or discarded as a unit.

?

lld/ELF/InputFiles.cpp

static void addDependentLibrary(StringRef specifier, const InputFile *f) {
   else if (Optional<std::string> s = searchLibraryBaseName(specifier))
     driver->addFile(*s, /*withLOption=*/true);
   else
     error(toString(f) +
           ": unable to find library from dependent library specifier: " +
           specifier);
 }
 
 template <class ELFT>
+static void handleSectionGroup(ArrayRef<InputSectionBase *> sections,
+                               ArrayRef<typename ELFT::Word> entries) {
+  bool hasAlloc = false;
+  for (uint32_t index : entries.slice(1)) {
+    if (index >= sections.size())
+      return;
+    if (InputSectionBase *s = sections[index])
+      if (s != &InputSection::discarded && s->flags & SHF_ALLOC)
+        hasAlloc = true;
+  }
+
+  // If any member has the SHF_ALLOC flag, the whole group is subject to garbage
+  // collection. See the comment in markLive(). This rule retains .debug_types
+  // and .rela.debug_types.
+  if (!hasAlloc)
+    return;
+
+  // Connect the members in a circular doubly-linked list via
+  // nextInSectionGroup.
+  InputSectionBase *head;
+  InputSectionBase *prev = nullptr;
+  for (uint32_t index : entries.slice(1)) {
+    InputSectionBase *s = sections[index];
+    if (!s || s == &InputSection::discarded)
+      continue;
+    if (prev)
+      prev->nextInSectionGroup = s;
+    else
+      head = s;
+    prev = s;
+  }
+  if (prev)
+    prev->nextInSectionGroup = head;
+}
+
+template <class ELFT>
 void ObjFile<ELFT>::initializeSections(bool ignoreComdats) {
   const ELFFile<ELFT> &obj = this->getObj();
 
   ArrayRef<Elf_Shdr> objSections = CHECK(obj.sections(), this);
   uint64_t size = objSections.size();
   this->sections.resize(size);
   this->sectionStringTable =
       CHECK(obj.getSectionStringTable(objSections), this);
for (size_t i = 0, e = objSections.size(); i < e; ++i) {
 
     InputSection *isec = cast<InputSection>(this->sections[i]);
     linkSec->dependentSections.push_back(isec);
     if (!isa<InputSection>(linkSec))
       error("a section " + isec->name +
             " with SHF_LINK_ORDER should not refer a non-regular section: " +
             toString(linkSec));
   }
 
-  // For each secion group, connect its members in a circular doubly-linked list
-  // via nextInSectionGroup. See the comment in markLive().
-  for (ArrayRef<Elf_Word> entries : selectedGroups) {
-    InputSectionBase *head;
-    InputSectionBase *prev = nullptr;
-    for (uint32_t secIndex : entries.slice(1)) {
-      if (secIndex >= this->sections.size())
-        continue;
-      InputSectionBase *s = this->sections[secIndex];
-      if (!s || s == &InputSection::discarded)
-        continue;
-      if (prev)
-        prev->nextInSectionGroup = s;
-      else
-        head = s;
-      prev = s;
-    }
-    if (prev)
-      prev->nextInSectionGroup = head;
-  }
+  for (ArrayRef<Elf_Word> entries : selectedGroups)
+    handleSectionGroup<ELFT>(this->sections, entries);
 }
 
 // For ARM only, to set the EF_ARM_ABI_FLOAT_SOFT or EF_ARM_ABI_FLOAT_HARD
 // flag in the ELF Header we need to look at Tag_ABI_VFP_args to find out how
 // the input objects have been compiled.
 static void updateARMVFPArgs(const ARMAttributeParser &attributes,
                              const InputFile *f) {
   if (!attributes.hasAttribute(ARMBuildAttrs::ABI_VFP_args))
     // If an ABI tag isn't present then it is implicitly given the value of 0
     // which maps to ARMBuildAttrs::BaseAAPCS. However many assembler files,
     // including some in glibc that don't use FP args (and should have value 3)
     // don't have the attribute so we do not consider an implicit value of 0
     // as a clash.
     return;
 
   unsigned vfpArgs = attributes.getAttributeValue(ARMBuildAttrs::ABI_VFP_args);

lld/test/ELF/gc-sections-group-debug.s

+# REQUIRES: x86
+## Check that group members are retained, if no member has the SHF_ALLOC flag.
+## This rule retains .debug_types and .rela.debug_types emitted by clang/gcc.
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: ld.lld --gc-sections %t.o -o %t
+# RUN: llvm-readobj -S %t | FileCheck %s
+
+# CHECK: Name: .debug_types
+
+.section .debug_types,"G",@progbits,abcd,comdat
+.quad .debug_types

lld/test/ELF/gc-sections-group.s

 # REQUIRES: x86
 ## Check that group members are retained or discarded as a unit, and
-## non-SHF_ALLOC sections in a group are subject to garbage collection.
-## This is compatible with GNU ld.
+## non-SHF_ALLOC sections in a group are subject to garbage collection,
+## if at least one member has the SHF_ALLOC flag.
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
 # RUN: ld.lld --gc-sections %t.o -o %t.dead
 # RUN: llvm-readobj -S %t.dead | FileCheck %s --check-prefix=CHECK-DEAD
 
 ## .mynote.bar is retained because it is not in a group.
 # CHECK-DEAD-NOT: Name: .myanote.foo
 # CHECK-DEAD-NOT: Name: .mytext.foo