[LLD][Debuginfo] create DWARFContext only once for the same object file.

[avl]

Summary

LLD already has a mechanism for caching creation of DWARCContext:

llvm::call_once(initDwarfLine, [this]() { initializeDwarf(); });

Though it is not used in all places.

I need that patch for implementing "Remove obsolete debug info" feature
(D74169). But this caching mechanism is useful by itself, and I think it
would be good to use it without connection to "Remove obsolete debug info"
feature. So this patch changes inplace creation of DWARFContext with
its cached version.

Additionally, that patch makes that RecoverableErrors would not be fatal
errors. i.e., If some RecoverableErrors occurred while parsing Debug Info,
LLD would still return 0.

Depends on D74308

[grimar]

lld/ELF/InputFiles.cpp

269

Lets also see what other LLD people think, but it does not feel to me that LLD should get something like "recoverable error" concept.
We already have warnings to report something not critical, so why not just report a warning in this case?

[avl]

lld/ELF/InputFiles.cpp

269

The reason for reporting this as error is to have matching output with default DebugInfoDWARF output. i.e. when DebugInfoDWARF used in other context it would report this as "error". It could look a bit inconsistent when the same problem would be reported as "warning" from LLD context. If that is not important - then we could report it as warning and do not introduce "recoverable error" concept.

[MaskRay]

lld/ELF/InputFiles.cpp

269

I recall @ruiu mentioned that we don't want "recoverable error", but I can't find the thread.

[avl]

lld/ELF/InputFiles.cpp

269

Ok, I would change it to warning then.

[avl]

fix lit-test failure, use "warning" instead of "recoverable error"

[avl]

reported failures do not seem related to my patch.

[avl]

ping.

[jhenderson]

lld/ELF/SyntheticSections.cpp

2677

Seems like a not-really-related change? It's certainly not NFC.

2682

Ditto.

[avl]

lld/ELF/SyntheticSections.cpp

2677

During this review, it was suggested that debuginfo errors would be reported as warnings, when in lld context. Thus, I set warning handler for DWARFContext _and_ patched related places in lld.

originally it was NFC, but yes, now it is not NFC. Would change patch header, thanks.

[MaskRay]

This changes looks fine, though I have a question about a regex use.

lld/test/ELF/gdb-index-parse-fail.s

5

Why did the debug_info regex become complex now?

[ruiu]

lld/ELF/InputFiles.cpp

269

Err -> err

270

Warning -> warning

307

Calling call_once from multiple places doesn't seem to be a very good way to manage object instantiation. How about this?

• Define getDwarf() function that returns a DWARFCache object. That function initializes Dwarf member if it is not initialized yet
• Remove initializeDwarf (since now getDwarf initializes the Dwarf member)
• Remove getDWARFContext and rewrite getDWARFContext() with getDwarf()->getContext()

[avl]

lld/test/ELF/gdb-index-parse-fail.s

5

Without this patch the error was reported from lld/ELF/SyntheticSections.cpp :

if (Error e = cu->tryExtractDIEsIfNeeded(false)) {
  error(toString(sec) + ": " + toString(std::move(e)));
}

lld knows the section name and prints it in error message :

ld.lld: error: gdb-index-parse-fail.s.tmp1.o:(.debug_info): invalid reference to or invalid content in .debug_str_offsets[.dwo]: insufficient space for 32 bit header prefix

With this patch the error could be reported not from lld/ELF/SyntheticSections.cpp but from DWARFUnit.cpp, using error handler provided to DWARFContext in InputFiles.cpp:

[&](Error Err) { warn(getName() + ": " + toString(std::move(Err))); }

In that case section name is not printed(DWARFUnit.cpp does not print section name). So the error looks like this(without section name):

ld.lld: error: gdb-index-parse-fail.s.tmp1.o: invalid reference to or invalid content in .debug_str_offsets[.dwo]: insufficient space for 32 bit header prefix

Thus regex become more comlpex to accept both variants: with/without section name.

[avl]

lld/ELF/InputFiles.cpp

307

Just to make things clear - call_once would still be used. But it would be used in single place - inside getDwarf() function - right?

[jhenderson]

lld/ELF/SyntheticSections.cpp

2677

Okay. Seems like it should be a separate change though (i.e. one for the DWARFContext call_once stuff, and the other for error->warn).

