[lldb] Refactor variable paths to support languages with non-pointer "this" (NFC)

[kastiglione]

Summary

frame variable contextually supports accessing ivars via a language's implicit
instance variable, ex this in C++ or self in ObjC.

It has been assumed that the instance variable is a pointer, resulting in this-> or
self-> prefixes. However some languages have a reference based instance variable
instead of a pointer. An example of this is Swift.

This changes DeclContextIsClassMethod and a few of its callers to return (via an out
pointer) whether the instance variable is a pointer or reference. This information is
used in GetValueForVariableExpressionPath to construct a language appropriate
prefix for the ivar.

Some cleanup included:

1. The language parameter wasn't used and has been removed
2. The is_instance_method parameter is redundant and has been removed -- a non-empty instance_var_name indicates the context is an instance method
3. IsClassMethod's parameters have been declared with default values of nullptr
4. Renamed some variables/parameters

[shafik]

lldb/include/lldb/Symbol/CompilerDeclContext.h

77

If we are going to refactor this, this change does not feel very C++y passing around pointers. I know we want a way to call this w/o any arguments but perhaps we can write an overload for that case?

Does instance_var_name_ptr need to be a string? Maybe we can encode it using an enum, we don't have a lot of cases this, self, maybe even not a pointer as well and get ride of instance_is_pointer_ptr.

[kastiglione]

lldb/include/lldb/Symbol/CompilerDeclContext.h

77

Something like?

enum InstanceVariable {
   ThisPointer,
   SelfPointer,
   Self,
};

[teemperor]

lldb/include/lldb/Symbol/CompilerDeclContext.h

77

We could also make this function that is something like llvm::Optional<SelfRef> GetCurrentObjectRef and SelfRef is just ConstString + enum if it's a pointer/ref/whatever.

FWIW, encoding the string inside an enum doesn't seem to fit with the idea that the TypeSystem interface just needs to be implemented (but not extended) when adding a new language plugin (if the language uses a different name like $this or Self then the enum needs to be expanded). Also not sure what use this has to the caller (I don't see how the callers do anything else with this enum then translating it to the actual string and checking if it's a pointer, both are more complicated with an enum).

[kastiglione]

lldb/include/lldb/Symbol/CompilerDeclContext.h

77

I like this approach. Before I make the change, some questions/thoughts.

I'm thinking the second field will be a bool, ex: is_pointer. The reason for bool and not enum is that I don't know if it's worth the complexity of trying to distinguish between reference and value. In Swift, the self variable could be reference (class) or value (struct, enum…).

Instead of SelfRef I'm thinking {This,Self,Instance}Variable, since it's info about the variable (name, pointer-ness).

Do we need to return a ConstString, or can it be const char * and let the caller do what it wants. It seems it will always be a string literal, and const char * is a lower common denominator. I guess I'm ultimately unclear on when, if ever, to not use ConstString?

[jingham]

lldb/include/lldb/Symbol/CompilerDeclContext.h

77

ConstString's main purpose is to hold strings we're likely to compare against a lot. For instance, if you take a symbol name and you are going to look it up everywhere, it's appropriate for that to be a ConstString since we're going to turn it into that anyway to do the searches.

Since a caller is likely to turn around and look up "this" having gotten that name back, a ConstString seems an okay choice. Another way to do this would be to make function statics with ConstStrings for "this" and "self". When you make a ConstString from a c-string we have to hash it look for it in the string pool. Copying a ConstString is just copying a pointer. So if you have just a couple of options, making static ConstStrings makes returning the result cheap. And since ConstString's are all null-terminated C-strings, ConstString -> cstring is cheap.

[aprantl]

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

9505

Another nicer way to implement an API with multiple return values would be to return a struct ClassMethodDescriptor or something like that.

lldb/source/Target/StackFrame.cpp

566

Ideally we would call into the LanguageRuntime here and ask it what the syntax for member access is in this language.

[teemperor]

