This is an archive of the discontinued LLVM Phabricator instance.

[lldb][NFC] Move generic DWARFASTParser code out of Clang-specific code
ClosedPublic

Authored by ljmf00 on Nov 27 2021, 4:36 PM.

Details

Summary

This patch moves ParseChildArrayInfo out of DWARFASTParserClang in order to decouple Clang-specific logic from DWARFASTParser.

Diff Detail

Event Timeline

ljmf00 created this revision.Nov 27 2021, 4:36 PM
ljmf00 requested review of this revision.Nov 27 2021, 4:36 PM
ljmf00 added a subscriber: Geod24.Nov 27 2021, 4:37 PM
ljmf00 added inline comments.Nov 28 2021, 2:43 PM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp
78

Why we are including just these specific attributes? Maybe we should add a comment explaining it. According to the DWARF standard, any attribute is eligible for any tag.

I think breaking it out of the Clang-specific class makes sense if we want LLDB to be more language-agnostic. Do you have an idea of what bits of DWARFASTParserClang can be moved out other than ParseChildArrayInfo and GetAccessTypeFromDWARF (from the patch on top of this)? What is your end-goal with this decoupling? I assume you want to work towards supporting languages non-clang-based languages but I'm curious about the motivation.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp
2

This file will need a license header.

78

I'm not sure why. Possibly they were added to make sure the switch was fully covered (potentially to silence a warning)? You could add a FIXME or a TODO if you feel that these attributes should have functionality associated with them like the ones above.

ljmf00 retitled this revision from [lldb] Move generic DWARFASTParser code out of Clang-specific code to [lldb][NFC] Move generic DWARFASTParser code out of Clang-specific code.Nov 29 2021, 3:18 PM
ljmf00 added inline comments.Nov 29 2021, 6:38 PM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp
2

Yes, my bad. Thanks for noticing it.

78

I don't think it is to mark it as fully covered since there are much more attributes, the default label will address it anyway, and according to the DWARF standard, any attribute can be in a type tag (realistically, any tag). We can take the example of DW_AT_description which is just a description associated with the symbol. I feel like this can be safely deleted but I'm afraid to do it in favour of some other rationale I'm not seeing.

shafik added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp
78

@clayborg it looks like this has been this way since you put this in: https://github.com/llvm/llvm-project/commit/261ac3f4b5b98d02dd8718078015a92cf07df736

Do you agree this is dead code or is there something we are missing?

clayborg added inline comments.Nov 30 2021, 2:19 PM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp
78

Fine to remove these extra attributes that are in the default case as we definitely don't have all of the attributes listed here.

ljmf00 updated this revision to Diff 391856.Dec 4 2021, 12:20 PM
ljmf00 added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp
2

Added.

78

Done.

ljmf00 updated this revision to Diff 391867.Dec 4 2021, 12:33 PM

I think breaking it out of the Clang-specific class makes sense if we want LLDB to be more language-agnostic. Do you have an idea of what bits of DWARFASTParserClang can be moved out other than ParseChildArrayInfo and GetAccessTypeFromDWARF (from the patch on top of this)? What is your end-goal with this decoupling? I assume you want to work towards supporting languages non-clang-based languages but I'm curious about the motivation.

@bulbazord Yes, my plan is to make LLDB interfaces more language-agnostic, to accommodate D programming language DWARFASTParser and TypeSystem. I've seen other language plugins such as Go that simply copy and paste this method, but I want to make D addition clearer and avoid such duplication. You can see more similar changes on Clang-specific code decoupling on the stacked changes.

I have made the requested changes, can you re-review, please? Also pinging @shafik .

Ping @shafik @bulbazord . Can you re-review?

clayborg accepted this revision.Jan 14 2022, 3:59 PM
This revision is now accepted and ready to land.Jan 14 2022, 3:59 PM
ljmf00 edited the summary of this revision. (Show Details)Feb 10 2022, 8:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 7:47 AM
ljmf00 added a comment.Jun 2 2022, 7:50 AM

Sorry for leaving this hanging for a long time. Feel free to leave feedback if I'm missing something, in the meantime.

I'll try to push the other patches too. I ran the test suite again and didn't break anything.