[ELF] --gdb-index: skip SHF_GROUP .debug_info

[MaskRay]

Summary

-gdwarf-5 -fdebug-types-section may produce multiple .debug_info sections. All
except one are type units (.debug_types before DWARF v5). When constructing
.gdb_index, we should ignore these type units. We use a simple heuristic: the
compile unit does not have the SHF_GROUP flag. (This needs to be revisited if
people place compile unit .debug_info in COMDAT groups.)

This issue manifests as a data race: because an object file may have multiple
.debug_info sections, we may concurrently construct LLDDwarfObj for the same
file in multiple threads. The threads may access InputSectionBase::data()
concurrently on the same input section. InputSectionBase::data() does a lazy
uncompress() and rewrites the member variable rawData. A thread running zlib
inflate() (transitively called by uncompress()) on a buffer with rawData
tampered by another thread may fail with uncompress failed: zlib error: Z_DATA_ERROR.

Even if no data race occurred in an optimistic run, if there are N .debug_info,
one CU entry and its address ranges will be replicated N times. The result
.gdb_index can be much larger than a correct one.

The new test gdb-index-dwarf5-type-unit.s actually has two compile units. This
cannot be produced with regular approaches (it can be produced with -r
--unique). This is used to demonstrate that the .gdb_index construction code
only considers the last non-SHF_GROUP .debug_info

[MaskRay]

Ready

[MaskRay]

Add REQUIRES: zlib

Reword a comment.

[grimar]

I can't say about the heuristic used and how well it works,
but have a few comments about the code.

lld/ELF/DWARF.cpp

34

So, you need this to read the flags from the sh_flags field of a section header in the initial object.
And you can't just use sec->flags, because we drop the SHF_GROUP flag much earlier.

I do not know a simpler way to access initial flags. Though probably it worth adding an assert to check that the number
of sections in obj->getObj().sections() == the number of them in obj->getSections() and a comment
about SHT_GROUP flag? That way we will not forget to simplify this loop back later when will stop relying on SHF_GROUP flag.

lld/ELF/SyntheticSections.cpp

2866

It doesn't seem the getDebugInfoSections helper and its name makes much sence now with this change?

1. It is not clear what it returns anymore: it might return either CU or TU .debug_infos.
2. You are not really using the result, since dobj.getInfoSection() is called instead to get debug info sections.
3. It is just needed now to find the number of files with .debug_info sections.

So I think it should probably just be inlined and/or rewritten to collect a file set mentioned probably?

[MaskRay]

address comments

[MaskRay]

lld/test/ELF/gdb-index-dwarf5-type-unit.s

52

@dblaikie Does this test achieve a good minimality/robustness/usefulness in your opinion?

[dblaikie]

lld/test/ELF/gdb-index-dwarf5-type-unit.s

52

I think so?

If I'm reading it correctly it's a type unit in a comdat, a compile unit, another compile unit in a distinct fragment of debug_info, and then another type unit in a comdat?

The existence of two compilation units in separate sections with the same name isn't a use case I know of any producer producing today - but if it helps test out the generality of this functionality, it sounds OK to me.

[MaskRay]

lld/test/ELF/gdb-index-dwarf5-type-unit.s

52

Thanks for confirmation.

The existence of two compilation units in separate sections with the same name isn't a use case I know of any producer producing today - but if it helps test out the generality of this functionality, it sounds OK to me.

This is super rare. ld -r --unique can create the case. I just want to make the behavior clearer/concise without adding another test.

[dblaikie]

lld/test/ELF/gdb-index-dwarf5-type-unit.s

52

Ah, sounds good!

[grimar]

lld/ELF/SyntheticSections.cpp

2890

Previously, parallelForEachN loop assumed that there is a one .debug_info section in an object.
Now it is not true and parallelizing for sections by itself (instead of doing it for files) doesn't look optimal.

