This is an archive of the discontinued LLVM Phabricator instance.

Permit cross-CU references
ClosedPublic

Authored by jankratochvil on May 3 2019, 6:32 AM.

Details

Summary

So far dw_offset_t was global for the whole SymbolFileDWARF but with .debug_types the same dw_offset_t may mean two different things depending on its section (=CU). So references now return whole new referenced DWARFDIE instead of just dw_offset_t.

This means that some functions have to now handle 16 bytes instead of 8 bytes but I do not see that anywhere performance critical.

Diff Detail

Repository
rL LLVM

Event Timeline

jankratochvil created this revision.May 3 2019, 6:32 AM
jankratochvil retitled this revision from 03/06: New CUDIERef to Permit cross-CU references.
jankratochvil edited the summary of this revision. (Show Details)

I like this patch in a lot of ways, because it makes the code cleaner, and prepares us for DWARF5, which can reference entries in other files in a lot of interesting ways.

The part I am not totally sure if is the alignment of lldb's DWARFFormValue with it's llvm counterpart. The llvm version does not have this fancy cross-unit resolution capability, but it just returns the raw value and leaves it up to the user to make sense of it (much like our class does now). However, it's possible that the llvm version does not have this functionality because there wasn't need for it, and it already has access to the DWARFContext, so it is certainly prepared to do more complex lookups, so it's possible it could be extended similarly once we go around to merging the two. And even if we go back to the world where form.Reference() returns a raw value, this may serve as a good intermediate step, because otherwise we'd have a hard time catching all the cases where someone calls Reference, and expects it to return an simple section offset.

So, overall, I'm inclined to accept this patch, but I'd like to hear what @JDevlieghere and @clayborg think.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
114–115 ↗(On Diff #199490)

It looks like these two functions are equivalent now and we can remove one of them (I vote to keep this one).

clayborg accepted this revision.May 15 2019, 9:45 AM

Just a few simplifications to GetName() and AppendTypeName() are in question and can optionally be done if needed.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
699 ↗(On Diff #199490)

Should we make GetName() just take a DWARFDIE now?:

GetName(abstract_die, s);
705 ↗(On Diff #199490)

Should we make AppendTypeName() just take a DWARFDIE now?:

AppendTypeName(type_die, s);
1199–1200 ↗(On Diff #199490)

Should we make AppendTypeName() just take a DWARFDIE now?:

AppendTypeName(next_die, s);
This revision is now accepted and ready to land.May 15 2019, 9:45 AM
jankratochvil marked an inline comment as done.May 15 2019, 12:17 PM

Just a few simplifications to GetName() and AppendTypeName() are in question and can optionally be done if needed.

I will do that in a different patch as its removal of the SymbolFileDWARF *dwarf2Data parameter goes outside of scope of this patch.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2019, 12:20 PM