Merge pull request #8579 from Michael137/lldb/type-completion-rework/lazy-method-loading

Note, this is a no-op when the LLDB setting
`plugin.typesystem.clang.experimental-redecl-completion` is not set (which is currently the default). So this patch has no effect unless the user explicitly opts into it.

The type-completion rework (aka redecl-completion) implemented in #8222 comes with a large performance penalty, since we now eagerly complete `RecordType`s. Completing a `RecordType` previously unconditionally resolved all member functions of said type. With redecl-completion completion, however, this meant we were now pulling in many more definitions than needed. Without redecl-completion, this isn't a problem, since importing method parameters is cheap (they are imported minimally), so we wouldn't notice that we always resolved all member functions.

This patch tries to load methods lazily when in redecl-completion mode. We do this by introducing a new `ExternalASTSource::FindExternalVisibleMethods` API which Clang calls when parsing a member access expression. The callback into LLDB will do a `FindFunctions` call for all methods with the method name we're looking for, filters out any mismatches, and lets Clang continue with its parsing. We still load following methods eagerly:
1. virtual functions: currently overrides are resolved in `CompleteRecordType`
2. operators: currently I couldn't find a point at which Clang can call into LLDB here to facilitate lazy loading
3. ctors/dtors: same reason as (2)

In our benchmark harness, we saw this patch bring down redecl-completion expression evaluation performance on-par with top-of-tree expression evaluation.

Author: Michael137

clang/include/clang/AST/ExternalASTSource.h

@@ -150,6 +150,11 @@ class ExternalASTSource : public RefCountedBase<ExternalASTSource> {
   virtual bool
   FindExternalVisibleDeclsByName(const DeclContext *DC, DeclarationName Name);
 
+  virtual bool FindExternalVisibleMethodsByName(const DeclContext *DC,
+                                                DeclarationName Name) {
+    return false;
+  }
+
   /// Ensures that the table of all visible declarations inside this
   /// context is up to date.
   ///

clang/lib/Sema/SemaExprMember.cpp

@@ -685,6 +685,13 @@ static bool LookupMemberExprInRecord(Sema &SemaRef, LookupResult &R,
     }
   }
 
+  if (ExternalASTSource *Source =
+          DC->getParentASTContext().getExternalSource()) {
+    if (auto LookupName = R.getLookupName()) {
+      Source->FindExternalVisibleMethodsByName(DC, LookupName);
+    }
+  }
+
   // The record definition is complete, now look up the member.
   SemaRef.LookupQualifiedName(R, DC, SS);
 

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

@@ -11,6 +11,7 @@
 #include "ClangDeclVendor.h"
 #include "ClangModulesDeclVendor.h"
 
+#include "NameSearchContext.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Symbol/CompilerDeclContext.h"
@@ -21,6 +22,7 @@
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclCXX.h"
 #include "clang/Basic/SourceManager.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangUtil.h"
@@ -615,6 +617,125 @@ bool ClangASTSource::IgnoreName(const ConstString name,
          name_string_ref.startswith("_$");
 }
 
