This is an archive of the discontinued LLVM Phabricator instance.

Differential D38491

[ELF] Decompress debug info sections early
ClosedPublic

Authored by smeenai on Oct 2 2017, 9:33 PM.

Details

Reviewers
ruiu
• rafael
grimar
Commits
rG50d7b36f5e14: [ELF] Decompress debug info sections early
rLLD314866: [ELF] Decompress debug info sections early
rL314866: [ELF] Decompress debug info sections early
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.

Diff Detail

Event Timeline

smeenai created this revision.Oct 2 2017, 9:33 PM
Herald added subscribers: aprantl, emaste. · View Herald TranscriptOct 2 2017, 9:33 PM
davide added a subscriber: davide.Oct 2 2017, 9:35 PM

what's the link time impact?

ELF/GdbIndex.cpp
32

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

smeenai added a comment.Oct 2 2017, 9:39 PM
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 added a comment.Oct 2 2017, 10:37 PM

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

grimar added inline comments.Oct 3 2017, 2:07 AM
ELF/GdbIndex.cpp
38

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

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 added inline comments.Oct 3 2017, 8:31 AM
ELF/GdbIndex.cpp
38

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

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 updated this revision to Diff 117537.Oct 3 2017, 8:48 AM

Simplify test

Harbormaster completed remote builds in B10776: Diff 117537.Oct 3 2017, 8:48 AM
grimar added inline comments.Oct 3 2017, 8:56 AM
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 added inline comments.Oct 3 2017, 8:57 AM
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 updated this revision to Diff 117538.Oct 3 2017, 9:03 AM

Ensure section is compressed

grimar edited edge metadata.Oct 3 2017, 9:16 AM

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

ruiu edited edge metadata.Oct 3 2017, 3:40 PM

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 updated this revision to Diff 117604.Oct 3 2017, 5:17 PM

Address @ruiu's comments

ruiu accepted this revision.Oct 3 2017, 5:18 PM

LGTM

ELF/GdbIndex.cpp
20

You can now remove this #include.

This revision is now accepted and ready to land.Oct 3 2017, 5:18 PM
Closed by commit rL314866: [ELF] Decompress debug info sections early (authored by smeenai). · Explain WhyOct 3 2017, 5:21 PM
This revision was automatically updated to reflect the committed changes.
smeenai added inline comments.Oct 3 2017, 5:22 PM
ELF/GdbIndex.cpp
20

Removed before committing. Thanks for the catch.

Revision Contents

PathSize
ELF/
3 lines
test/
ELF/
181 lines

Diff 117468

ELF/GdbIndex.cpp

Show All 11 Lines
// The format of the section is described at // The format of the section is described at
// https://sourceware.org/gdb/onlinedocs/gdb/Index-Section-Format.html. // https://sourceware.org/gdb/onlinedocs/gdb/Index-Section-Format.html.
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#include "GdbIndex.h" #include "GdbIndex.h"
#include "Memory.h" #include "Memory.h"
#include "llvm/DebugInfo/DWARF/DWARFDebugPubTable.h" #include "llvm/DebugInfo/DWARF/DWARFDebugPubTable.h"
#include "llvm/Object/Decompressor.h"
ruiuUnsubmitted
Not Done
ReplyInline Actions

You can now remove this #include.

ruiu: You can now remove this #include.
smeenaiAuthorUnsubmitted
Not Done
ReplyInline Actions

Removed before committing. Thanks for the catch.