[avl]

addressed comments.

[avl]

ping.

[ruiu]

lld/ELF/InputFiles.cpp

269

make is an lld's way of memory management, and I don't think you need to change this, so replace std::make_unique<DWARFCache> with make<DWARFCache>.

lld/ELF/InputFiles.h

288–289

I think you don't need to change this line

[avl]

lld/ELF/InputFiles.cpp

269

Right. But in this concrete case, there is a memory error because of data inter-dependency(if "make" is used).

The problem that DWARFCache::variableLoc references sections which are deleted first:

DWARFCache {

llvm::DenseMap<StringRef, VarLoc> variableLoc;
}

variableLoc has StringRef as a key which uses data from corresponding InputSection.

When destructor of llvm::DenseMap is called, it checks some keys.
But since data referenced by key are already deleted, there is a memory error.
The problem is that InputSections are already deallocated at this time.

freeArena() function starts from the first allocated object and goes till the last allocated object.
DWARFCache was allocated after InputSections.
Thus, when ~DWARFCache() is called - the sections, it references to, are already deleted.

To avoid that problem, I used std::unique_ptr for DWARFCache so that it would be deleted from the destructor of ObjFile. ObjFile is allocated before InputSections.
As a result, ~DWARFCache() is called first, and the problem does not happen.

[ruiu]

LGTM

[MaskRay]

Commit dcf6494abed7550fc84d6b59b33efc634a95c1fe does not have a subject line.

See my inline comment: I am a bit concerned if we cache DWARFContext for every input file for the proposed --gc-debuginfo feature.

For that affected internal target (which uses --gdb-index):

% /usr/bin/time -f "%P %M" /tmp/ReleaseA/bin/ld.lld @response.txt --no-gdb-index
546% 13761460

# Between D74773 and my fix
% /usr/bin/time -f "%P %M" /tmp/ReleaseA/bin/ld.lld @response.txt
1010% 22083732

# Before D74773 or after my fix
% /usr/bin/time -f "%P %M" /tmp/ReleaseA/bin/ld.lld @response.txt
1098% 18219472

% /usr/bin/time -f "%P %M" gold @response.txt
99% 16236236

lld/ELF/SyntheticSections.cpp

2825

We don't want to keep cached DWARFContext here.

--gdb-index can be memory heavy and caching DWARFContext for every input section can greatly increase memory usage.

I have fixed it (restored the previous behavior) with commit 0bb362c1649912d3a328dd01c700626d0a9f5a2c.

lld/ELF/InputFiles.h

public:
   bool someNoSplitStack = false;
 
   // Pointer to this input file's .llvm_addrsig section, if it has one.
   const Elf_Shdr *addrsigSec = nullptr;
 
   // SHT_LLVM_CALL_GRAPH_PROFILE table
   ArrayRef<Elf_CGProfile> cgProfile;
 
+  // Get cached DWARF information.
+  DWARFCache *getDwarf();
+
 private:
   void initializeSections(bool ignoreComdats);
   void initializeSymbols();
   void initializeJustSymbols();
-  void initializeDwarf();
+
   InputSectionBase *getRelocTarget(const Elf_Shdr &sec);
   InputSectionBase *createInputSection(const Elf_Shdr &sec);
   StringRef getSectionName(const Elf_Shdr &sec);
 
   bool shouldMerge(const Elf_Shdr &sec, StringRef name);
 
   // Each ELF symbol contains a section index which the symbol belongs to.
   // However, because the number of bits dedicated for that is limited, a
private:
 
   // .shstrtab contents.
   StringRef sectionStringTable;
 
   // Debugging information to retrieve source file and line for error
   // reporting. Linker may find reasonable number of errors in a
   // single object file, so we cache debugging information in order to
   // parse it only once for each object file we link.
-  DWARFCache *dwarf;
-  llvm::once_flag initDwarfLine;
+  std::unique_ptr<DWARFCache> dwarf;
+  llvm::once_flag initDwarf;
 };
 
 // LazyObjFile is analogous to ArchiveFile in the sense that
 // the file contains lazy symbols. The difference is that
 // LazyObjFile wraps a single file instead of multiple files.
 //
 // This class is used for --start-lib and --end-lib options which
 // instruct the linker to link object files between them with the

