This is an archive of the discontinued LLVM Phabricator instance.

DWARF: Make DIERefs always valid
ClosedPublic

Authored by labath on Jun 17 2019, 1:41 AM.

Details

Summary

This patch makes the DIERef class always valid by default constructor
and operator bool. This allows one to express the validity of a DIERef
in the type system. Places which are working with potentially-invalid
DIERefs have been updated to use Optional<DIERef> instead.

The constructor taking a DWARFFormValue was not needed, as all places
which were constructing a DIERef this way were immediately converting it
into a DWARFDIE or a user_id. This can be done without constructing an
intermediate DIERef.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Jun 17 2019, 1:41 AM
labath marked an inline comment as done.Jun 17 2019, 1:43 AM
labath added inline comments.
source/Plugins/SymbolFile/DWARF/DIERef.cpp
1 ↗(On Diff #205002)

I'm not deleting this file, because I'm going to add some stuff to it in the next patch in this series.

I am concerned that our mapping from DIERef to lldb::user_id_t won't work for all cases now that we are/have expanded the DIERef class (including as we add the DWO field). I voiced this concern in https://reviews.llvm.org/D63428. Let me know what you think.

source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
59 ↗(On Diff #205002)

Shouldn't we be using the section from the DIERef in "entry" instead of hard coding to "DIERef::Section::DebugInfo" here? If so we need a test to cover this case so we don't regress

70 ↗(On Diff #205002)

Shouldn't we be using the section from the DIERef in "entry" instead of hard coding to "DIERef::Section::DebugInfo" here?

source/Plugins/SymbolFile/DWARF/HashedNameToDIE.h
64 ↗(On Diff #205002)

DIEInfo objects are only ever used in .debug_info?

labath marked 2 inline comments as done.Jun 17 2019, 12:17 PM

I am concerned that our mapping from DIERef to lldb::user_id_t won't work for all cases now that we are/have expanded the DIERef class (including as we add the DWO field). I voiced this concern in https://reviews.llvm.org/D63428. Let me know what you think.

I'll reply to that review tomorrow, for now here's a quick reply to the comments in this review.

source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
59 ↗(On Diff #205002)

debug_names is DWARF5 only, which has moved back type units into the debug_info section. So entry does not contain the section, nor does it need to because we can always assume all units will be in the debug_info section.

source/Plugins/SymbolFile/DWARF/HashedNameToDIE.h
64 ↗(On Diff #205002)

Yes. They are only used with apple indexes, and those only work with debug_info sections. -fdebug-types-section is hard-disabled on apple targets nowadays (it used to crash the compiler before that).

JDevlieghere accepted this revision.Jun 17 2019, 12:22 PM
This revision is now accepted and ready to land.Jun 17 2019, 12:22 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2019, 12:31 AM