(Just pushing this back into Dave's review queue)

lldb/include/lldb/Symbol/CompilerDeclContext.h

77

FWIW, the available string types you could use would be:

• llvm::StringRef -> no ownership and cheap
• std::string -> ownership
• ConstString -> Can be very cheap or expensive depending on how you use it.
• const char * -> Nearly always a bad idea (exceptions are stuff that do C interop).

[kastiglione]

This was superseded by D145276.

lldb/include/lldb/Symbol/CompilerDeclContext.h

 //===-- CompilerDeclContext.h -----------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SYMBOL_COMPILERDECLCONTEXT_H
 #define LLDB_SYMBOL_COMPILERDECLCONTEXT_H
 
 #include <vector>
 
 #include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-private.h"
 
 namespace lldb_private {
 
 /// Represents a generic declaration context in a program. A declaration context
 /// is data structure that contains declarations (e.g. namespaces).
 ///
 /// This class serves as an abstraction for a declaration context inside one of
bool IsValid() const {
     return m_type_system != nullptr && m_opaque_decl_ctx != nullptr;
   }
 
   std::vector<CompilerDecl> FindDeclByName(ConstString name,
                                            const bool ignore_using_decls);
 
   /// Checks if this decl context represents a method of a class.
   ///
-  /// \param[out] language_ptr
+  /// \param[out] instance_var_name_ptr
   ///     If non NULL and \b true is returned from this function,
-  ///     this will indicate if the language that respresents the method.
-  ///
-  /// \param[out] is_instance_method_ptr
-  ///     If non NULL and \b true is returned from this function,
-  ///     this will indicate if the method is an instance function (true)
-  ///     or a class method (false indicating the function is static, or
-  ///     doesn't require an instance of the class to be called).
+  ///     this will indicate the implicit variable name for the language
+  ///     like "this" for C++, and "self" for Objective C.
   ///
-  /// \param[out] language_object_name_ptr
+  /// \param[out] instance_is_pointer_ptr
   ///     If non NULL and \b true is returned from this function,
-  ///     this will indicate if implicit object name for the language
-  ///     like "this" for C++, and "self" for Objective C.
+  ///     this will indicate whether the implicit object for the language
+  ///     is a pointer or a reference.
   ///
   /// \return
   ///     Returns true if this is a decl context that represents a method
   ///     in a struct, union or class.
-  bool IsClassMethod(lldb::LanguageType *language_ptr,
-                     bool *is_instance_method_ptr,
-                     ConstString *language_object_name_ptr);
+  bool IsClassMethod(ConstString *instance_var_name_ptr = nullptr,
+                     bool *instance_is_pointer_ptr = nullptr);
 
   /// Check if the given other decl context is contained in the lookup
   /// of this decl context (for example because the other context is a nested
   /// inline namespace).
   ///
   /// @param[in] other
   ///     The other decl context for which we should check if it is contained
   ///     in the lookoup of this context.

lldb/include/lldb/Symbol/SymbolContext.h

 //===-- SymbolContext.h -----------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SYMBOL_SYMBOLCONTEXT_H
 #define LLDB_SYMBOL_SYMBOLCONTEXT_H
 
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "lldb/Core/Address.h"
 #include "lldb/Core/Mangled.h"
 #include "lldb/Symbol/LineEntry.h"
 #include "lldb/Utility/Iterable.h"
 #include "lldb/lldb-private.h"
 
 namespace lldb_private {
 
 class SymbolContextScope;
public:
   /// \return
   ///     The block object pointer that defines the function that is
   ///     represented by this symbol context object, nullptr otherwise.
   Block *GetFunctionBlock();
 
   /// If this symbol context represents a function that is a method, return
   /// true and provide information about the method.
   ///
-  /// \param[out] language
-  ///     If \b true is returned, the language for the method.
-  ///
-  /// \param[out] is_instance_method
-  ///     If \b true is returned, \b true if this is a instance method,
-  ///     \b false if this is a static/class function.
-  ///
-  /// \param[out] language_object_name
+  /// \param[out] instance_var_name
   ///     If \b true is returned, the name of the artificial variable
   ///     for the language ("this" for C++, "self" for ObjC).
   ///
+  /// \param[out] instance_is_pointer
+  ///     If \b true is returned, identifies whether the language's instance
+  ///     variable is a pointer or reference.
+  ///
   /// \return
   ///     \b True if this symbol context represents a function that
   ///     is a method of a class, \b false otherwise.
-  bool GetFunctionMethodInfo(lldb::LanguageType &language,
-                             bool &is_instance_method,
-                             ConstString &language_object_name);
+  bool GetFunctionMethodInfo(ConstString *instance_var_name,
+                             bool *instance_is_pointer);
 
   /// Sorts the types in TypeMap according to SymbolContext to TypeList
   ///
   void SortTypeList(TypeMap &type_map, TypeList &type_list) const;
 
   /// Find a name of the innermost function for the symbol context.
   ///
   /// For instance, if the symbol context contains an inlined block, it will

lldb/include/lldb/Symbol/TypeSystem.h

 #include <string>
 
 #include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/SmallBitVector.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
 
 #include "lldb/Core/PluginInterface.h"
 #include "lldb/Expression/Expression.h"
 #include "lldb/Symbol/CompilerDecl.h"
 #include "lldb/Symbol/CompilerDeclContext.h"
 #include "lldb/lldb-private.h"
 
 class DWARFDIE;
 class DWARFASTParser;
 class PDBASTParser;
public:
   DeclContextFindDeclByName(void *opaque_decl_ctx, ConstString name,
                             const bool ignore_imported_decls);
 
   virtual ConstString DeclContextGetName(void *opaque_decl_ctx) = 0;
 
   virtual ConstString
   DeclContextGetScopeQualifiedName(void *opaque_decl_ctx) = 0;
 
-  virtual bool DeclContextIsClassMethod(
-      void *opaque_decl_ctx, lldb::LanguageType *language_ptr,
-      bool *is_instance_method_ptr, ConstString *language_object_name_ptr) = 0;
+  virtual bool DeclContextIsClassMethod(void *opaque_decl_ctx,
+                                        ConstString *instance_var_name_ptr,
+                                        bool *instance_is_pointer_ptr) = 0;
 
   virtual bool DeclContextIsContainedInLookup(void *opaque_decl_ctx,
                                               void *other_opaque_decl_ctx) = 0;
 
   // Tests
 #ifndef NDEBUG
   /// Verify the integrity of the type to catch CompilerTypes that mix
   /// and match invalid TypeSystem/Opaque type pairs.

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

for (uint32_t index = 0; index < num_indices; ++index) {
     if (!function) {
       sc_sym_list.Append(sym_ctx);
       continue;
     }
     // Filter out functions without declaration contexts, as well as
     // class/instance methods, since they'll be skipped in the code that
     // follows anyway.
     CompilerDeclContext func_decl_context = function->GetDeclContext();
-    if (!func_decl_context ||
-        func_decl_context.IsClassMethod(nullptr, nullptr, nullptr))
+    if (!func_decl_context || func_decl_context.IsClassMethod())
       continue;
     // We can only prune functions for which we can copy the type.
     CompilerType func_clang_type = function->GetType()->GetFullCompilerType();
     CompilerType copied_func_type = GuardedCopyType(func_clang_type);
     if (!copied_func_type) {
       sc_sym_list.Append(sym_ctx);
       continue;
     }
for (uint32_t index = 0, num_indices = sc_list.GetSize();
 
       if (sym_ctx.function) {
         CompilerDeclContext decl_ctx = sym_ctx.function->GetDeclContext();
 
         if (!decl_ctx)
           continue;
 
         // Filter out class/instance methods.
-        if (decl_ctx.IsClassMethod(nullptr, nullptr, nullptr))
+        if (decl_ctx.IsClassMethod())
           continue;
 
         AddOneFunction(context, sym_ctx.function, nullptr);
         context.m_found_function_with_type_info = true;
         context.m_found_function = true;
       } else if (sym_ctx.symbol) {
         if (sym_ctx.symbol->GetType() == eSymbolTypeReExported && target) {
           sym_ctx.symbol = sym_ctx.symbol->ResolveReExportedSymbol(*target);

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

 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTFwd.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/Basic/TargetInfo.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/SmallVector.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangPersistentVariables.h"
 #include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Symbol/CompilerType.h"
 #include "lldb/Symbol/TypeSystem.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Flags.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Logging.h"
public:
   DeclContextFindDeclByName(void *opaque_decl_ctx, ConstString name,
                             const bool ignore_using_decls) override;
 
   ConstString DeclContextGetName(void *opaque_decl_ctx) override;
 
   ConstString DeclContextGetScopeQualifiedName(void *opaque_decl_ctx) override;
 
   bool DeclContextIsClassMethod(void *opaque_decl_ctx,
-                                lldb::LanguageType *language_ptr,
-                                bool *is_instance_method_ptr,
-                                ConstString *language_object_name_ptr) override;
+                                ConstString *instance_var_name_ptr,
+                                bool *instance_is_pointer_ptr) override;
 
   bool DeclContextIsContainedInLookup(void *opaque_decl_ctx,
                                       void *other_opaque_decl_ctx) override;
 
   // Clang specific clang::DeclContext functions
 
   static clang::DeclContext *
   DeclContextGetAsDeclContext(const CompilerDeclContext &dc);

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

clang::NamedDecl *named_decl =
         llvm::dyn_cast<clang::NamedDecl>((clang::DeclContext *)opaque_decl_ctx);
     if (named_decl)
       return ConstString(GetTypeNameForDecl(named_decl));
   }
   return ConstString();
 }
 
 bool TypeSystemClang::DeclContextIsClassMethod(
-    void *opaque_decl_ctx, lldb::LanguageType *language_ptr,
-    bool *is_instance_method_ptr, ConstString *language_object_name_ptr) {
+    void *opaque_decl_ctx, ConstString *instance_var_name_ptr,
+    bool *instance_is_pointer_ptr) {
   if (opaque_decl_ctx) {
     clang::DeclContext *decl_ctx = (clang::DeclContext *)opaque_decl_ctx;
     if (ObjCMethodDecl *objc_method =
             llvm::dyn_cast<clang::ObjCMethodDecl>(decl_ctx)) {
-      if (is_instance_method_ptr)
-        *is_instance_method_ptr = objc_method->isInstanceMethod();
-      if (language_ptr)
-        *language_ptr = eLanguageTypeObjC;
-      if (language_object_name_ptr)
-        language_object_name_ptr->SetCString("self");
+      if (objc_method->isInstanceMethod()) {
+        if (instance_var_name_ptr)
+          instance_var_name_ptr->SetCString("self");
+        if (instance_is_pointer_ptr)
+          *instance_is_pointer_ptr = true;
+      }
       return true;
     } else if (CXXMethodDecl *cxx_method =
                    llvm::dyn_cast<clang::CXXMethodDecl>(decl_ctx)) {
-      if (is_instance_method_ptr)
-        *is_instance_method_ptr = cxx_method->isInstance();
-      if (language_ptr)
-        *language_ptr = eLanguageTypeC_plus_plus;
-      if (language_object_name_ptr)
-        language_object_name_ptr->SetCString("this");
+      if (cxx_method->isInstance()) {
+        if (instance_var_name_ptr)
+          instance_var_name_ptr->SetCString("this");
+        if (instance_is_pointer_ptr)
+          *instance_is_pointer_ptr = true;
+      }
       return true;
     } else if (clang::FunctionDecl *function_decl =
                    llvm::dyn_cast<clang::FunctionDecl>(decl_ctx)) {
       ClangASTMetadata *metadata = GetMetadata(function_decl);
       if (metadata && metadata->HasObjectPtr()) {
-        if (is_instance_method_ptr)
-          *is_instance_method_ptr = true;
-        if (language_ptr)
-          *language_ptr = eLanguageTypeObjC;
-        if (language_object_name_ptr)
-          language_object_name_ptr->SetCString(metadata->GetObjectPtrName());
+        if (instance_var_name_ptr)
+          instance_var_name_ptr->SetCString(metadata->GetObjectPtrName());
+        if (instance_is_pointer_ptr)
+          *instance_is_pointer_ptr = true;
         return true;
       }
     }
   }
   return false;
 }
 
 bool TypeSystemClang::DeclContextIsContainedInLookup(

lldb/source/Symbol/CompilerDeclContext.cpp

 //===-- CompilerDeclContext.cpp -------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Symbol/CompilerDeclContext.h"
 #include "lldb/Symbol/CompilerDecl.h"
 #include "lldb/Symbol/TypeSystem.h"
 #include <vector>
 
 using namespace lldb_private;
 
 std::vector<CompilerDecl>
 CompilerDeclContext::FindDeclByName(ConstString name,

 }
 
 ConstString CompilerDeclContext::GetScopeQualifiedName() const {
   if (IsValid())
     return m_type_system->DeclContextGetScopeQualifiedName(m_opaque_decl_ctx);
   return ConstString();
 }
 
-bool CompilerDeclContext::IsClassMethod(lldb::LanguageType *language_ptr,
-                                        bool *is_instance_method_ptr,
-                                        ConstString *language_object_name_ptr) {
+bool CompilerDeclContext::IsClassMethod(ConstString *instance_var_name_ptr,
+                                        bool *instance_is_pointer_ptr) {
   if (IsValid())
     return m_type_system->DeclContextIsClassMethod(
-        m_opaque_decl_ctx, language_ptr, is_instance_method_ptr,
-        language_object_name_ptr);
+        m_opaque_decl_ctx, instance_var_name_ptr, instance_is_pointer_ptr);
   return false;
 }
 
 bool CompilerDeclContext::IsContainedInLookup(CompilerDeclContext other) const {
   if (!IsValid())
     return false;
 
   // If the other context is just the current context, we don't need to go

lldb/source/Symbol/SymbolContext.cpp

 //===-- SymbolContext.cpp -------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Symbol/SymbolContext.h"
 
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/StringConvert.h"
 #include "lldb/Symbol/Block.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/ObjectFile.h"
if (function) {
     // There is no block information in this symbol context, so we must assume
     // that the block that is desired is the top level block of the function
     // itself.
     return &function->GetBlock(true);
   }
   return nullptr;
 }
 
-bool SymbolContext::GetFunctionMethodInfo(lldb::LanguageType &language,
-                                          bool &is_instance_method,
-                                          ConstString &language_object_name)
-
-{
+bool SymbolContext::GetFunctionMethodInfo(ConstString *instance_var_name,
+                                          bool *instance_is_pointer) {
   Block *function_block = GetFunctionBlock();
   if (function_block) {
     CompilerDeclContext decl_ctx = function_block->GetDeclContext();
     if (decl_ctx)
-      return decl_ctx.IsClassMethod(&language, &is_instance_method,
-                                    &language_object_name);
+      return decl_ctx.IsClassMethod(instance_var_name, instance_is_pointer);
   }
   return false;
 }
 
 void SymbolContext::SortTypeList(TypeMap &type_map, TypeList &type_list) const {
   Block *curr_block = block;
   bool isInlinedblock = false;
   if (curr_block != nullptr &&

lldb/source/Target/StackFrame.cpp

 //===-- StackFrame.cpp ----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Disassembler.h"
 #include "lldb/Core/FormatEntity.h"
 #include "lldb/Core/Mangled.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Core/ValueObjectConstResult.h"
 #include "lldb/Core/ValueObjectMemory.h"
ValueObjectSP StackFrame::GetValueForVariableExpressionPath(
   if (var_sp) {
     var_expr = var_expr.drop_front(name_const_string.GetLength());
   }
 
   if (!var_sp && (options & eExpressionPathOptionsAllowDirectIVarAccess)) {
     // Check for direct ivars access which helps us with implicit access to
     // ivars with the "this->" or "self->"
     GetSymbolContext(eSymbolContextFunction | eSymbolContextBlock);
-    lldb::LanguageType method_language = eLanguageTypeUnknown;
-    bool is_instance_method = false;
-    ConstString method_object_name;
-    if (m_sc.GetFunctionMethodInfo(method_language, is_instance_method,
-                                   method_object_name)) {
-      if (is_instance_method && method_object_name) {
-        var_sp = variable_list->FindVariable(method_object_name);
+    ConstString instance_var_name;
+    bool instance_is_pointer = true;
+    if (m_sc.GetFunctionMethodInfo(&instance_var_name, &instance_is_pointer)) {
+      if (instance_var_name) {
+        var_sp = variable_list->FindVariable(instance_var_name);
         if (var_sp) {
           separator_idx = 0;
+          if (instance_is_pointer)
             var_expr_storage = "->";
+          else
+            var_expr_storage = ".";
           var_expr_storage += var_expr;
           var_expr = var_expr_storage;
           synthetically_added_instance_object = true;
         }
       }
     }
   }