Page MenuHomePhabricator

[ELF] --gdb-index: extract entities from .debug_info when .debug_gnu_pubnames is absent
Needs ReviewPublic

Authored by MaskRay on Nov 13 2018, 3:07 PM.

Details

Summary

When .debug_gnu_pubnames is absent, gold traverses .debug_info to
extract entities to make up symbol table entries. This patch implements
a subset of it to improve .gdb_index coverage.

Event Timeline

MaskRay created this revision.Nov 13 2018, 3:07 PM

Is this test case consistent with gold's behavior? (given this subprogram has no source locations here, for instance - does gold still consider such a simplified DIE to be a thing that should be indexed?)

Also would probably be good to document the differences/incompleteness - what cases does this not cover? (false negatives, where an index entry should be present but isn't - and false positives (index entries that shouldn't be present) & the details of things like the attributes for the entry?)

echristo added inline comments.Nov 13 2018, 4:21 PM
ELF/SyntheticSections.cpp
2417

Can you document what the different subset is?

2439

From a quick look: what if DW_AT_sibling isn't there? Does this still work?

dblaikie added inline comments.Nov 13 2018, 4:46 PM
ELF/SyntheticSections.cpp
2439

Yeah, not a problem, this isn't powered by DW_AT_sibling (see https://github.com/llvm-mirror/llvm/blob/master/lib/DebugInfo/DWARF/DWARFUnit.cpp#L695 ). Probably be nice if we changed this sibling based iteration to iterator-based iteration to avoid confusion like this & separate out DIE navigation from DIEs themselves.

Oh, right, we do have such an API & should probably use that here instead:

for (const DWARFDie &Child : D.children())
  extractSymbol(Child, Idx, Ret);
MaskRay updated this revision to Diff 173967.Nov 13 2018, 5:03 PM
MaskRay marked 3 inline comments as done.

Add comment about the missing parts.

Is this test case consistent with gold's behavior?

It does. But I think I need to do more work to understand what gold expects (it may have issues with my other multi-cu tests).

% fllvm-mc -filetype=obj gdb-index-debug-info.s -o t.o
% gold --gdb-index t.o -o t.gold
% fld.lld --gdb-index t.o -o t.gold
% diff <(fllvm-dwarfdump -debug-info t.gold) <(fllvm-dwarfdump -debug-info t.lld)
1c1
< t.gold:       file format ELF64-x86-64
---
> t.lld:        file format ELF64-x86-64
grimar added a subscriber: grimar.Nov 14 2018, 12:47 AM
MaskRay updated this revision to Diff 174137.Nov 14 2018, 5:30 PM

Update comment

ruiu added inline comments.Nov 14 2018, 5:49 PM
ELF/SyntheticSections.cpp
2424

Miss -> Missinug

2426

What is the consequence of the function always returning None?

MaskRay marked an inline comment as done.Nov 14 2018, 5:52 PM
MaskRay added inline comments.
ELF/SyntheticSections.cpp
2426

Our internal script may break weirdly. I may not sure if this information is useful and I am happy to remove it.

ruiu added inline comments.Nov 14 2018, 8:12 PM
ELF/SyntheticSections.cpp
2426

I'm not sure if this is a good thing to create a half-baked .gdb-index section for an incomplete input. Silently creating a .gdb-index that is not complete could be very confusing. I feel like if we do this, we should do this perfectly. Or we should report an warning and should refuse to create a .gdb-index section.

Can gold create a complete .gdb-index section from object files that don't have .debug_gnu_pubnames sections? What if both .debug_gnu_pubnames and .debug_gnu_pubtypes are absent? (I believe -ggnu-pubnames creates both sections, so it is hard to imagine that an object file contains only one of them though.)

If gold can still create .gdb-index sections from object files compiled without -ggnu-pubnames, then, what is the point of -ggnu-pubnames option?

dblaikie added inline comments.Nov 14 2018, 9:23 PM
ELF/SyntheticSections.cpp
2426

Yeah, I tend to agree about half-baking this. Though, equally I don't personally mind if this is done incrementally either (but I'm not sure it makes it that much easier to follow compared to implementing an attempt at the full algorithm up-front).

What's the purpose of gnu-pubnames:

  • Linker/debugger (whichever consumes it) performance - when present it's faster to walk the list than to parse all the DWARF to find things
  • With Split DWARF it's necessary (to some degree) - the linker can't parse the DWARF (because the linker doesn't see most of the DWARF - it's in the .dwo file/not present in the objects that are being linked) so it must rely on gnu-pubnames to build the gdb-index in that case. (I say necessary to some degree, because in theory the debugger could still go and read all the debug info in the .dwo/.dwps - it'd be insanely slow, but possible - but in fact GDB relies on the presence of the .gdb_index for correctness (When reading split DWARF it doesn't have a fallback to build the index itself from the DWARF))
saugustine added inline comments.Nov 15 2018, 11:05 AM
ELF/SyntheticSections.cpp
2426

Gold can create a correct index from object files without gnu pubnames, but it is dramatically slower.