lld/ELF/InputFiles.cpp

case ELF32BEKind:
     return getSrcMsgAux(cast<ObjFile<ELF32BE>>(*this), sym, sec, offset);
   case ELF64LEKind:
     return getSrcMsgAux(cast<ObjFile<ELF64LE>>(*this), sym, sec, offset);
   case ELF64BEKind:
     return getSrcMsgAux(cast<ObjFile<ELF64BE>>(*this), sym, sec, offset);
   }
 }
 
-template <class ELFT> void ObjFile<ELFT>::initializeDwarf() {
-  dwarf = make<DWARFCache>(std::make_unique<DWARFContext>(
-      std::make_unique<LLDDwarfObj<ELFT>>(this)));
+template <class ELFT> DWARFCache *ObjFile<ELFT>::getDwarf() {
+  llvm::call_once(initDwarf, [this]() {
+    dwarf = std::make_unique<DWARFCache>(std::make_unique<DWARFContext>(
+        std::make_unique<LLDDwarfObj<ELFT>>(this), "",
+        [&](Error err) { warn(getName() + ": " + toString(std::move(err))); },
+        [&](Error warning) {
+          warn(getName() + ": " + toString(std::move(warning)));
+        }));
+  });
+
+  return dwarf.get();
 }
 
 // Returns the pair of file name and line number describing location of data
 // object (variable, array, etc) definition.
 template <class ELFT>
 Optional<std::pair<std::string, unsigned>>
 ObjFile<ELFT>::getVariableLoc(StringRef name) {
-  llvm::call_once(initDwarfLine, [this]() { initializeDwarf(); });
-
-  return dwarf->getVariableLoc(name);
+  return getDwarf()->getVariableLoc(name);
 }
 
 // Returns source line information for a given offset
 // using DWARF debug info.
 template <class ELFT>
 Optional<DILineInfo> ObjFile<ELFT>::getDILineInfo(InputSectionBase *s,
                                                   uint64_t offset) {
-  llvm::call_once(initDwarfLine, [this]() { initializeDwarf(); });
-
   // Detect SectionIndex for specified section.
   uint64_t sectionIndex = object::SectionedAddress::UndefSection;
   ArrayRef<InputSectionBase *> sections = s->file->getSections();
   for (uint64_t curIndex = 0; curIndex < sections.size(); ++curIndex) {
     if (s == sections[curIndex]) {
       sectionIndex = curIndex;
       break;
     }
   }
 
-  return dwarf->getDILineInfo(offset, sectionIndex);
+  return getDwarf()->getDILineInfo(offset, sectionIndex);
 }
 
 ELFFileBase::ELFFileBase(Kind k, MemoryBufferRef mb) : InputFile(k, mb) {
   ekind = getELFKind(mb, "");
 
   switch (ekind) {
   case ELF32LEKind:
     init<ELF32LE>();
     break;
   case ELF32BEKind:
     init<ELF32BE>();
     break;

lld/ELF/SyntheticSections.cpp

 #include "Config.h"
 #include "InputFiles.h"
 #include "LinkerScript.h"
 #include "OutputSections.h"
 #include "SymbolTable.h"
 #include "Symbols.h"
 #include "Target.h"
 #include "Writer.h"
+#include "lld/Common/DWARF.h"
 #include "lld/Common/ErrorHandler.h"
 #include "lld/Common/Memory.h"
 #include "lld/Common/Strings.h"
 #include "lld/Common/Threads.h"
 #include "lld/Common/Version.h"
 #include "llvm/ADT/SetOperations.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/BinaryFormat/Dwarf.h"

 
 static std::vector<GdbIndexSection::AddressEntry>
 readAddressAreas(DWARFContext &dwarf, InputSection *sec) {
   std::vector<GdbIndexSection::AddressEntry> ret;
 
   uint32_t cuIdx = 0;
   for (std::unique_ptr<DWARFUnit> &cu : dwarf.compile_units()) {
     if (Error e = cu->tryExtractDIEsIfNeeded(false)) {
       warn(toString(sec) + ": " + toString(std::move(e)));
       return {};
     }
     Expected<DWARFAddressRangesVector> ranges = cu->collectAddressRanges();
     if (!ranges) {
       warn(toString(sec) + ": " + toString(ranges.takeError()));
       return {};
     }
 
     ArrayRef<InputSectionBase *> sections = sec->file->getSections();
     for (DWARFAddressRange &r : *ranges) {
       if (r.SectionIndex == -1ULL)
         continue;
       // Range list with zero size has no effect.
template <class ELFT> GdbIndexSection *GdbIndexSection::create() {
   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) {
-    ObjFile<ELFT> *file = sections[i]->getFile<ELFT>();
-    DWARFContext dwarf(std::make_unique<LLDDwarfObj<ELFT>>(file));
+    DWARFContext *dwarf =
+        sections[i]->getFile<ELFT>()->getDwarf()->getContext();
 
     chunks[i].sec = sections[i];
-    chunks[i].compilationUnits = readCuList(dwarf);
-    chunks[i].addressAreas = readAddressAreas(dwarf, 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()),
+        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/include/lld/Common/DWARF.h

 class DWARFCache {
 public:
   DWARFCache(std::unique_ptr<llvm::DWARFContext> dwarf);
   llvm::Optional<llvm::DILineInfo> getDILineInfo(uint64_t offset,
                                                  uint64_t sectionIndex);
   llvm::Optional<std::pair<std::string, unsigned>>
   getVariableLoc(StringRef name);
 
+  llvm::DWARFContext *getContext() { return dwarf.get(); }
+
 private:
   std::unique_ptr<llvm::DWARFContext> dwarf;
   std::vector<const llvm::DWARFDebugLine::LineTable *> lineTables;
   struct VarLoc {
     const llvm::DWARFDebugLine::LineTable *lt;
     unsigned file;
     unsigned line;
   };
   llvm::DenseMap<StringRef, VarLoc> variableLoc;
 };
 
 } // namespace lld
 
 #endif

lld/test/ELF/gdb-index-invalid-ranges.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
 # RUN: ld.lld --gdb-index -e main %t.o -o /dev/null 2>&1 | FileCheck %s
 # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %p/Inputs/gdb-index-invalid-ranges.obj.s -o %t2.o
 # RUN: llvm-ar rc %t.a %t.o
 # RUN: ld.lld --gdb-index -e main %t2.o %t.a -o /dev/null 2>&1 | FileCheck --check-prefix=ARCHIVE %s
 
-# CHECK: ld.lld: warning: {{.*}}gdb-index-invalid-ranges.s.tmp.o:(.debug_info): decoding address ranges: invalid range list entry at offset 0x10
-# ARCHIVE: ld.lld: warning: {{.*}}gdb-index-invalid-ranges.s.tmp.a(gdb-index-invalid-ranges.s.tmp.o):(.debug_info): decoding address ranges: invalid range list entry at offset 0x10
+# CHECK: ld.lld: warning: {{.*}}gdb-index-invalid-ranges.s.tmp.o:{{(\(\.debug_info\):)?}} decoding address ranges: invalid range list entry at offset 0x10
+# ARCHIVE: ld.lld: warning: {{.*}}gdb-index-invalid-ranges.s.tmp.a(gdb-index-invalid-ranges.s.tmp.o):{{(\(\.debug_info\):)?}} decoding address ranges: invalid range list entry at offset 0x10
 
 .section .text.foo1,"ax",@progbits
 .globl f1
 .Lfunc_begin0:
 f1:
  nop
 .Lfunc_end0:
 

lld/test/ELF/gdb-index-parse-fail.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t1.o
 # RUN: ld.lld --gdb-index %t1.o -o /dev/null 2>&1 | FileCheck %s
 
-# CHECK: warning: {{.*}}:(.debug_info): invalid reference to or invalid content in .debug_str_offsets[.dwo]: insufficient space for 32 bit header prefix
+# CHECK: warning: {{.*}}:{{(\(\.debug_info\):)?}} invalid reference to or invalid content in .debug_str_offsets[.dwo]: insufficient space for 32 bit header prefix
 
 .section .debug_abbrev,"",@progbits
   .byte  1                           # Abbreviation Code
   .byte  17                          # DW_TAG_compile_unit
   .byte  0                           # DW_CHILDREN_no
   .byte  114                         # DW_AT_str_offsets_base
   .byte  23                          # DW_FORM_sec_offset
   .byte  0                           # EOM(1)