[lldb] Fix simple template names and template params with scope qualifiers

[aeubanks]

Summary

Followup to D134378.

With PrintingPolicy::SuppressScope, we'd also not print the scope in template params. The intention was only to skip the scope for the class because we expect template params to be fully qualified when comparing them for simple template names.

Instead, use NamedDecl::getNameForDiagnostic if we're dealing with a tag, which is what we actually use when emitting debug info in clang. That already has an option to suppress the scope on the base name.

[dblaikie]

@aaron.ballman does this seem OK? (this was based on my suggestion in the related review linked in the description)

It probably needs tests in clang too - not sure if there's an opportunity to use a unit test to simplify access to the printing policy & exercise this?

[aaron.ballman]

...we expect template params to be fully qualified when comparing them for simple template names

So lldb is not inspecting the AST, they're doing reflection (of a sort) on the pretty printed names? Or am I misunderstanding something?

clang/include/clang/AST/PrettyPrinter.h

134 ↗

(On Diff #473766)

I think this should be expressed in the positive rather than in the negative (SuppressTemplateParamScope) so it's consistent with the rest of the options.

[dblaikie]

In D137583#3917706, @aaron.ballman wrote:

...we expect template params to be fully qualified when comparing them for simple template names

So lldb is not inspecting the AST, they're doing reflection (of a sort) on the pretty printed names? Or am I misunderstanding something?

Not reflection as such - but building names for the user, but partly from the AST - basically LLDB wants to be able to produce the same name that CGDebugInfo produces - so, maybe it should produce it the same way as CGDebugInfo, which isn't to use the pretty printer from scratch.

@aeubanks would this work for lldb's use case? https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGDebugInfo.cpp#L5229 it'd be identical to the original debug info generation, and looks like it doesn't require a printing policy change/feature. Sorry I didn't think of that earlier. I guess since Qualified would be false for lldb's use case, you could go down into the implementation and just call the unqualified side directly: NamedDecl::printName(OS, Policy); should print it unqualified for this name, but respect the qualified printing policy flag for any nested names, parameters, etc.

[aaron.ballman]

In D137583#3917735, @dblaikie wrote:

In D137583#3917706, @aaron.ballman wrote:

...we expect template params to be fully qualified when comparing them for simple template names

So lldb is not inspecting the AST, they're doing reflection (of a sort) on the pretty printed names? Or am I misunderstanding something?

Not reflection as such - but building names for the user, but partly from the AST - basically LLDB wants to be able to produce the same name that CGDebugInfo produces - so, maybe it should produce it the same way as CGDebugInfo, which isn't to use the pretty printer from scratch.

Ah thank you for the clarification, that makes more sense to me.

@aeubanks would this work for lldb's use case? https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGDebugInfo.cpp#L5229 it'd be identical to the original debug info generation, and looks like it doesn't require a printing policy change/feature. Sorry I didn't think of that earlier. I guess since Qualified would be false for lldb's use case, you could go down into the implementation and just call the unqualified side directly: NamedDecl::printName(OS, Policy); should print it unqualified for this name, but respect the qualified printing policy flag for any nested names, parameters, etc.

I agree that it seems pretty sensible for the debugger to use the same mechanisms as debug info in terms of what names to display for users.

[aeubanks]

use getNameForDiagnostics

[aeubanks]

add comment

[aeubanks]

In D137583#3917735, @dblaikie wrote:

In D137583#3917706, @aaron.ballman wrote:

...we expect template params to be fully qualified when comparing them for simple template names

So lldb is not inspecting the AST, they're doing reflection (of a sort) on the pretty printed names? Or am I misunderstanding something?

Not reflection as such - but building names for the user, but partly from the AST - basically LLDB wants to be able to produce the same name that CGDebugInfo produces - so, maybe it should produce it the same way as CGDebugInfo, which isn't to use the pretty printer from scratch.

@aeubanks would this work for lldb's use case? https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGDebugInfo.cpp#L5229 it'd be identical to the original debug info generation, and looks like it doesn't require a printing policy change/feature. Sorry I didn't think of that earlier. I guess since Qualified would be false for lldb's use case, you could go down into the implementation and just call the unqualified side directly: NamedDecl::printName(OS, Policy); should print it unqualified for this name, but respect the qualified printing policy flag for any nested names, parameters, etc.

much better, thanks!

[dblaikie]

Awesome!

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h

#endif
 
   // Type Completion
 
   bool GetCompleteType(lldb::opaque_compiler_type_t type) override;
 
   // Accessors
 
   ConstString GetTypeName(lldb::opaque_compiler_type_t type,
-                          bool BaseOnly) override;
+                          bool base_only) override;
 
   ConstString GetDisplayTypeName(lldb::opaque_compiler_type_t type) override;
 
   uint32_t GetTypeInfo(lldb::opaque_compiler_type_t type,
                        CompilerType *pointee_or_element_compiler_type) override;
 
   lldb::LanguageType
   GetMinimumLanguage(lldb::opaque_compiler_type_t type) override;
