[Lldb-commits] [PATCH] D134378: [lldb] Support simplified template names

[David Blaikie via Phabricator via lldb-commits]

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