This is an archive of the discontinued LLVM Phabricator instance.

Move non-DWARF code: `DWARFUnit` -> `SymbolFileDWARF`
ClosedPublic

Authored by jankratochvil on Nov 24 2019, 12:50 PM.

Details

Summary

This patchset is removing non-DWARF code from DWARFUnit as discussed with @labath.
@labath, maybe this is the only patch you did mean?
I do not need this patch myself but I am fine checking it in if you want it.

Diff Detail

Event Timeline

jankratochvil created this revision.Nov 24 2019, 12:50 PM

@labath, maybe this is the only patch you did mean?

This is (more or less -- I can't say I had a really exact picture in my head) what I had in mind, when I said that moving the high-level code out of the DWARFUnit class would be useful for the llvm<->lldb dwarf parser convergence. I am aware that you need more for the DWZ stuff -- that is fine, and I like that you have split that into a separate patch.

Anyway, I think this is a good direction to move in. When the time comes, I'd like to get some of clang ast folks to give this a look too, but maybe that's not needed yet...

jankratochvil planned changes to this revision.Nov 25 2019, 9:28 AM
jankratochvil retitled this revision from RFC 2/3: Move non-DWARF code: `DWARFUnit` -> `SymbolFileDWARF` to Move non-DWARF code: `DWARFUnit` -> `SymbolFileDWARF`.
jankratochvil edited the summary of this revision. (Show Details)

This moves a lot of lldb-specific stuff out of low-level dwarf code, and as such, I think this is a great step towards the parser unification.

I'd just like to squeeze one interface tweak to function we are moving around, but otherwise, I think this is fine.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
4012

please return the DWARFDeclContext by value here

jankratochvil marked 2 inline comments as done.Jan 30 2020, 9:13 PM
jankratochvil added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
4012

I did the same also for DWARFDebugInfoEntry::GetDWARFDeclContext. It is a part of the updated patch but you can see it separately.

jankratochvil marked an inline comment as done.
labath accepted this revision.Jan 31 2020, 12:53 AM

Looks great, modulo the inline comment.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3987–4008

Don't use else after return. In fact, you might as well fold the GetDWARFParser call into the if condition (if (DWARFASTParser *parser = ...) parser->...)

This revision is now accepted and ready to land.Jan 31 2020, 12:53 AM
This revision was automatically updated to reflect the committed changes.
jankratochvil marked 2 inline comments as done.Jan 31 2020, 6:26 AM

Checked in, thanks for the review.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3987–4008

This was only moving the code, I did not expect to also refactor it. But I have thus made these code cleanups which I found.

jankratochvil marked an inline comment as done.Jan 31 2020, 6:57 AM

This commit broke: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/7400/
Going to revert the return value refactorization.
That DWARFDebugInfoEntry::GetDWARFDeclContext() refactorization for return value was non-trivial and I see I did screw it up I think despite I did not want to. It is now adding it in opposite order.