[Symbol] Add TypeSystem::GetClassName

[xiaobai]

Summary

This will generalize some behavior in ValueObject. Although this code
path is still specific to C++, it no longer references ClangASTContext directly.
This will allow us to move ClangASTContext out of Symbol in the near future.

[shafik]

As far as I can tell we don't have coverage for GetBaseClassPath it would be good to add it while we are refactoring this code.

[teemperor]

Some minor comments to make this more NFC.

lldb/source/Core/ValueObject.cpp

2023

Can't you just call GetCompilerType().GetTypeSystem()? This way this whole code isn't C++ specific and much shorter. It also fixes that we end up in the Scratch type system for a language when the CompilerType might not even be from that TypeSystem (but from another TypeSystem for the same language).

lldb/source/Symbol/ClangASTContext.cpp

3423

ClangUtil::IsClangType(compiler_type) seems like a more complete check. Then you can also drop the lang_type parameter.

[xiaobai]

Address feedback from @teemperor

[jingham]

One comment on a part you didn't change, but left a C++ specific track in the generic ValueObject code.

lldb/source/Core/ValueObject.cpp

2037

That's still implicitly C. Other languages (swift) use "." for the same purposes. If this is really language agnostic we either need a "GetSeparator" function, or we need s "ComposePath" operation from the language.

[xiaobai]

lldb/source/Core/ValueObject.cpp

2037

Yep, it is. We would need to get the language plugin for whatever we're dealing with. There might be a decent way to get to that by getting the LanguageType from the CompilerType, and trying to find a plugin for that (and some default separator for languages that don't have a plugin). Definitely worth doing.

I think adding a "GetClassSeparator" method in this patch would stray from the purpose of this patch, so I'd like to do that in a follow-up if possible.

[jingham]

lldb/source/Core/ValueObject.cpp

2037

FBM, but can you at least leave a FIXME here so if you don't get to the follow-up right away it's clear this still needs to be done?

[xiaobai]

Add fixme

[xiaobai]

Rebase

[JDevlieghere]

LGTM

[clayborg]

Let me know what everyone thinks of adding a "fully_qualified" argument to the TypeSystem::GetClassName()?

lldb/include/lldb/Symbol/TypeSystem.h

200–201

Maybe add an extra argument to get a fully qualified class name?

/// \param fully_qualified Get the the class basename if false, or a fully qualified name if true
virtual llvm::Optional<std::string>
  GetClassName(const CompilerType &compiler_type, bool fully_qualified) = 0;

Then the code in the dumper doesn't need to use "::" for all languages in ValueObject::GetBaseClassPath

lldb/source/Core/ValueObject.cpp

2032–2042

bool fully_qualified = true;
llvm::Optional<std::string> class_name = type_system->GetClassName(GetCompilerType(), fully_qualified);
if (class_name)
  s.PutCString(class_name.getValue());

[xiaobai]

In D69820#1817621, @clayborg wrote:

Let me know what everyone thinks of adding a "fully_qualified" argument to the TypeSystem::GetClassName()?

I think that you would have to add more than a fully_qualified argument to the method. You need to reference the parent ValueObject to fully qualify something as far as I can tell, so you'd have to pass along the parent as well. I think that it sounds nice for the TypeSystem to fully qualify the class name but I don't think that it's necessarily better than what exists now. You'll still have recursion walking the parent ValueObjects, you've just split it across two different methods in different classes. Maybe I'm overthinking this or missing something though?

[clayborg]

Or do we

In D69820#1817768, @xiaobai wrote:

In D69820#1817621, @clayborg wrote:

Let me know what everyone thinks of adding a "fully_qualified" argument to the TypeSystem::GetClassName()?

I think that you would have to add more than a fully_qualified argument to the method. You need to reference the parent ValueObject to fully qualify something as far as I can tell, so you'd have to pass along the parent as well. I think that it sounds nice for the TypeSystem to fully qualify the class name but I don't think that it's necessarily better than what exists now. You'll still have recursion walking the parent ValueObjects, you've just split it across two different methods in different classes. Maybe I'm overthinking this or missing something though?

Just for the type I am saying. Get the fully qualified name of the type. That should be all built into the type in the type system as well right? So no other value would need to be used, just ask the CompilerType for its fully qualified name.

[clayborg]

if this wasn't just getting a fully qualified class name then ignore my comments.

[labath]

lldb/include/lldb/Symbol/TypeSystem.h

201

I'm confused by the CompilerType argument here. It seems odd as the CompilerType object already contains a type system. Though we seem to have some precedent for this, the majority of TypeSystem functions seems to take an opaque_compiler_type_t (and this seems reasonable to me -- the user would call CompilerType::GetClassName, which would then forward the call to m_type_system->GetClassName(m_opaque_ptr)...).

[xiaobai]

Made obsolete by D73517

