[Lldb-commits] [lldb] 019bd64 - [lldb] Don't complete ObjCInterfaceDecls in ClangExternalASTSourceCallbacks::FindExternalVisibleDeclsByName

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-05-27T12:39:24+02:00
New Revision: 019bd6485c52a62c008eacfdf0d13a26ca6b0a6f

URL: https://github.com/llvm/llvm-project/commit/019bd6485c52a62c008eacfdf0d13a26ca6b0a6f
DIFF: https://github.com/llvm/llvm-project/commit/019bd6485c52a62c008eacfdf0d13a26ca6b0a6f.diff

LOG: [lldb] Don't complete ObjCInterfaceDecls in ClangExternalASTSourceCallbacks::FindExternalVisibleDeclsByName

Summary:
For ObjCInterfaceDecls, LLDB iterates over the `methods` of the interface in FindExternalVisibleDeclsByName
since commit ef423a3ba57045f80b0fcafce72121449a8b54d4 .
However, when LLDB calls `oid->methods()` in that function, Clang will pull in all declarations in the current
DeclContext from the current ExternalASTSource (which is again, `ClangExternalASTSourceCallbacks`). The
reason for that is that `methods()` is just a wrapper for `decls()` which is supposed to provide a list of *all*
(both currently loaded and external) decls in the DeclContext.

However, `ClangExternalASTSourceCallbacks::FindExternalLexicalDecls` doesn't implement support for ObjCInterfaceDecl,
so we don't actually add any declarations and just mark the ObjCInterfaceDecl as having no ExternalLexicalStorage.

As LLDB uses the ExternalLexicalStorage to see if it can complete a type with the ExternalASTSource, this causes
that LLDB thinks our class can't be completed any further by the ExternalASTSource
and will from on no longer make any CompleteType/FindExternalLexicalDecls calls to that decl. This essentially
renders those types unusable in the expression parser as they will always be considered incomplete.

This patch just changes the call to `methods` (which is just a `decls()` wrapper), to some ad-hoc `noload_methods`
call which is wrapping `noload_decls()`. `noload_decls()` won't trigger any calls to the ExternalASTSource, so
this prevents that ExternalLexicalStorage will be set to false.

The test for this is just adding a method to an ObjC interface. Before this patch, this unset the ExternalLexicalStorage
flag and put the interface into the state described above.

In a normal user session this situation was triggered by setting a breakpoint in a method of some ObjC class. This
caused LLDB to create the MethodDecl for that specific method and put it into the the ObjCInterfaceDecl.
Also `ObjCLanguageRuntime::LookupInCompleteClassCache` needs to be unable to resolve the type do
an actual definition when the breakpoint is set (I'm not sure how exactly this can happen, but we just
found no Type instance that had the `TypePayloadClang::IsCompleteObjCClass` flag set in its payload in
the situation where this happens. This however doesn't seem to be a regression as logic wasn't changed
from what I can see).

The module-ownership.mm test had to be changed as the only reason why the ObjC interface in that test had
it's ExternalLexicalStorage flag set to false was because of this unintended side effect. What actually happens
in the test is that ExternalLexicalStorage is first set to false in `DWARFASTParserClang::CompleteTypeFromDWARF`
when we try to complete the `SomeClass` interface, but is then the flag is set back to true once we add
the last ivar of `SomeClass` (see `SetMemberOwningModule` in `TypeSystemClang.cpp` which is called
when we add the ivar). I'll fix the code for that in a follow-up patch.

I think some of the code here needs some rethinking. LLDB and Clang shouldn't infer anything about the ExternalASTSource
and its ability to complete the current type form the `ExternalLexicalStorage` flag. We probably should
also actually provide any declarations when we get asked for the lexical decls of an ObjCInterfaceDecl. But both of those
changes are bigger (and most likely would cause us to eagerly complete more types), so those will be follow up patches
and this patch just brings us back to the state before commit ef423a3ba57045f80b0fcafce72121449a8b54d4 .

Fixes rdar://63584164

Reviewers: aprantl, friss, shafik

Reviewed By: aprantl, shafik

Subscribers: arphaman, abidh, JDevlieghere

Differential Revision: https://reviews.llvm.org/D80556

Added: 
    

Modified: 
    lldb/source/Plugins/ExpressionParser/Clang/ClangExternalASTSourceCallbacks.cpp
    lldb/test/Shell/SymbolFile/DWARF/module-ownership.mm
    lldb/unittests/Symbol/TestTypeSystemClang.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExternalASTSourceCallbacks.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExternalASTSourceCallbacks.cpp
