[ELF] Decompress debug info sections early

[smeenai]

Summary

When reporting a symbol conflict, LLD parses the debug info to report
source location information. Sections have not been decompressed at this
point, so if an object file contains zlib compressed debug info, LLD
ends up passing this compressed debug info to the DWARF parser, which
causes debug info parsing failures and can trigger assertions in the
parser (as the test case demonstrates).

Decompress debug sections when constructing the LLDDwarfObj to avoid
this issue. This doesn't handle GNU-style compressed debug info sections
(.zdebug_*), which at present are simply ignored by LLDDwarfObj; those
can be done in a follow-up.

[davide]

what's the link time impact?

ELF/GdbIndex.cpp

32 ↗

(On Diff #117468)

Not your fault but M looks like a not so great variable name choice.

[smeenai]

In D38491#886697, @davide wrote:

what's the link time impact?

I didn't measure it, but I don't think this should make a noticeable difference, since if we don't decompress the sections here, they'll be decompressed later in decompressAndMergeSections anyway.

[smeenai]

Additionally, I believe this codepath is only used in error reporting scenarios.

[grimar]

ELF/GdbIndex.cpp

38 ↗

(On Diff #117468)

We call decompressAndMergeSections at some point in linker, and at first I thought that better would be to just call it earlier or at least to decompress sections early, but that is not so convinent to do. Assuming we still want to use threading for that it is more convinent to do when we have full list of input sections, like now.

And here uncompress is used for few sections in a single file and only when we reporting errors. That is probably looks fine for me.

test/ELF/compressed-debug-conflict.test

77 ↗

(On Diff #117468)

I am pretty sure you should not need this and few other sections, symbols and relocations. Testcase can be reduced.
Though I would suggest not to use yaml2obj here, but use asm input instead. Just like we usually do (see compressed-debug-input.s, gdb-index-empty.s for example). Doing that should simplify testcase a lot.

[smeenai]

ELF/GdbIndex.cpp

38 ↗

(On Diff #117468)

You're right; this loses the parallelism. I think that's okay as long as there are no plans to eventually use this outside of error-reporting scenarios.

test/ELF/compressed-debug-conflict.test

77 ↗

(On Diff #117468)

I originally had the YAML because I wanted to reproduce the exact debug info that was causing the assertion failure, but I realize now that you can do so via assembly debug info as well. Lemme play around with reducing this.

[smeenai]

Simplify test

[grimar]

test/ELF/compressed-debug-conflict.s

5 ↗

(On Diff #117537)

As far I remember, if compressed section size is greater then uncompressed, then
producer (llvm-mc) will not compress it.
I would add check here to verify that input object contains SHF_COMPRESSED section.

19 ↗

(On Diff #117537)

I guess that is why you duplicated "repeat" here: to increase debug info size to force section be compressed, right ?

[smeenai]

test/ELF/compressed-debug-conflict.s

5 ↗

(On Diff #117537)

That's a good idea.

19 ↗

(On Diff #117537)

Yup, since zlib will be able to compress the repeats well and get the compressed size below the uncompressed size.

[smeenai]

Ensure section is compressed

[grimar]

I have no futher comments, this LGTM, please wait for other reviewers approval. Thanks !

[ruiu]

I'd make two changes:

1. Decompressor::isCompressedELFSection(Sec->Flags, Sec->Name) is always called before calling uncompress(). It is better to move that check inside uncompress and rename it maybeUncompress.
2. With this change, it's not clear whether it is guaranteed that uncompress is called at most once. I'd make a change to uncompress so that it doesn't do anything on a second call. You can just check if UncompressBuf is empty.

[smeenai]

Address @ruiu's comments

[ruiu]

LGTM

ELF/GdbIndex.cpp

20 ↗

(On Diff #117604)

You can now remove this #include.

[smeenai]

ELF/GdbIndex.cpp

20 ↗

(On Diff #117604)

Removed before committing. Thanks for the catch.

lld/trunk/ELF/GdbIndex.cpp

template <class ELFT> LLDDwarfObj<ELFT>::LLDDwarfObj(ObjFile<ELFT> *Obj) {
   for (InputSectionBase *Sec : Obj->getSections()) {
     if (!Sec)
       continue;
     if (LLDDWARFSection *M = StringSwitch<LLDDWARFSection *>(Sec->Name)
                                  .Case(".debug_info", &InfoSection)
                                  .Case(".debug_ranges", &RangeSection)
                                  .Case(".debug_line", &LineSection)
                                  .Default(nullptr)) {
+      Sec->maybeUncompress();
       M->Data = toStringRef(Sec->Data);
       M->Sec = Sec;
       continue;
     }
     if (Sec->Name == ".debug_abbrev")
       AbbrevSection = toStringRef(Sec->Data);
     else if (Sec->Name == ".debug_gnu_pubnames")
       GnuPubNamesSection = toStringRef(Sec->Data);

lld/trunk/ELF/InputSection.h

public:
   template <class ELFT> ObjFile<ELFT> *getFile() const;
 
   template <class ELFT> llvm::object::ELFFile<ELFT> getObj() const {
     return getFile<ELFT>()->getObj();
   }
 
   InputSection *getLinkOrderDep() const;
 
-  void uncompress();
+  void maybeUncompress();
 
   // Returns a source location string. Used to construct an error message.
   template <class ELFT> std::string getLocation(uint64_t Offset);
   template <class ELFT> std::string getSrcMsg(uint64_t Offset);
   template <class ELFT> std::string getObjMsg(uint64_t Offset);
 
   template <class ELFT> void relocate(uint8_t *Buf, uint8_t *BufEnd);
   void relocateAlloc(uint8_t *Buf, uint8_t *BufEnd);

lld/trunk/ELF/InputSection.cpp

else if (auto *MS = dyn_cast<MergeInputSection>(this))
     Sec = MS->getParent();
   else if (auto *EH = dyn_cast<EhInputSection>(this))
     Sec = EH->getParent();
   else
     return cast<OutputSection>(this);
   return Sec ? Sec->getParent() : nullptr;
 }
 
-// Uncompress section contents. Note that this function is called
-// from parallelForEach, so it must be thread-safe.
-void InputSectionBase::uncompress() {
+// Uncompress section contents if required. Note that this function
+// is called from parallelForEach, so it must be thread-safe.
+void InputSectionBase::maybeUncompress() {
+  if (UncompressBuf)
+    return;
+
+  if (!Decompressor::isCompressedELFSection(Flags, Name))
+    return;
+
   Decompressor Dec = check(Decompressor::create(Name, toStringRef(Data),
                                                 Config->IsLE, Config->Is64));
 
   size_t Size = Dec.getDecompressedSize();
   UncompressBuf.reset(new char[Size]());
   if (Error E = Dec.decompress({UncompressBuf.get(), Size}))
     fatal(toString(this) +
           ": decompress failed: " + llvm::toString(std::move(E)));

lld/trunk/ELF/SyntheticSections.cpp

 // finalizes each synthetic section in order to compute an output offset for
 // each piece of each input section.
 void elf::decompressAndMergeSections() {
   // splitIntoPieces needs to be called on each MergeInputSection before calling
   // finalizeContents(). Do that first.
   parallelForEach(InputSections, [](InputSectionBase *S) {
     if (!S->Live)
       return;
-    if (Decompressor::isCompressedELFSection(S->Flags, S->Name))
-      S->uncompress();
+    S->maybeUncompress();
     if (auto *MS = dyn_cast<MergeInputSection>(S))
       MS->splitIntoPieces();
   });
 
   std::vector<MergeSyntheticSection *> MergeSections;
   for (InputSectionBase *&S : InputSections) {
     MergeInputSection *MS = dyn_cast<MergeInputSection>(S);
     if (!MS)

lld/trunk/test/ELF/compressed-debug-conflict.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple i686-linux-gnu -compress-debug-sections=zlib %s -o %t.o
+# RUN: llvm-readobj -sections %t.o | FileCheck -check-prefix=OBJ %s
+# RUN: not ld.lld %t.o %t.o -o %tout 2>&1 | FileCheck -check-prefix=ERROR %s
+
+# OBJ:      Sections [
+# OBJ:        Section {
+# OBJ:          Index: 3
+# OBJ-NEXT:     Name: .debug_line (16)
+# OBJ-NEXT:     Type: SHT_PROGBITS (0x1)
+# OBJ-NEXT:     Flags [ (0x800)
+# OBJ-NEXT:       SHF_COMPRESSED (0x800)
+# OBJ-NEXT:     ]
+
+# ERROR:      error: duplicate symbol: main
+# ERROR-NEXT: >>> defined at reduced.c:2 (/tmp/reduced.c:2)
+# ERROR-NEXT: >>>
+# ERROR-NEXT: >>> defined at reduced.c:2 (/tmp/reduced.c:2)
+# ERROR-NEXT: >>>
+
+  .text
+  .file "reduced.c"
+  .globl  main
+main:
+  .file 1 "/tmp" "reduced.c"
+  .loc  1 2 0
+  xorl  %eax, %eax
+  retl
+  .file 2 "/tmp/repeat/repeat/repeat/repeat" "repeat.h"