This is an archive of the discontinued LLVM Phabricator instance.

Simplify `GetName`+`AppendTypeName` by `DWARFDIE`
ClosedPublic

Authored by jankratochvil on May 21 2019, 11:07 AM.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

jankratochvil created this revision.May 21 2019, 11:07 AM
JDevlieghere added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
1073

How about turning this into an early return?

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
129

Since we pass by value we can remove the const?

jankratochvil marked 3 inline comments as done.May 21 2019, 12:17 PM
jankratochvil added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
1073

This deep nesting is a general coding style in LLDB so I try to keep it. Yes, I would like to (and I use elsewhere) early returns to keep the nesting low. I will submit another patch for it when you say it is OK to clean it up.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
129

There was already const dw_offset_t but OK, I will remove it as it is probably a LLVM rule.

jankratochvil marked an inline comment as done.
clayborg requested changes to this revision.May 21 2019, 1:07 PM

The whole goal of the DWARFDebugInfoEntry::GetName() and DWARFDebugInfoEntry::AppendTypeName() we might be needing to dump the name of a DIE in DWARF that was not parsed yet. Like if a DIE has a forward reference to a DIE and we are trying to dump a DWARFDebugInfoEntry when things go wrong. If this is not needed then just move these functions into DWARFDIE.

This revision now requires changes to proceed.May 21 2019, 1:07 PM

We really shouldn't have any DWARFDIE references inside DWARFDebugInfoEntry.h. I noticed there are a lot now which is wrong.

We really shouldn't have any DWARFDIE references inside DWARFDebugInfoEntry.h. I noticed there are a lot now which is wrong.

I take this back. Many uses are good inside of DWARFDebugInfoEntry as I ok'ed patches when I looked at them. Initially this file was intended to be the level below DWARFDIE, but that has changed, and for good reason: make sure we never use the wrong DWARFUnit with a DWARFDebugInfoEntry. But my initial comment about moving the GetName and AppendTypeName still stand. If we aren't using these for forward references when we don't have all of the DWARFDebugInfoEntry objects parsed already, then they should be moved to DWARFDIE and removed from here.

Isn't all of this dead code? These functions seem to be only called from DWARFDebugInfoEntry::Dump, and I can find no callers of the Dump method.

If that's the case, what I'd do is delete all of this stuff, and then if we ever need to dump debug info entries again, implement a llvm-style dump method on the DWARFDIE class. This will move us closer to aligning lldb and llvm versions of these two classes.

We really shouldn't have any DWARFDIE references inside DWARFDebugInfoEntry.h. I noticed there are a lot now which is wrong.

I take this back. Many uses are good inside of DWARFDebugInfoEntry as I ok'ed patches when I looked at them. Initially this file was intended to be the level below DWARFDIE, but that has changed, and for good reason: make sure we never use the wrong DWARFUnit with a DWARFDebugInfoEntry. But my initial comment about moving the GetName and AppendTypeName still stand. If we aren't using these for forward references when we don't have all of the DWARFDebugInfoEntry objects parsed already, then they should be moved to DWARFDIE and removed from here.

I think it is still possible (and desirable) to maintain (or restore) the "DWARFDIE >> DWARFDebugInfoEntry" invariant, while still making sure that users don't have to remember which entry goes with which compile unit. The way to achieve that is by making that nobody (outside the DWARFDIE class, and maybe some other low-level stuff) deals with DWARFDebugInfoEntries directly). That's pretty much what was done in the llvm's version of the parser where DWARFDebugInfoEntry is an extremely dumb class with just 5 simple accessor functions, and all of the interesting stuff happens in DWARFDie.

I have moved it to DWARFDIE.cpp. I agree that it is probably all irrelevant after moving to the LLVM DWARF/ part. I have also removed return type as (1) it was wrong in one case and (2) no existing caller used the return type. I also refactored the deep nesting noted by @JDevlieghere.
I can also remove all the dumping code instead upon request, @labath.

jankratochvil marked an inline comment as done.May 22 2019, 8:59 AM
clayborg accepted this revision.May 22 2019, 10:14 AM
This revision is now accepted and ready to land.May 22 2019, 10:14 AM
JDevlieghere added inline comments.May 22 2019, 4:43 PM
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
1073

The deep nesting is definitely a bug and not a feature. We've been removing it where it makes sense.

^ Feel free to address that in a follow up commit

This revision was automatically updated to reflect the committed changes.