+bool ClangASTSource::FindExternalVisibleMethodsByName(
+    const clang::DeclContext *DC, clang::DeclarationName Name) {
+  if (!TypeSystemClang::UseRedeclCompletion())
+    return true;
+
+  SmallVector<clang::NamedDecl *> decls;
+  NameSearchContext context(*m_clang_ast_context, decls, Name, DC);
+  FindExternalVisibleMethods(context);
+
+  return true;
+}
+
+void ClangASTSource::FindExternalVisibleMethods(NameSearchContext &context) {
+  assert(m_ast_context);
+
+  const ConstString name(context.m_decl_name.getAsString().c_str());
+  CompilerDeclContext namespace_decl;
+  FindExternalVisibleMethods(context, lldb::ModuleSP(), namespace_decl);
+}
+
+bool ClangASTSource::CompilerDeclContextsMatch(
+    CompilerDeclContext candidate_decl_ctx, DeclContext const *context,
+    TypeSystemClang &ts) {
+  auto CDC1 = candidate_decl_ctx.GetTypeSystem()->DeclContextGetCompilerContext(
+      candidate_decl_ctx.GetOpaqueDeclContext());
+
+  // Member functions have at least 2 entries (1
+  // for method name, 1 for parent class)
+  assert(CDC1.size() > 1);
+
+  // drop last entry (which is function name)
+  CDC1.pop_back();
+
+  const auto CDC2 = ts.DeclContextGetCompilerContext(
+      const_cast<clang::DeclContext *>(context));
+
+  // Quick by-name check of the entire context hierarchy.
+  if (CDC1 == CDC2)
+    return true;
+
+  // Otherwise, check whether the 'candidate_decl_ctx' is a base class of
+  // 'context'.
+  auto const *candidate_context =
+      (static_cast<clang::DeclContext *>(
+           candidate_decl_ctx.GetOpaqueDeclContext()))
+          ->getParent();
+
+  auto const *candidate_cxx_record =
+      dyn_cast<clang::CXXRecordDecl>(candidate_context);
+  if (!candidate_cxx_record)
+    return false;
+
+  auto const *cxx_record = dyn_cast<clang::CXXRecordDecl>(context);
+  if (!cxx_record)
+    return false;
+
+  // cxx_record comes from user expression AST. So we need to get origin
+  // to compare against candidate_context.
+  auto orig = GetDeclOrigin(cxx_record);
+  if (!orig.Valid())
+    return false;
+
+  if (llvm::cast<CXXRecordDecl>(orig.decl)->isDerivedFrom(candidate_cxx_record))
+    return true;
+
+  return false;
+}
+
+void ClangASTSource::FindExternalVisibleMethods(
+    NameSearchContext &context, lldb::ModuleSP module_sp,
+    CompilerDeclContext &namespace_decl) {
+  assert(m_ast_context);
+
+  SymbolContextList sc_list;
+  const ConstString name(context.m_decl_name.getAsString().c_str());
+  if (!m_target)
+    return;
+
+  if (context.m_found_type)
+    return;
+
+  ModuleFunctionSearchOptions function_options;
+  function_options.include_inlines = false;
+  function_options.include_symbols = false;
+  m_target->GetImages().FindFunctions(name, lldb::eFunctionNameTypeMethod,
+                                      function_options, sc_list);
+
+  auto num_matches = sc_list.GetSize();
+  if (num_matches == 0)
+    return;
+
+  for (const SymbolContext &sym_ctx : sc_list) {
+    assert(sym_ctx.function);
+    CompilerDeclContext decl_ctx = sym_ctx.function->GetDeclContext();
+    if (!decl_ctx)
+      continue;
+
+    assert(decl_ctx.IsClassMethod());
+
+    if (!CompilerDeclContextsMatch(decl_ctx, context.m_decl_context,
+                                   context.m_clang_ts))
+      continue;
+
+    clang::CXXMethodDecl *src_method = llvm::cast<CXXMethodDecl>(
+        static_cast<clang::DeclContext *>(decl_ctx.GetOpaqueDeclContext()));
+    Decl *copied_decl = CopyDecl(src_method);
+
+    if (!copied_decl)
+      continue;
+
+    CXXMethodDecl *copied_method_decl = dyn_cast<CXXMethodDecl>(copied_decl);
+
+    if (!copied_method_decl)
+      continue;
+
+    context.AddNamedDecl(copied_method_decl);
+  }
+}
+
 void ClangASTSource::FindExternalVisibleDecls(
     NameSearchContext &context, lldb::ModuleSP module_sp,
     CompilerDeclContext &namespace_decl) {

lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.h

@@ -87,6 +87,9 @@ class ClangASTSource : public ImporterBackedASTSource,
   bool FindExternalVisibleDeclsByName(const clang::DeclContext *DC,
                                       clang::DeclarationName Name) override;
 
+  bool FindExternalVisibleMethodsByName(const clang::DeclContext *DC,
+                                        clang::DeclarationName Name) override;
+
   /// Enumerate all Decls in a given lexical context.
   ///
   /// \param[in] DC
@@ -197,6 +200,7 @@ class ClangASTSource : public ImporterBackedASTSource,
   /// \param[in] context
   ///     The NameSearchContext to use when filing results.
   virtual void FindExternalVisibleDecls(NameSearchContext &context);
+  virtual void FindExternalVisibleMethods(NameSearchContext &context);
 
   clang::Sema *getSema();
 
@@ -219,6 +223,12 @@ class ClangASTSource : public ImporterBackedASTSource,
       return m_original.FindExternalVisibleDeclsByName(DC, Name);
     }
 
+    bool
+    FindExternalVisibleMethodsByName(const clang::DeclContext *DC,
+                                     clang::DeclarationName Name) override {
+      return m_original.FindExternalVisibleMethodsByName(DC, Name);
+    }
+
     void FindExternalLexicalDecls(
         const clang::DeclContext *DC,
         llvm::function_ref<bool(clang::Decl::Kind)> IsKindWeWant,
@@ -288,6 +298,9 @@ class ClangASTSource : public ImporterBackedASTSource,
   void FindExternalVisibleDecls(NameSearchContext &context,
                                 lldb::ModuleSP module,
                                 CompilerDeclContext &namespace_decl);
+  void FindExternalVisibleMethods(NameSearchContext &context,
+                                  lldb::ModuleSP module,
+                                  CompilerDeclContext &namespace_decl);
 
   /// Find all Objective-C methods matching a given selector.
   ///
@@ -364,6 +377,10 @@ class ClangASTSource : public ImporterBackedASTSource,
       NameSearchContext &context,
       DeclFromUser<const clang::ObjCInterfaceDecl> &origin_iface_decl);
 