smeenai: Removed before committing. Thanks for the catch.
#include "llvm/Object/ELFObjectFile.h" #include "llvm/Object/ELFObjectFile.h"
using namespace llvm; using namespace llvm;
using namespace llvm::object; using namespace llvm::object;
using namespace lld; using namespace lld;
using namespace lld::elf; using namespace lld::elf;
template <class ELFT> LLDDwarfObj<ELFT>::LLDDwarfObj(ObjFile<ELFT> *Obj) { template <class ELFT> LLDDwarfObj<ELFT>::LLDDwarfObj(ObjFile<ELFT> *Obj) {
for (InputSectionBase *Sec : Obj->getSections()) { for (InputSectionBase *Sec : Obj->getSections()) {
if (!Sec) if (!Sec)
continue; continue;
if (LLDDWARFSection *M = StringSwitch<LLDDWARFSection *>(Sec->Name) if (LLDDWARFSection *M = StringSwitch<LLDDWARFSection *>(Sec->Name)
davideUnsubmitted
Not Done
ReplyInline Actions

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

davide: Not your fault but `M` looks like a not so great variable name choice.
.Case(".debug_info", &InfoSection) .Case(".debug_info", &InfoSection)
.Case(".debug_ranges", &RangeSection) .Case(".debug_ranges", &RangeSection)
.Case(".debug_line", &LineSection) .Case(".debug_line", &LineSection)
.Default(nullptr)) { .Default(nullptr)) {
if (Decompressor::isCompressedELFSection(Sec->Flags, Sec->Name))
Sec->uncompress();
grimarUnsubmitted
Not Done
ReplyInline Actions

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.

grimar: We call `decompressAndMergeSections` at some point in linker, and at first I thought that…
smeenaiAuthorUnsubmitted
Not Done
ReplyInline Actions

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.

smeenai: You're right; this loses the parallelism. I think that's okay as long as there are no plans to…
M->Data = toStringRef(Sec->Data); M->Data = toStringRef(Sec->Data);
M->Sec = Sec; M->Sec = Sec;
continue; continue;
} }
if (Sec->Name == ".debug_abbrev") if (Sec->Name == ".debug_abbrev")
AbbrevSection = toStringRef(Sec->Data); AbbrevSection = toStringRef(Sec->Data);
else if (Sec->Name == ".debug_gnu_pubnames") else if (Sec->Name == ".debug_gnu_pubnames")
GnuPubNamesSection = toStringRef(Sec->Data); GnuPubNamesSection = toStringRef(Sec->Data);
▲ Show 20 Lines • Show All 49 Lines • Show Last 20 Lines

test/ELF/compressed-debug-conflict.test

  • This file was added.
# REQUIRES: x86
# RUN: yaml2obj %s -o %t.o
# RUN: not ld.lld %t.o %t.o -o %tout 2>&1 | FileCheck %s
# CHECK: error: duplicate symbol: main
# CHECK-NEXT: >>> defined at reduced.c:2 (/tmp/reduced.c:2)
# CHECK-NEXT: >>>
# CHECK-NEXT: >>> defined at reduced.c:2 (/tmp/reduced.c:2)
# CHECK-NEXT: >>>
--- !ELF
FileHeader:
Class: ELFCLASS32
Data: ELFDATA2LSB
Type: ET_REL
Machine: EM_386
Sections:
- Name: .text
Type: SHT_PROGBITS
Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
AddressAlign: 0x0000000000000010
Content: 5589E55031C0C745FC0000000083C4045DC3
- Name: .debug_str
Type: SHT_PROGBITS
Flags: [ SHF_MERGE, SHF_STRINGS ]
AddressAlign: 0x0000000000000001
Content: 636C616E672076657273696F6E20362E302E3020287472756E6B203331343730382920286C6C766D2F7472756E6B2033313437313529002F746D702F726564756365642E63002F746D70006D61696E00696E74006D79696E7400
- Name: .debug_abbrev
Type: SHT_PROGBITS
AddressAlign: 0x0000000000000001
Content: 011101250E1305030E10171B0EB44219110112060000022E00110112064018030E3A0B3B0B49133F1900000316004913030E3A0B3B0B0000042400030E3E0B0B0B000000
- Name: .debug_info
Type: SHT_PROGBITS
Flags: [ SHF_COMPRESSED ]
AddressAlign: 0x0000000000000001
Content: 010000004E00000001000000789CF362606060610001164610C9C3800E84809809C6600C05B11899AC8124B31B54051323D80456160600373F0173
- Name: .rel.debug_info
Type: SHT_REL
Link: .symtab
AddressAlign: 0x0000000000000004
Info: .debug_info
Relocations:
- Offset: 0x0000000000000006
Symbol: .debug_abbrev
Type: R_386_32
- Offset: 0x000000000000000C
Symbol: ''
Type: R_386_32
- Offset: 0x0000000000000012
Symbol: ''
Type: R_386_32
- Offset: 0x0000000000000016
Symbol: .debug_line
Type: R_386_32
- Offset: 0x000000000000001A
Symbol: ''
Type: R_386_32
- Offset: 0x000000000000001E
Symbol: .text
Type: R_386_32
- Offset: 0x0000000000000027
Symbol: .text
Type: R_386_32
- Offset: 0x0000000000000031
Symbol: ''
Type: R_386_32
- Offset: 0x0000000000000040
Symbol: ''
Type: R_386_32
- Offset: 0x0000000000000047
Symbol: ''
Type: R_386_32
- Name: .debug_ranges
Type: SHT_PROGBITS
AddressAlign: 0x0000000000000001
Content: ''
- Name: .debug_macinfo
grimarUnsubmitted
Not Done
ReplyInline Actions

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.

grimar: I am pretty sure you should not need this and few other sections, symbols and relocations.
smeenaiAuthorUnsubmitted
Not Done
ReplyInline Actions

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: I originally had the YAML because I wanted to reproduce the exact debug info that was causing…
Type: SHT_PROGBITS
AddressAlign: 0x0000000000000001
Content: '00'
- Name: .debug_pubnames
Type: SHT_PROGBITS
AddressAlign: 0x0000000000000001
Content: 170000000200000000004E000000260000006D61696E0000000000
- Name: .rel.debug_pubnames
Type: SHT_REL
Link: .symtab
AddressAlign: 0x0000000000000004
Info: .debug_pubnames
Relocations:
- Offset: 0x0000000000000006
Symbol: .debug_info
Type: R_386_32
- Name: .debug_pubtypes
Type: SHT_PROGBITS
AddressAlign: 0x0000000000000001
Content: 200000000200000000004E0000003B0000006D79696E740046000000696E740000000000
- Name: .rel.debug_pubtypes
Type: SHT_REL
Link: .symtab
AddressAlign: 0x0000000000000004
Info: .debug_pubtypes
Relocations:
- Offset: 0x0000000000000006
Symbol: .debug_info
Type: R_386_32
- Name: .comment
Type: SHT_PROGBITS
Flags: [ SHF_MERGE, SHF_STRINGS ]
AddressAlign: 0x0000000000000001
Content: 00636C616E672076657273696F6E20362E302E3020287472756E6B203331343730382920286C6C766D2F7472756E6B203331343731352900
- Name: .note.GNU-stack
Type: SHT_PROGBITS
AddressAlign: 0x0000000000000001
Content: ''
- Name: .debug_frame
Type: SHT_PROGBITS
AddressAlign: 0x0000000000000004
Content: 10000000FFFFFFFF04000400017C080C0404880114000000000000000000000012000000410E088502420D05
- Name: .rel.debug_frame
Type: SHT_REL
Link: .symtab
AddressAlign: 0x0000000000000004
Info: .debug_frame
Relocations:
- Offset: 0x0000000000000018
Symbol: .debug_frame
Type: R_386_32
- Offset: 0x000000000000001C
Symbol: .text
Type: R_386_32
- Name: .debug_line
Type: SHT_PROGBITS
Flags: [ SHF_COMPRESSED ]
AddressAlign: 0x0000000000000001
Content: 010000006D00000001000000789CCB64606060620802928C8CBFF97881242323880344FA25B9050C2042BF28B52035B1042BC5C050949A529A9C9AA2970CD20411D4CB009A0904AC6052985580EB04132BD0680001C8172D
- Name: .rel.debug_line
Type: SHT_REL
Link: .symtab
AddressAlign: 0x0000000000000004
Info: .debug_line
Relocations:
- Offset: 0x000000000000005F
Symbol: .text
Type: R_386_32
Symbols:
Local:
- Name: reduced.c
Type: STT_FILE
- Section: .debug_str
- Section: .debug_str
Value: 0x0000000000000037
- Section: .debug_str
Value: 0x0000000000000046
- Section: .debug_str
Value: 0x000000000000004B
- Section: .debug_str
Value: 0x0000000000000050
- Section: .debug_str
Value: 0x0000000000000054
- Name: .text
Type: STT_SECTION
Section: .text
- Name: .debug_abbrev
Type: STT_SECTION
Section: .debug_abbrev
- Name: .debug_info
Type: STT_SECTION
Section: .debug_info
- Name: .debug_frame
Type: STT_SECTION
Section: .debug_frame
- Name: .debug_line
Type: STT_SECTION
Section: .debug_line
Global:
- Name: main
Type: STT_FUNC
Section: .text
Size: 0x0000000000000012
...