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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1528
+DWARFASTParserClang::GetTemplateParametersString(const DWARFDIE &die) {
+  if (llvm::StringRef(die.GetName()).contains("<"))
+    return std::string();
----------------
reverse the condition and use early exit?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1541
+      llvm::raw_string_ostream os(template_name);
+      arg.dump(os);
+      if (!template_name.empty()) {
----------------
`dump` is a "debugging aid". I guess you should call `print` and pass the appropriate `PrintingPolicy`. It might just be the default, though maybe we could also try to make an effort to match the output in the non-simplified-names case.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1551
+    }
+    if (!all_template_names.empty()) {
+      all_template_names.append(">");
----------------
When can this be empty? Should we still include the `<>` in those cases?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2497
+  // templates parameters, try again with the template parameters stripped since
+  // with -gsimple-template-names the DT_name may only contain the base name.
+  if (types.Empty()) {
----------------
AT_name ?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2498
+  // with -gsimple-template-names the DT_name may only contain the base name.
+  if (types.Empty()) {
+    const llvm::StringRef nameRef = name.GetStringRef();
----------------
I don't think this can be keyed off of `Empty()` for two reasons:
- I believe the user can pass a non-empty type list into this function, expecting it to append to it
- just because you found one templated type above, it doesn't mean that there can't be another non-templated type somewhere (if only a part of the binary was built with simplified names).

I think we should just unconditionally (possibly guarded by a max_matches check) try both.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2500
+    const llvm::StringRef nameRef = name.GetStringRef();
+    auto it = nameRef.find('<');
+    if (it != llvm::StringRef::npos) {
----------------
Michael137 wrote:
> Is there some other way to determine whether the debug-info was generated from `-gsimple-template-names`? Then we wouldn't have to check the existence of a `<` in the name in multiple places and wouldn't have to do this lookup speculatively. With this change, if we fail to find a template type in the index, we would do the lookup all over again, only to not find it again. Could that get expensive? Just trying to figure out if we can avoid doing this `GetTypes` call twice.
> 
> There have been [talks](https://github.com/llvm/llvm-project/issues/58362) about doing a base-name search by default followed by fuzzy matching on template parameters (though in the context of function names). The `simple-template-names` came up as a good motivator/facilitator for doing so. But for that idea to work here we'd have to be able to retrieve from the index by basename, so perhaps out of the scope of what we're trying to do here
> 
> tagging people involved in that discussion: @dblaikie @aprantl @jingham @labath
> 
> Other than this, generally LGTM
Negative matches should be fast, and since typically only one of the branches will produce any results, I think this should be fine. Filtering matches in the simplified case would be slower, since you'd have build the full name of all potential candidates (and that could be thousands of instantiations), but I don't know how slow. But that's the price we pay for better filtering (which we don't do right now, but we could start doing afterwards).


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2577
 
   ConstString name = pattern.back().name;
 
----------------
Do we need to do something similar in this FindTypes overload as well?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134378/new/

https://reviews.llvm.org/D134378