lldb/include/lldb/Symbol/ClangASTContext.h

public:
   static llvm::Optional<std::string> GetCXXClassName(const CompilerType &type);
 
   // Type Completion
 
   bool GetCompleteType(lldb::opaque_compiler_type_t type) override;
 
   // Accessors
 
+  llvm::Optional<std::string>
+  GetClassName(const CompilerType &compiler_type) override;
+
   ConstString GetTypeName(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;
 

lldb/include/lldb/Symbol/TypeSystem.h

public:
   virtual bool GetCompleteType(lldb::opaque_compiler_type_t type) = 0;
 
   // AST related queries
 
   virtual uint32_t GetPointerByteSize() = 0;
 
   // Accessors
 
+  virtual llvm::Optional<std::string>
+  GetClassName(const CompilerType &compiler_type) = 0;
+
   virtual ConstString GetTypeName(lldb::opaque_compiler_type_t type) = 0;
 
   virtual uint32_t
   GetTypeInfo(lldb::opaque_compiler_type_t type,
               CompilerType *pointee_or_element_compiler_type) = 0;
 
   virtual lldb::LanguageType
   GetMinimumLanguage(lldb::opaque_compiler_type_t type) = 0;

lldb/source/Core/ValueObject.cpp

 #include "lldb/DataFormatters/DumpValueObjectOptions.h"
 #include "lldb/DataFormatters/FormatManager.h"
 #include "lldb/DataFormatters/StringPrinter.h"
 #include "lldb/DataFormatters/TypeFormat.h"
 #include "lldb/DataFormatters/TypeSummary.h"
 #include "lldb/DataFormatters/ValueObjectPrinter.h"
 #include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Host/Config.h"
-#include "lldb/Symbol/ClangASTContext.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/CompilerType.h"
 #include "lldb/Symbol/Declaration.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Symbol/Type.h"
 #include "lldb/Symbol/Variable.h"
 #include "lldb/Target/ExecutionContext.h"
 #include "lldb/Target/Language.h"
if (m_synthetic_children_sp.get() == nullptr)
     return false;
 
   CalculateSyntheticValue(true);
 
   return m_synthetic_value != nullptr;
 }
 
 bool ValueObject::GetBaseClassPath(Stream &s) {
-  if (IsBaseClass()) {
-    bool parent_had_base_class =
-        GetParent() && GetParent()->GetBaseClassPath(s);
-    CompilerType compiler_type = GetCompilerType();
-    llvm::Optional<std::string> cxx_class_name =
-        ClangASTContext::GetCXXClassName(compiler_type);
-    if (cxx_class_name) {
-      if (parent_had_base_class)
-        s.PutCString("::");
-      s.PutCString(cxx_class_name.getValue());
-    }
-    return parent_had_base_class || cxx_class_name;
-  }
-  return false;
+  if (!IsBaseClass())
+    return false;
+
+  TargetSP target_sp = GetTargetSP();
+  if (!target_sp)
+    return false;
+
+  auto *type_system = GetCompilerType().GetTypeSystem();
+  if (!type_system) {
+    LLDB_LOG(
+        lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_EXPRESSIONS),
+        "Unable to get TypeSystem in order to get class name in "
+        "ValueObject::GetBaseClassPath");
+    return false;
+  }
+
+  llvm::Optional<std::string> class_name =
+      type_system->GetClassName(GetCompilerType());
+  bool parent_had_base_class = GetParent() && GetParent()->GetBaseClassPath(s);
+  if (class_name) {
+      // FIXME: This is still specific to C++. We should implement something
+      // like `GetClassSeparator` in the Language plugins to figure out what to
+      // use here.
+    s.PutCString(class_name.getValue());
+  return parent_had_base_class || class_name;
 }
 
 ValueObject *ValueObject::GetNonBaseClassParent() {
   if (GetParent()) {
     if (GetParent()->IsBaseClass())
       return GetParent()->GetNonBaseClassParent();
     else
       return GetParent();

lldb/source/Symbol/ClangASTContext.cpp

bool ClangASTContext::CanPassInRegisters(const CompilerType &type) {
   return false;
 }
 
 bool ClangASTContext::SupportsLanguage(lldb::LanguageType language) {
   return ClangASTContextSupportsLanguage(language);
 }
 
 Optional<std::string>
+ClangASTContext::GetClassName(const CompilerType &compiler_type) {
+  if (!ClangUtil::IsClangType(compiler_type))
+    return llvm::None;
+  return GetCXXClassName(compiler_type);
+}
+
+Optional<std::string>
 ClangASTContext::GetCXXClassName(const CompilerType &type) {
   if (!type)
     return llvm::None;
 
   clang::QualType qual_type(ClangUtil::GetCanonicalQualType(type));
   if (qual_type.isNull())
     return llvm::None;