Also, you do not need to build a list sections, because you are not using them (you call dobj.getInfoSection(); anyways), so
you only need a list of files to work with.

With that we can have a simpler code:

  SetVector<ObjFile<ELFT> *> files;
  for (InputSectionBase *s : inputSections) {
    InputSection *isec = dyn_cast<InputSection>(s);
    if (!isec)
      continue;
    // .debug_gnu_pub{names,types} are useless in executables.
    // They are present in input object files solely for creating
    // a .gdb_index. So we can remove them from the output.
    if (s->name == ".debug_gnu_pubnames" || s->name == ".debug_gnu_pubtypes")
      s->markDead();
    else if (isec->name == ".debug_info")
      files.insert(isec->getFile<ELFT>());
  }

  std::vector<GdbChunk> chunks(files.size());
  std::vector<std::vector<NameAttrEntry>> nameAttrs(files.size());

  parallelForEachN(0, files.size(), [&](size_t i) {
    // To keep memory usage low, we don't want to keep cached DWARFContext, so
    // avoid getDwarf() here.
    DWARFContext dwarf(std::make_unique<LLDDwarfObj<ELFT>>(files[i]));
    auto &dobj = static_cast<const LLDDwarfObj<ELFT> &>(dwarf.getDWARFObj());
...

[MaskRay]

address comments

[grimar]

LGTM I think.

[MaskRay]

@hans This patch can be cleanly cherry-picked to release/11.x It addresses a pain for DWARF v5 -fdebug-types-section users.

lld/ELF/SyntheticSections.cpp

2890

Forgot to push this comment:

Thanks! I'll make a small change: SetVector<InputFile *>, just so that we don't need to instantiate nearly identical code for 4 instantiations of ELFT.

[hans]

In D85579#2237351, @MaskRay wrote:

@hans This patch can be cleanly cherry-picked to release/11.x It addresses a pain for DWARF v5 -fdebug-types-section users.

Pushed as 21d01a67c9613932053dd89c9957782f86e0c93f. Please let me know if there are any follow-ups.

lld/ELF/DWARF.cpp

 //===- DWARF.cpp ----------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // The -gdb-index option instructs the linker to emit a .gdb_index section.

 #include "llvm/DebugInfo/DWARF/DWARFDebugPubTable.h"
 #include "llvm/Object/ELFObjectFile.h"
 
 using namespace llvm;
 using namespace llvm::object;
 using namespace lld;
 using namespace lld::elf;
 
-template <class ELFT> LLDDwarfObj<ELFT>::LLDDwarfObj(ObjFile<ELFT> *obj) {
-  for (InputSectionBase *sec : obj->getSections()) {
+template <class ELFT>
+LLDDwarfObj<ELFT>::LLDDwarfObj(ObjFile<ELFT> *obj) {
+  using Elf_Shdr = typename ELFT::Shdr;
+  ArrayRef<Elf_Shdr> objSections = CHECK(obj->getObj().sections(), obj);
+  for (auto it : llvm::enumerate(obj->getSections())) {
+    InputSectionBase *sec = it.value();
     if (!sec)
       continue;
 
+    // In DWARF v5, a .debug_info in a COMDAT group may represent a type unit.
+    // Don't bother calling data() (which may uncompress the content) on it.
     if (LLDDWARFSection *m =
             StringSwitch<LLDDWARFSection *>(sec->name)
                 .Case(".debug_addr", &addrSection)
                 .Case(".debug_gnu_pubnames", &gnuPubnamesSection)
                 .Case(".debug_gnu_pubtypes", &gnuPubtypesSection)
-                .Case(".debug_info", &infoSection)
                 .Case(".debug_loclists", &loclistsSection)
                 .Case(".debug_ranges", &rangesSection)
                 .Case(".debug_rnglists", &rnglistsSection)
                 .Case(".debug_str_offsets", &strOffsetsSection)
                 .Case(".debug_line", &lineSection)
                 .Default(nullptr)) {
       m->Data = toStringRef(sec->data());
       m->sec = sec;
       continue;
     }
 
     if (sec->name == ".debug_abbrev")
       abbrevSection = toStringRef(sec->data());
     else if (sec->name == ".debug_str")
       strSection = toStringRef(sec->data());
     else if (sec->name == ".debug_line_str")
       lineStrSection = toStringRef(sec->data());
+    else if (sec->name == ".debug_info" &&
+             !(objSections[it.index()].sh_flags & ELF::SHF_GROUP)) {
+      // In DWARF v5, -fdebug-types-section places type units in .debug_info
+      // sections in COMDAT groups. They are not compile units and thus should
+      // be ignored. If we place compile units in COMDAT groups in the future,
+      // we will need to perform a lightweight parsing.
+      infoSection.Data = toStringRef(sec->data());
+      infoSection.sec = sec;
+    }
   }
 }
 
 namespace {
 template <class RelTy> struct LLDRelocationResolver {
   // In the ELF ABIs, S sepresents the value of the symbol in the relocation
   // entry. For Rela, the addend is stored as part of the relocation entry.
   static uint64_t resolve(object::RelocationRef ref, uint64_t s,

lld/ELF/SyntheticSections.cpp

void GdbIndexSection::initOutputSize() {
   if (!symbols.empty()) {
     GdbSymbol &sym = symbols.back();
     size += sym.nameOff + sym.name.size() + 1;
   }
 }
 
 static std::vector<InputSection *> getDebugInfoSections() {
   std::vector<InputSection *> ret;
+  // See the comment in LLDDwarfObj<ELFT>::LLDDwarfObj. NOTE: isec->data() may
+  // uncompress the full content, which should be parallelized.
+  // FIXME This does not actually work because we drop SHF_FLAG very early.
   for (InputSectionBase *s : inputSections)
     if (InputSection *isec = dyn_cast<InputSection>(s))
-      if (isec->name == ".debug_info")
+      if (isec->name == ".debug_info" && !(isec->flags & SHF_GROUP))
         ret.push_back(isec);
   return ret;
 }
 
 static std::vector<GdbIndexSection::CuEntry> readCuList(DWARFContext &dwarf) {
   std::vector<GdbIndexSection::CuEntry> ret;
   for (std::unique_ptr<DWARFUnit> &cu : dwarf.compile_units())
     ret.push_back({cu->getOffset(), cu->getLength() + 4});
for (GdbSymbol &sym : ret) {
     off += sym.name.size() + 1;
   }
 
   return ret;
 }
 
 // Returns a newly-created .gdb_index section.
 template <class ELFT> GdbIndexSection *GdbIndexSection::create() {
   std::vector<InputSection *> sections = getDebugInfoSections();
 
   // .debug_gnu_pub{names,types} are useless in executables.
   // They are present in input object files solely for creating
   // a .gdb_index. So we can remove them from the output.
   for (InputSectionBase *s : inputSections)
     if (s->name == ".debug_gnu_pubnames" || s->name == ".debug_gnu_pubtypes")
       s->markDead();
 
   std::vector<GdbChunk> chunks(sections.size());
   std::vector<std::vector<NameAttrEntry>> nameAttrs(sections.size());
 
   parallelForEachN(0, sections.size(), [&](size_t i) {
     // To keep memory usage low, we don't want to keep cached DWARFContext, so
     // avoid getDwarf() here.
     ObjFile<ELFT> *file = sections[i]->getFile<ELFT>();
     DWARFContext dwarf(std::make_unique<LLDDwarfObj<ELFT>>(file));
 
     chunks[i].sec = sections[i];
     chunks[i].compilationUnits = readCuList(dwarf);
     chunks[i].addressAreas = readAddressAreas(dwarf, sections[i]);
     nameAttrs[i] = readPubNamesAndTypes<ELFT>(
         static_cast<const LLDDwarfObj<ELFT> &>(dwarf.getDWARFObj()),
         chunks[i].compilationUnits);
   });
 
   auto *ret = make<GdbIndexSection>();
   ret->chunks = std::move(chunks);
   ret->symbols = createSymbols(nameAttrs, ret->chunks);
   ret->initOutputSize();
   return ret;
 }
 

lld/test/ELF/gdb-index-dwarf5-type-unit.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: ld.lld --gdb-index -Ttext=0x1000 %t.o -o %t
+# RUN: llvm-dwarfdump --gdb-index %t | FileCheck %s
+
+# CHECK:      CU list offset = 0x18, has 1 entries:
+# CHECK-NEXT:   0: Offset = 0x0, Length = 0x19
+
+# CHECK:      Address area offset = 0x28, has 1 entries:
+# CHECK-NEXT:   Low/High address = [0x1000, 0x1001) (Size: 0x1), CU id = 0
+
+.globl main
+main:
+.Lfunc_begin0:
+  retq
+.Lfunc_end0:
+
+.section  .debug_abbrev,"",@progbits
+  .byte  1                         # Abbreviation Code
+  .byte  65                        # DW_TAG_type_unit
+  .byte  0                         # DW_CHILDREN_no
+  .byte  0                         # EOM(1)
+  .byte  0                         # EOM(2)
+
+  .byte  2                         # Abbreviation Code
+  .byte  17                        # DW_TAG_compile_unit
+  .byte  0                         # DW_CHILDREN_no
+  .byte  17                        # DW_AT_low_pc
+  .byte  1                         # DW_FORM_addr
+  .byte  18                        # DW_AT_high_pc
+  .byte  6                         # DW_FORM_data4
+  .byte  0                         # EOM(1)
+  .byte  0                         # EOM(2)
+
+  .byte  0                         # EOM(3)
+
+.macro TYPE_UNIT id signature
+.section  .debug_info,"G",@progbits,\signature
+  .long  .Ldebug_info_end\id-.Ldebug_info_start\id # Length of Unit
+.Ldebug_info_start\id:
+  .short 5                         # DWARF version number
+  .byte  2                         # DWARF Unit Type
+  .byte  8                         # Address Size
+  .long  .debug_abbrev             # Offset Into Abbrev. Section
+  .quad  \signature                # Type Signature
+  .long  .Ldebug_info_end\id       # Type DIE Offset
+  .byte  1                         # Abbrev [1] DW_TAG_type_unit
+.Ldebug_info_end\id:
+.endm
+
+TYPE_UNIT 0, 123
+
+.section  .debug_info,"",@progbits
+.Lcu_begin0:
+  .long .Lcu_end0-.Lcu_begin0-4    # Length of Unit
+  .short 5                         # DWARF version number
+  .byte  1                         # DWARF Unit Type
+  .byte  8                         # Address Size
+  .long  .debug_abbrev             # Offset Into Abbrev. Section
+  .byte  2                         # Abbrev [2] DW_TAG_compile_unit
+  .quad  .Lfunc_begin0             # DW_AT_low_pc
+  .long  .Lfunc_end0-.Lfunc_begin0 # DW_AT_high_pc
+.Lcu_end0:
+
+#.section  .debug_info,"G",@progbits,1234
+#  .long  .Ldebug_info_end0-.Ldebug_info_start0 # Length of Unit
+#.Ldebug_info_start0:
+#  .short 5                         # DWARF version number
+#  .byte  2                         # DWARF Unit Type
+#  .byte  8                         # Address Size
+#  .long  .debug_abbrev             # Offset Into Abbrev. Section
+#  .quad  1234                      # Type Signature
+#  .long  .Ldebug_info_end0         # Type DIE Offset
+#  .byte  1                         # Abbrev [1] DW_TAG_type_unit
+#.Ldebug_info_end0:
+
+TYPE_UNIT 1, 456