This is an archive of the discontinued LLVM Phabricator instance.

[ELF] --gdb-index: support .debug_loclists
ClosedPublic

Authored by MaskRay on Apr 28 2020, 7:34 PM.

Details

Summary

--gdb-index currently crashes when reading a translation unit with
DWARF v5 .debug_loclists . Call stack:

SyntheticSections.cpp GdbIndexSection::create
SyntheticSections.cpp readAddressAreas
DWARFUnit.cpp DWARFUnit::tryExtractDIEsIfNeeded
DWARFListTable.cpp DWARFListTableHeader::extract
...
DWARFDataExtractor.cpp DWARFDataExtractor::getRelocatedValue
lld/ELF/DWARF.cpp LLDDwarfObj<ELFT>::find (sec.sec is nullptr)
...

This patch adds support for .debug_loclists to make DWARFUnit::tryExtractDIEsIfNeeded happy.
Building --gdb-index does not need .debug_loclists

Diff Detail

Event Timeline

MaskRay created this revision.Apr 28 2020, 7:34 PM

I'm surprised that generating gdb-index requires reading any debug locations - is there any chance of refactoring the APIs to make this not needed at all? (I mean, the bug fix is probably still a good thing - but maybe if the libDebugInfoDWARF APIs were changed the StringSwitch could be selective rather than exhaustive, just about the sections needed for gdb-index)

MaskRay edited the summary of this revision. (Show Details)Apr 28 2020, 9:32 PM

I'm surprised that generating gdb-index requires reading any debug locations - is there any chance of refactoring the APIs to make this not needed at all? (I mean, the bug fix is probably still a good thing - but maybe if the libDebugInfoDWARF APIs were changed the StringSwitch could be selective rather than exhaustive, just about the sections needed for gdb-index)

An alternative fix is to change https://github.com/llvm/llvm-project/blob/master/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp#L543 (DWARFUnit::tryExtractDIEsIfNeeded) to optionally decode .debug_loclists, and let --gdb-index not decode .debug_loclists, but I think the complexity may be larger...

grimar accepted this revision.Apr 29 2020, 12:27 AM

LGTM

This revision is now accepted and ready to land.Apr 29 2020, 12:27 AM
dblaikie accepted this revision.Apr 29 2020, 2:53 PM

Fair enough - seems like a reasonable fix/consistent with the existing code (are there any other missing sections?) though might be nice to revisit the API at some point. I don't think there should be any reading/need for loc/loclists in LLD's use of DWARF (except the full DWARF-aware linking stuff being prototyped)

MaskRay added a comment.EditedApr 29 2020, 2:59 PM

Fair enough - seems like a reasonable fix/consistent with the existing code (are there any other missing sections?) though might be nice to revisit the API at some point. I don't think there should be any reading/need for loc/loclists in LLD's use of DWARF (except the full DWARF-aware linking stuff being prototyped)

Thanks! --gdb-index does not need .debug_loclists . Only the full DWARF aware linking may need it.

I'll update the comment in lld/test/ELF/gdb-index-loclists.s to mention that this is just a regression test.

are there any other missing sections?

Hopefully no :)

MaskRay updated this revision to Diff 261053.Apr 29 2020, 2:59 PM

Update comment

MaskRay edited the summary of this revision. (Show Details)Apr 29 2020, 3:03 PM
This revision was automatically updated to reflect the committed changes.