dblaikie added inline comments.
Herald added a subscriber: JDevlieghere.
================
Comment at: lldb/packages/Python/lldbsuite/test/make/Makefile.rules:230-231
-DEBUG_INFO_FLAG ?= -g
+# DO NOT SUBMIT
+DEBUG_INFO_FLAG ?= -g -gsimple-template-names
----------------
Oh, nifty place to test this - I'd been testing with the default changed in
clang itself.
================
Comment at:
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1558-1560
+ if (template_param_infos.hasParameterPack()) {
+ args = template_param_infos.packed_args->args;
+ }
----------------
I think there's some existing problems with lldb here (& gdb, as it happens) -
this doesn't specify where the parameter pack is in the argument list (looks
like it assumes it's the only template parameters?) - and doesn't handle the
possibility of multiple parameter packs in a single parameter list, which I
think is possible in some corner cases.
I think maybe the existing lldb code can handle some non-pack parameters
followed by a single pack, but this code assumes it's either non-pack or a
pack, never both - so might be more restrictive than the existing limitations?
But maybe not - I might've misread/misremembered the other lldb code.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1564
+ llvm::raw_string_ostream os(template_name);
+ arg.dump(os);
+ if (!template_name.empty()) {
----------------
OK, so this line uses Clang's AST printing (avoiding having to reimplement all
the type printing in lldb, like I've done in llvm-dwarfdump/libDebugInfoDWARF)
- but I guess there's a reason we can't do that at a higher level for the whole
template, but have to do it per template argument?
It'd be nice if we could do it for the whole parameter list/template
specialization, to avoid having to have this <>, etc handling.
I guess it's a circular problem - have to build the name to look up the AST if
we already have it... so can't build the name /from/ the AST, as such. Fair
enough.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1585
+ DWARFDIE parent_decl_ctx_die = die.GetParentDeclContextDIE();
+ // TODO: change this to get the correct decl context parent....
+ while (parent_decl_ctx_die) {
----------------
What's incorrect about it at the moment?
Oh, I see this code has just moved around.
================
Comment at:
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1590-1591
+ case DW_TAG_namespace: {
+ const char *namespace_name = parent_decl_ctx_die.GetName();
+ if (namespace_name) {
+ qualified_name.insert(0, "::");
----------------
================
Comment at:
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1605-1608
+ const char *class_union_struct_name =
+ parent_decl_ctx_die.GetName();
+
+ if (class_union_struct_name) {
----------------
================
Comment at:
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1805-1812
+ bool is_template = false;
+ TypeSystemClang::TemplateParameterInfos template_param_infos;
+ if (ParseTemplateParameterInfos(die, template_param_infos)) {
+ is_template = !template_param_infos.args.empty() ||
+ template_param_infos.packed_args;
+ }
+
----------------
Maybe? But the named variable and less indentation's probably good too.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2491-2495
+ TypeSP Ty = GetTypeForDIE(die);
+ llvm::StringRef qualName = Ty->GetQualifiedName().AsCString();
+ auto it = qualName.find('<');
+ if (it == llvm::StringRef::npos)
+ return true;
----------------
Maybe a comment here? I guess what's happening here is that if the name found
isn't a template then it isn't relevant/can't match the original query that did
have template parameters?
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2498-2501
+ llvm::StringRef qualNameTemplateParams =
+ qualName.slice(it, qualName.size());
+ if (templateParams != qualNameTemplateParams)
+ return true;
----------------
And this checks the stringified template params match exactly - so there's
opportunity for improvement in the future to compare with more fuzzy matching
(I guess we can't use clang's AST because that'd involve making two instances
of the type somehow which doesn't make a lot of sense) so that users don't have
to spell the template spec exactly the same way clang does (eg: `x<int, int>`
versus `x<int,int>` - or other more complicated situations with function
pointers, parentheses, casts, etc.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134378/new/
https://reviews.llvm.org/D134378
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
