Page MenuHomePhabricator

Cleanup DWARFCompileUnit and DWARFUnit in preparation for adding DWARFTypeUnit
ClosedPublic

Authored by clayborg on Apr 2 2018, 9:45 AM.

Details

Summary

Many things that were in DWARFCompileUnit actually need to be in DWARFUnit. This patch moves all DWARFUnit specific things over into DWARFUnit and fixes the layering. This is in preparation for adding DWARFTypeUnit for the .debug_types patch.

Diff Detail

Repository
rL LLVM

Event Timeline

clayborg created this revision.Apr 2 2018, 9:45 AM
davide added a comment.Apr 2 2018, 9:55 AM

Thanks Greg. This is a very large patch, but it seems mostly churn. I'll try to find the time to review it carefully tomorrow.

Yes this is just moving all ivars from DWARFCompileUnit to DWARFUnit, moving all functions that used those accessors to DWARFUnit and remove the indirection through DWARFCompileUnit that was in DWARFUnit using:

virtual DWARFCompileUnit &Data() = 0;

No real functional change, just fixing layering and moving code. Also a renaming from GetCompileUnitDIEOnly to GetUnitDIEOnly.

I have only one general question and otherwise I'm fine with this: Does this bring LLDB's DWARF parser closer to the structure of LLVM's? We still want to long-term replace LLDB's DWARF parser with LLVM's so every refactoring should aim at making their interfaces more similar.

I have only one general question and otherwise I'm fine with this: Does this bring LLDB's DWARF parser closer to the structure of LLVM's? We still want to long-term replace LLDB's DWARF parser with LLVM's so every refactoring should aim at making their interfaces more similar.

It does indeed. Very close.

aprantl accepted this revision.Apr 5 2018, 8:45 AM

That's great! Let's go for it then. Could you please doxygen'ify the comments?

This revision is now accepted and ready to land.Apr 5 2018, 8:45 AM
This revision was automatically updated to reflect the committed changes.

I disagree with this patch as DWARFUnit was a lightweight wrapper for DWARFPartialUnit. Now I will have to create some new lightweight superclass like DWARFAbstractUnit.
My patch prepared it for:

DWARFUnit->DWARFCompileUnit
DWARFUnit->DWARFPartialUnit

And I planned the type units should be implemented like:

DWARFUnit->DWARFSomeNameUnit->DWARFCompileUnit
DWARFUnit->DWARFSomeNameUnit->DWARFTypeUnit
DWARFUnit->DWARFPartialUnit

This patch just reused + changed my abstraction for a completely different purpose and I will have to reimplement it again under a different name. Or what do you suggest?

I have reverted this commit after approval by Davide Italiano and Adrian Prantl at:

https://lists.llvm.org/pipermail/lldb-commits/Week-of-Mon-20180402/040176.html

The revert:

git-svn-id: https://llvm.org/svn/llvm-project/lldb/trunk@329423 91177308-0d34-0410-b5e6-96231b3b80d8
GIT commit a4b4ca0aef594f71e44478c2f16ca9fd53938ec6