This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Don't create Clang AST nodes in GetDIEClassTemplateParams
ClosedPublic

Authored by aeubanks on Jan 23 2023, 3:47 PM.

Details

Summary

Otherwise we may be inserting a decl into a DeclContext that's not fully defined yet.

This simplifies/removes some clang AST node creation code. Instead, use
clang::printTemplateArgumentList().

Diff Detail

Event Timeline

aeubanks created this revision.Jan 23 2023, 3:47 PM
Herald added a project: Restricted Project. · View Herald Transcript
aeubanks requested review of this revision.Jan 23 2023, 3:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2023, 3:47 PM

there may be an alternate solution involving making some declarations into definitions (at all? earlier?), but I don't have that level of understanding of lldb

Michael137 added inline comments.Jan 24 2023, 5:29 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
1533

Ah the function was declared but not defined (presumably a leftover from https://reviews.llvm.org/D138834)

Can we elaborate in the function docstring how this differs from GetTemplateParameters?

1579–1580

While you're here mind correcting this?

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
101

Btw, what's guaranteeing that we don't call this more than once per DIE? Is there any way we could add a test that would notify us of that kind of regression? Or perhaps an assert somewhere in the DWARFASTParserClang?

lldb/test/API/lang/cpp/nested-template/TestNestedTemplate.py
13

Nit: technically don't need to run the program to test image lookup?

lldb/test/API/lang/cpp/nested-template/main.cpp
10

Nit: we typically use std::puts("Set breakpoint here") or return 0 for source regex breakpoints. The rationale was that breaking on comments isn't always reliable/future-proof.

there may be an alternate solution involving making some declarations into definitions (at all? earlier?), but I don't have that level of understanding of lldb

One idea that came to mind is that instead of creating a dummy template decl in GetDIEClassTemplateParams, you could perhaps construct a clang::TemplateArgumentList from the TemplateArguments stored in TemplateParameterInfos and use the clang::printTemplateArgumentList to stringify the template arguments, passing it the printing-policy which is used in TypeSystemClang to print type-names in your current implementation.

Michael137 added inline comments.Jan 24 2023, 6:41 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
1533

Meant to say GetDIEClassTemplateParams

aeubanks updated this revision to Diff 491924.Jan 24 2023, 2:54 PM
aeubanks marked an inline comment as done.

use suggested alternative

aeubanks retitled this revision from [lldb] Don't create Clang AST nodes in GetCPlusPlusQualifiedName to [lldb] Don't create Clang AST nodes in GetDIEClassTemplateParams.Jan 24 2023, 2:55 PM
aeubanks edited the summary of this revision. (Show Details)

there may be an alternate solution involving making some declarations into definitions (at all? earlier?), but I don't have that level of understanding of lldb

One idea that came to mind is that instead of creating a dummy template decl in GetDIEClassTemplateParams, you could perhaps construct a clang::TemplateArgumentList from the TemplateArguments stored in TemplateParameterInfos and use the clang::printTemplateArgumentList to stringify the template arguments, passing it the printing-policy which is used in TypeSystemClang to print type-names in your current implementation.

this worked perfectly and simplified a lot of code, thanks!

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
1533

yeah I forgot to remove the declaration in that patch :|

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
101

now we don't create a clang AST node

lldb/test/API/lang/cpp/nested-template/TestNestedTemplate.py
13

last time I couldn't find the right thing to call to load the binary but not set a breakpoint, but perhaps this works?

lldb/test/API/lang/cpp/nested-template/main.cpp
10

obsolete now that we don't set a breakpoint

Michael137 accepted this revision.Jan 24 2023, 5:22 PM

awesome, looks much better now
thanks!

This revision is now accepted and ready to land.Jan 24 2023, 5:22 PM