This is an archive of the discontinued LLVM Phabricator instance.

RFC 3/3: Remove DWARFDIE dependency from functions moved by D70646
AbandonedPublic

Authored by jankratochvil on Nov 24 2019, 1:13 PM.

Details

Summary

This patchset is removing dependency on DWARFDIE's DWARFUnit *m_cu for stuff which is unavailable in DW_TAG_partial_unit without accessing its parent DW_TAG_compile_unit (which includes DW_TAG_partial_unit by DW_AT_import).
This patch has no regressions but to really make it usable for DWZ one still needs to adjust DWARFBaseDIE::GetDIERef() as one cannot generate DIERef (containing main unit idenitifier for DWZ) purely from DWARFDIE (not containing DWARFUnit *main_cu).
Refactored DWARFBaseDIE::GetDIERef() (adding some parameter) would then need adjustment also of functions calling it directly - DWARFBaseDIE::GetID() and SymbolFileDWARF::GetUID(const DWARFBaseDIE &die) (and many functions calling these).
I do not plan to check it in yet. I plan to finish it for use by DWZ patchset first (D40474 et al.). Asking whether this is a good way forward.

Diff Detail

Event Timeline

jankratochvil created this revision.Nov 24 2019, 1:13 PM
labath added inline comments.Nov 25 2019, 2:36 AM
lldb/include/lldb/Symbol/TypeSystem.h
108–110 ↗(On Diff #230822)

This part looks pretty dodgy. I'd like to avoid introducing plugin references in non-plugin code. It looks like this class isn't particularly clean already (DWARFDIE forward decl), but this seems to make the problem much worse.

Since this class already contains a SymbolFile pointer, maybe we could create some kind of a ast-parser constructing method on the SymbolFile class to avoid mentioning the SymbolFileDWARF directly.

If I was doing this, I'd probably try to take this one step further and merge the GetDWARFParser/GetPDBParser methods (whose only calls are in the relevant symbol file plugins) into a single GetASTParser method and do appropriate casts in the plugins themselves.

jankratochvil planned changes to this revision.Nov 25 2019, 9:28 AM

Remove that dodgy new parameter SymbolFileDWARF &dwarf.

jankratochvil planned changes to this revision.Nov 26 2019, 12:55 PM
jankratochvil marked 2 inline comments as done.
jankratochvil added inline comments.
lldb/include/lldb/Symbol/TypeSystem.h
108–110 ↗(On Diff #230822)

TBH I did not do the described refactoring, I just used the existing m_sym_file.

jankratochvil abandoned this revision.Jan 22 2020, 6:59 AM
jankratochvil marked an inline comment as done.

I tried to use DIERef instead of DWARFDIE everywhere as @labath does not like to increase DWARFDIE size from 16 bytes to 24 bytes. But that is not really feasible. For DIERef one needs to also carry SymbolFileDWARF * along and also resolving DIERef into DWARFDIE is slow as it has to bisect CUs from the DIE offset. I will post a different proposal how to implement DWZ.