+  bool CompilerDeclContextsMatch(CompilerDeclContext candidate_decl_ctx,
+                                 clang::DeclContext const *context,
+                                 TypeSystemClang &ts);
+
 protected:
   bool FindObjCMethodDeclsWithOrigin(
       NameSearchContext &context,

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

@@ -1341,6 +1341,13 @@ void ClangExpressionDeclMap::LookupFunction(
   }
 }
 
+void ClangExpressionDeclMap::FindExternalVisibleMethods(
+    NameSearchContext &context) {
+  assert(m_ast_context);
+
+  ClangASTSource::FindExternalVisibleMethods(context);
+}
+
 void ClangExpressionDeclMap::FindExternalVisibleDecls(
     NameSearchContext &context, lldb::ModuleSP module_sp,
     const CompilerDeclContext &namespace_decl) {

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

@@ -18,6 +18,7 @@
 #include "ClangASTSource.h"
 #include "ClangExpressionVariable.h"
 
+#include "Plugins/ExpressionParser/Clang/NameSearchContext.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Expression/Materializer.h"
 #include "lldb/Symbol/SymbolContext.h"
@@ -266,6 +267,7 @@ class ClangExpressionDeclMap : public ClangASTSource {
   /// \param[in] context
   ///     The NameSearchContext that can construct Decls for this name.
   void FindExternalVisibleDecls(NameSearchContext &context) override;
+  void FindExternalVisibleMethods(NameSearchContext &context) override;
 
   /// Find all entities matching a given name in a given module/namespace,
   /// using a NameSearchContext to make Decls for them.

lldb/source/Plugins/ExpressionParser/Clang/ClangExternalASTSourceCallbacks.h

@@ -37,6 +37,11 @@ class ClangExternalASTSourceCallbacks : public ImporterBackedASTSource {
   bool FindExternalVisibleDeclsByName(const clang::DeclContext *DC,
                                       clang::DeclarationName Name) override;
 
+  bool FindExternalVisibleMethodsByName(const clang::DeclContext *DC,
+                                        clang::DeclarationName Name) override {
+    return false;
+  }
+
   void CompleteType(clang::TagDecl *tag_decl) override;
 
   void CompleteType(clang::ObjCInterfaceDecl *objc_decl) override;

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

@@ -2246,8 +2246,34 @@ bool DWARFASTParserClang::CompleteRecordType(const DWARFDIE &die,
                       default_accessibility, layout_info);
 
     // Now parse any methods if there were any...
-    for (const DWARFDIE &die : member_function_dies)
-      dwarf->ResolveType(die);
+    for (const DWARFDIE &mem : member_function_dies) {
+      if (!TypeSystemClang::UseRedeclCompletion() ||
+          type_is_objc_object_or_interface) {
+        dwarf->ResolveType(mem);
+        continue;
+      }
+
+      ConstString mem_name(mem.GetName());
+      ConstString die_name(die.GetName());
+      const bool is_ctor = mem_name == die_name;
+      const bool is_virtual_method =
+          mem.GetAttributeValueAsUnsigned(
+              DW_AT_virtuality, DW_VIRTUALITY_none) > DW_VIRTUALITY_none;
+      const bool is_operator = mem_name.GetStringRef().starts_with("operator");
+      const bool is_static_method =
+          mem.GetFirstChild().GetAttributeValueAsUnsigned(DW_AT_artificial,
+                                                          0) == 0;
+
+      // FIXME: With RedeclCompletion, we currently don't have a good
+      // way to call `FindExternalVisibleMethods` from Clang
+      // for static functions, constructors or operators.
+      // So resolve them now.
+      //
+      // We want to resolve virtual methods now too because
+      // we set the method overrides below.
+      if (is_ctor || is_operator || is_virtual_method || is_static_method)
+        dwarf->ResolveType(mem);
+    }
 
     if (type_is_objc_object_or_interface) {
       ConstString class_name(clang_type.GetTypeName());

lldb/test/API/commands/expression/completion/TestExprCompletion.py

@@ -13,6 +13,7 @@
 class CommandLineExprCompletionTestCase(TestBase):
     NO_DEBUG_INFO_TESTCASE = True
 
+    @expectedFailureAll(setting=('plugin.typesystem.clang.experimental-redecl-completion', 'true'))
     def test_expr_completion(self):
         self.build()
         self.main_source = "main.cpp"

lldb/test/API/commands/expression/context-object/TestContextObject.py

@@ -5,9 +5,29 @@
 import lldb
 import lldbsuite.test.lldbutil as lldbutil
 from lldbsuite.test.lldbtest import *
-
+from lldbsuite.test.decorators import *
 
 class ContextObjectTestCase(TestBase):
+    @expectedFailureAll(setting=('plugin.typesystem.clang.experimental-redecl-completion', 'true'))
+    def test_context_object_eval_function(self):
+        """Tests expression evaluation of functions in context of an object."""
+        self.build()
+
+        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+            self, "// Break here", self.main_source_spec
+        )
+        frame = thread.GetFrameAtIndex(0)
+        for obj in "cpp_struct", "cpp_struct_ref":
+            obj_val = frame.FindVariable(obj)
+            self.assertTrue(obj_val.IsValid())
+
+            # Test functions evaluation
+            value = obj_val.EvaluateExpression("function()")
+            self.assertTrue(value.IsValid())
+            self.assertSuccess(value.GetError())
+            self.assertEqual(value.GetValueAsSigned(), 2222)
+
+
     def test_context_object(self):
         """Tests expression evaluation in context of an object."""
         self.build()
@@ -35,12 +55,6 @@ def test_context_object(self):
             self.assertSuccess(value.GetError())
             self.assertEqual(value.GetValueAsSigned(), 1111)
 
-            # Test functions evaluation
-            value = obj_val.EvaluateExpression("function()")
-            self.assertTrue(value.IsValid())
-            self.assertSuccess(value.GetError())
-            self.assertEqual(value.GetValueAsSigned(), 2222)
-
             # Test that we retrieve the right global
             value = obj_val.EvaluateExpression("global.field")
             self.assertTrue(value.IsValid())

lldb/test/API/commands/expression/import-std-module/empty-module/TestEmptyStdModule.py

@@ -15,6 +15,7 @@ class ImportStdModule(TestBase):
     @add_test_categories(["libc++"])
     @skipIfRemote
     @skipIf(compiler=no_match("clang"))
+    @expectedFailureAll(setting=('plugin.typesystem.clang.experimental-redecl-completion', 'true'))
     def test(self):
         self.build()
 

lldb/test/API/commands/expression/pr35310/TestExprsBug35310.py

@@ -12,6 +12,7 @@ def setUp(self):
         self.main_source = "main.cpp"
         self.main_source_spec = lldb.SBFileSpec(self.main_source)
 
+    @expectedFailureAll(setting=('plugin.typesystem.clang.experimental-redecl-completion', 'true'))
     def test_issue35310(self):
         """Test invoking functions with non-standard linkage names.
 

lldb/test/API/lang/cpp/function-ref-qualifiers/TestCppFunctionRefQualifiers.py

@@ -34,11 +34,11 @@ def test(self):
         self.expect_expr("Foo{}.func()", result_type="int64_t", result_value="3")
 
         self.filecheck("target modules dump ast", __file__)
-        # CHECK:      |-CXXMethodDecl {{.*}} func 'uint32_t () const &'
-        # CHECK-NEXT: | `-AsmLabelAttr {{.*}}
-        # CHECK-NEXT: |-CXXMethodDecl {{.*}} func 'int64_t () const &&'
-        # CHECK-NEXT: | `-AsmLabelAttr {{.*}}
-        # CHECK-NEXT: |-CXXMethodDecl {{.*}} func 'uint32_t () &'
-        # CHECK-NEXT: | `-AsmLabelAttr {{.*}}
-        # CHECK-NEXT: `-CXXMethodDecl {{.*}} func 'int64_t () &&'
-        # CHECK-NEXT:   `-AsmLabelAttr {{.*}}
+        # CHECK-DAG: CXXMethodDecl {{.*}} func 'uint32_t () const &'
+        # CHECK-DAG: `-AsmLabelAttr {{.*}}
+        # CHECK-DAG: CXXMethodDecl {{.*}} func 'int64_t () const &&'
+        # CHECK-DAG: `-AsmLabelAttr {{.*}}
+        # CHECK-DAG: CXXMethodDecl {{.*}} func 'uint32_t () &'
+        # CHECK-DAG: `-AsmLabelAttr {{.*}}
+        # CHECK-DAG: CXXMethodDecl {{.*}} func 'int64_t () &&'
+        # CHECK-DAG: `-AsmLabelAttr {{.*}}

lldb/test/API/python_api/class_members/TestSBTypeClassMembers.py

@@ -19,6 +19,7 @@ def setUp(self):
         self.source = "main.mm"
         self.line = line_number(self.source, "// set breakpoint here")
 
+    @expectedFailureAll(setting=('plugin.typesystem.clang.experimental-redecl-completion', 'true'))
     @skipUnlessDarwin
     def test(self):
         """Test SBType APIs to fetch member function types."""