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

[teemperor]

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

[aprantl]

This sounds like it is in the spirit of the API, yes. Thanks!

[aprantl]

Great analysis.

[shafik]

So do we still need the changes in SetMemberOwningModule that set setHasExternalVisibleStorage and setHasExternalLexicalStorage?

[teemperor]

In D80556#2055541, @shafik wrote:

So do we still need the changes in SetMemberOwningModule that set setHasExternalVisibleStorage and setHasExternalLexicalStorage?

That's a good point. I think that is the fix to get the module-ownership test working as intended (haven't tested that though). But not setting ExternalLexicalStorage can have bad consequences while having it set by accident should only be a small performance hit (as we will do the round-trip over SymbolFile that should early-exit on decls it already completed). So I think we should fix this in the follow-up commits and only try avoid unsetting ExternalLexicalStorage for this patch that gets back ported.

[shafik]

It is fine to look at SetMemberOwningModule in a follow-up PR. I am not concerned from a performance perspective but just leaving around unnecessary code that will just confuse someone later on. If it is not needed anymore we should remove ASAP.

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

 }
 
 bool ClangExternalASTSourceCallbacks::FindExternalVisibleDeclsByName(
     const clang::DeclContext *DC, clang::DeclarationName Name) {
   llvm::SmallVector<clang::NamedDecl *, 4> decls;
   // 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);
   }
   return !SetExternalVisibleDeclsForName(DC, Name, decls).empty();
 }
 
 OptionalClangModuleID
 ClangExternalASTSourceCallbacks::RegisterModule(clang::Module *module) {

lldb/test/Shell/SymbolFile/DWARF/module-ownership.mm

 }
 @synthesize number = private_ivar;
 @end
 
 SomeClass *obj1;
 // 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
 // CHECK-OBJC-NEXT:   `-getter ObjCMethod 0x[[NUMBER]] 'number'
 
 // Template specializations are not yet supported, so they lack the ownership info:
 Template<int> t2;
 // CHECK-DAG: ClassTemplateSpecializationDecl {{.*}} imported in A struct Template
 
 Namespace::InNamespace<int> t3;
 // CHECK-DAG: ClassTemplateSpecializationDecl {{.*}} imported in A struct InNamespace
 
 Namespace::AlsoInNamespace<int> t4;
 // CHECK-DAG: ClassTemplateSpecializationDecl {{.*}} imported in A.B struct AlsoInNamespace

lldb/unittests/Symbol/TestTypeSystemClang.cpp

m_ast->AddMethodToCXXRecordType(
         is_explicit, is_attr_used, is_artificial);
   }
 
   // Complete the definition and check the created record.
   m_ast->CompleteTagDeclarationDefinition(t);
   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");
+}