#endif
   }
 
 private:
   /// Returns the PrintingPolicy used when generating the internal type names.
   /// These type names are mostly used for the formatter selection.
   clang::PrintingPolicy GetTypePrintingPolicy();
   /// Returns the internal type name for the given NamedDecl using the
   /// type printing policy.
-  std::string GetTypeNameForDecl(const clang::NamedDecl *named_decl);
+  std::string GetTypeNameForDecl(const clang::NamedDecl *named_decl,
+                                 bool qualified = true);
 
   const clang::ClassTemplateSpecializationDecl *
   GetAsTemplateSpecialization(lldb::opaque_compiler_type_t type);
 
   // Classes that inherit from TypeSystemClang can see and modify these
   std::string m_target_triple;
   std::unique_ptr<clang::ASTContext> m_ast_up;
   std::unique_ptr<clang::LangOptions> m_language_options_up;

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

PrintingPolicy TypeSystemClang::GetTypePrintingPolicy() {
   //   std::basic_string<char, std::char_traits<char>, std::allocator<char> >
   // to support setups where LLDB was able to reconstruct default arguments
   // (and we then would have suppressed them from the type name) and also setups
   // where LLDB wasn't able to reconstruct the default arguments.
   printing_policy.SuppressDefaultTemplateArgs = false;
   return printing_policy;
 }
 