index e4054b441d55..390afb458b5a 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExternalASTSourceCallbacks.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExternalASTSourceCallbacks.cpp
@@ -53,7 +53,8 @@ bool ClangExternalASTSourceCallbacks::FindExternalVisibleDeclsByName(
   // Objective-C methods are not added into the LookupPtr when they originate
   // from an external source. SetExternalVisibleDeclsForName() adds them.
   if (auto *oid = llvm::dyn_cast<clang::ObjCInterfaceDecl>(DC)) {
-    for (auto *omd : oid->methods())
+    clang::ObjCContainerDecl::method_range noload_methods(oid->noload_decls());
+    for (auto *omd : noload_methods)
       if (omd->getDeclName() == Name)
         decls.push_back(omd);
   }

diff  --git a/lldb/test/Shell/SymbolFile/DWARF/module-ownership.mm b/lldb/test/Shell/SymbolFile/DWARF/module-ownership.mm
index b20e08024b9b..311fd34d40e8 100644
--- a/lldb/test/Shell/SymbolFile/DWARF/module-ownership.mm
+++ b/lldb/test/Shell/SymbolFile/DWARF/module-ownership.mm
@@ -46,7 +46,7 @@ @implementation SomeClass {
 // RUN: lldb-test symbols -dump-clang-ast -find type --language=ObjC++ \
 // RUN:   -compiler-context 'Module:A,Struct:SomeClass' %t.o \
 // RUN:   | FileCheck %s --check-prefix=CHECK-OBJC
-// CHECK-OBJC: ObjCInterfaceDecl {{.*}} imported in A SomeClass
+// CHECK-OBJC: ObjCInterfaceDecl {{.*}} imported in A <undeserialized declarations> SomeClass
 // CHECK-OBJC-NEXT: |-ObjCIvarDecl
 // CHECK-OBJC-NEXT: |-ObjCMethodDecl 0x[[NUMBER:[0-9a-f]+]]{{.*}} imported in A
 // CHECK-OBJC-NEXT: `-ObjCPropertyDecl {{.*}} imported in A number 'int' readonly

diff  --git a/lldb/unittests/Symbol/TestTypeSystemClang.cpp b/lldb/unittests/Symbol/TestTypeSystemClang.cpp
index c67168ba5f56..bd7eb14d4533 100644
--- a/lldb/unittests/Symbol/TestTypeSystemClang.cpp
+++ b/lldb/unittests/Symbol/TestTypeSystemClang.cpp
@@ -688,3 +688,37 @@ TEST_F(TestTypeSystemClang, TestNotDeletingUserCopyCstrDueToMoveCStr) {
   auto *record = llvm::cast<CXXRecordDecl>(ClangUtil::GetAsTagDecl(t));
   EXPECT_TRUE(record->hasUserDeclaredCopyConstructor());
 }
+
+TEST_F(TestTypeSystemClang, AddMethodToObjCObjectType) {
+  // Create an interface decl and mark it as having external storage.
+  CompilerType c = m_ast->CreateObjCClass("A", m_ast->GetTranslationUnitDecl(),
+                                          OptionalClangModuleID(),
+                                          /*IsForwardDecl*/ false,
+                                          /*IsInternal*/ false);
+  ObjCInterfaceDecl *interface = m_ast->GetAsObjCInterfaceDecl(c);
+  m_ast->SetHasExternalStorage(c.GetOpaqueQualType(), true);
+  EXPECT_TRUE(interface->hasExternalLexicalStorage());
+
+  // Add a method to the interface.
+  std::vector<CompilerType> args;
+  CompilerType func_type =
+      m_ast->CreateFunctionType(m_ast->GetBasicType(lldb::eBasicTypeInt),
+                                args.data(), args.size(), /*variadic*/ false,
+                                /*quals*/ 0, clang::CallingConv::CC_C);
+  bool variadic = false;
+  bool artificial = false;
+  bool objc_direct = false;
+  clang::ObjCMethodDecl *method = TypeSystemClang::AddMethodToObjCObjectType(
+      c, "-[A foo]", func_type, lldb::eAccessPublic, artificial, variadic,
+      objc_direct);
+  ASSERT_NE(method, nullptr);
+
+  // The interface decl should still have external lexical storage.
+  EXPECT_TRUE(interface->hasExternalLexicalStorage());
+
+  // Test some properties of the created ObjCMethodDecl.
+  EXPECT_FALSE(method->isVariadic());
+  EXPECT_TRUE(method->isImplicit());
+  EXPECT_FALSE(method->isDirectMethod());
+  EXPECT_EQ(method->getDeclName().getObjCSelector().getAsString(), "foo");
+}