-std::string TypeSystemClang::GetTypeNameForDecl(const NamedDecl *named_decl) {
+std::string TypeSystemClang::GetTypeNameForDecl(const NamedDecl *named_decl,
+                                                bool qualified) {
   clang::PrintingPolicy printing_policy = GetTypePrintingPolicy();
   std::string result;
   llvm::raw_string_ostream os(result);
-  named_decl->printQualifiedName(os, printing_policy);
+  named_decl->getNameForDiagnostic(os, printing_policy, qualified);
   return result;
 }
 
 FunctionDecl *TypeSystemClang::CreateFunctionDeclaration(
     clang::DeclContext *decl_ctx, OptionalClangModuleID owning_module,
     llvm::StringRef name, const CompilerType &function_clang_type,
     clang::StorageClass storage, bool is_inline) {
   FunctionDecl *func_decl = nullptr;
bool TypeSystemClang::GetCompleteType(lldb::opaque_compiler_type_t type) {
   if (!type)
     return false;
   const bool allow_completion = true;
   return GetCompleteQualType(&getASTContext(), GetQualType(type),
                              allow_completion);
 }
 
 ConstString TypeSystemClang::GetTypeName(lldb::opaque_compiler_type_t type,
-                                         bool BaseOnly) {
+                                         bool base_only) {
   if (!type)
     return ConstString();
 
   clang::QualType qual_type(GetQualType(type));
 
   // Remove certain type sugar from the name. Sugar such as elaborated types
   // or template types which only serve to improve diagnostics shouldn't
   // act as their own types from the user's perspective (e.g., formatter
   // shouldn't format a variable differently depending on how the ser has
   // specified the type. '::Type' and 'Type' should behave the same).
   // Typedefs and atomic derived types are not removed as they are actually
   // useful for identifiying specific types.
   qual_type = RemoveWrappingTypes(qual_type,
                                   {clang::Type::Typedef, clang::Type::Atomic});
 
   // For a typedef just return the qualified name.
   if (const auto *typedef_type = qual_type->getAs<clang::TypedefType>()) {
     const clang::TypedefNameDecl *typedef_decl = typedef_type->getDecl();
     return ConstString(GetTypeNameForDecl(typedef_decl));
   }
 
-  clang::PrintingPolicy printing_policy(GetTypePrintingPolicy());
-  printing_policy.SuppressScope = BaseOnly;
-  return ConstString(qual_type.getAsString(printing_policy));
+  // For consistency, this follows the same code path that clang uses to emit
+  // debug info. This also handles when we don't want any scopes preceding the
+  // name.
+  if (auto *named_decl = qual_type->getAsTagDecl())
+    return ConstString(GetTypeNameForDecl(named_decl, !base_only));
+
+  return ConstString(qual_type.getAsString(GetTypePrintingPolicy()));
 }
 
 ConstString
 TypeSystemClang::GetDisplayTypeName(lldb::opaque_compiler_type_t type) {
   if (!type)
     return ConstString();
 
   clang::QualType qual_type(GetQualType(type));

lldb/test/API/lang/cpp/unique-types2/TestUniqueTypes2.py

def do_test(self, debug_flags):
         self.expect("image lookup -A -t 'FooPack<float, int>'", DATA_TYPES_DISPLAYED_CORRECTLY, error=True)
 
         self.expect("image lookup -A -t '::Foo<int>::Nested<char>'", DATA_TYPES_DISPLAYED_CORRECTLY, substrs=["1 match found"])
         self.expect("image lookup -A -t 'Foo<int>::Nested<char>'", DATA_TYPES_DISPLAYED_CORRECTLY, substrs=["1 match found"])
         self.expect("image lookup -A -t 'Foo<char>::Nested<char>'", DATA_TYPES_DISPLAYED_CORRECTLY, error=True)
         self.expect("image lookup -A -t 'Foo<int>::Nested<int>'", DATA_TYPES_DISPLAYED_CORRECTLY, error=True)
         self.expect("image lookup -A -t 'Nested<char>'", DATA_TYPES_DISPLAYED_CORRECTLY, substrs=["1 match found"])
         self.expect("image lookup -A -t '::Nested<char>'", DATA_TYPES_DISPLAYED_CORRECTLY, error=True)
+        self.expect("image lookup -A -t 'Foo<int>::Nested<ns::Bar>'", DATA_TYPES_DISPLAYED_CORRECTLY, substrs=["1 match found"])
 
         self.expect_expr("t1", result_type="Foo<char>")
         self.expect_expr("t1", result_type="Foo<char>")
         self.expect_expr("t2", result_type="Foo<int>")
         self.expect_expr("t3", result_type="Foo<Foo<int> >")
         self.expect_expr("p1", result_type="FooPack<char>")
         self.expect_expr("p2", result_type="FooPack<int>")
         self.expect_expr("p3", result_type="FooPack<Foo<int> >")
         self.expect_expr("p4", result_type="FooPack<char, int>")
         self.expect_expr("p5", result_type="FooPack<char, float>")
         self.expect_expr("p6", result_type="FooPack<int, int>")
         self.expect_expr("p7", result_type="FooPack<int, int, int>")
         self.expect_expr("n1", result_type="Foo<int>::Nested<char>")
+        self.expect_expr("n2", result_type="Foo<int>::Nested<ns::Bar>")
 
     @skipIf(compiler=no_match("clang"))
     @skipIf(compiler_version=["<", "15.0"])
     def test_simple_template_names(self):
         self.do_test(dict(CFLAGS_EXTRAS="-gsimple-template-names"))
 
     @skipIf(compiler=no_match("clang"))
     @skipIf(compiler_version=["<", "15.0"])
     def test_no_simple_template_names(self):
         self.do_test(dict(CFLAGS_EXTRAS="-gno-simple-template-names"))

lldb/test/API/lang/cpp/unique-types2/main.cpp

+namespace ns {
+struct Bar {};
+} // namespace ns
+
 template <class T> struct Foo {
   T t;
   template <class U> class Nested {
     U u;
   };
 };
 
 template <class T, class... Ss> class FooPack {
int main() {
   FooPack<int> p2;
   FooPack<Foo<int>> p3;
   FooPack<char, int> p4;
   FooPack<char, float> p5;
   FooPack<int, int> p6;
   FooPack<int, int, int> p7;
 
   Foo<int>::Nested<char> n1;
+  Foo<int>::Nested<ns::Bar> n2;
   // Set breakpoint here
 }