Add Objective-C property accessors loaded from Clang module DWARF to lookup

[aprantl]

Summary

This patch fixes a bug when synthesizing an ObjC property from -gmodules debug info. Because the method declaration that is injected via the non-modular property implementation is not added to the ObjCInterfaceDecl's lookup pointer, a second copy of the accessor would be generated when processing the ObjCPropertyDecl. This can be avoided by finding the existing method decl in ClangExternalASTSourceCallbacks::FindExternalVisibleDeclsByName() and adding it to the LookupPtr.

[teemperor]

I'm not sure I understand why are we not adding the property to the lookup ptr? I would assume we would call addDecl to it which should make it visible and I don't see any ObjCPropertyDecl (?) check in that code?

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

348

SetMemberOwningModule is called really often and these two conditions are always met in our code, so we are now constantly resetting setHasExternalVisibleStorage to true?

351

You shouldn't need this to get FindExternalVisibleDeclsByName and all this seems to work without it?

[aprantl]

I'm not sure I understand why are we not adding the property to the lookup ptr? I would assume we would call addDecl to it which should make it visible and I don't see any ObjCPropertyDecl (?) check in that code?

Thanks! This sounds like exactly the kind of feedback I was hoping for, just to be sure, could you please clarify what you mean specifically? The problem is that the ObjCMethodDecl for the getter that comes from the @synthesize declaration is missing in the LookupPtr and therefore this check:

clang::ObjCMethodDecl *getter = nullptr;
if (!getter_sel.isNull())
  getter = isInstance ? class_interface_decl->lookupInstanceMethod(getter_sel)
                      : class_interface_decl->lookupClassMethod(getter_sel);

fails and so TypeSystemClang::AddObjCClassProperty() synthesizes a second getter. Where are you suggesting to add the ObjCPropertyDecl to the lookup ptr and how would that help?

[teemperor]

So it seems there aren't any alternative solutions that are less invasive, so I think this can go in.

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

348

Actually this probably can stay. It's not perfect that we keep resetting, but we should usually add all members at once I think, so it's not really making this thing more complicated.

351

So if you move this above the comment this is already good enough. It's not needed for FindExternalVisibleDeclsByName but it makes sense that this has external lexical storage.

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

public:
 
   ClangExternalASTSourceCallbacks(TypeSystemClang &ast) : m_ast(ast) {}
 
   void FindExternalLexicalDecls(
       const clang::DeclContext *DC,
       llvm::function_ref<bool(clang::Decl::Kind)> IsKindWeWant,
       llvm::SmallVectorImpl<clang::Decl *> &Result) override;
 
+  bool FindExternalVisibleDeclsByName(const clang::DeclContext *DC,
+                                      clang::DeclarationName Name) override;
+
   void CompleteType(clang::TagDecl *tag_decl) override;
 
   void CompleteType(clang::ObjCInterfaceDecl *objc_decl) override;
 
   bool layoutRecordType(
       const clang::RecordDecl *Record, uint64_t &Size, uint64_t &Alignment,
       llvm::DenseMap<const clang::FieldDecl *, uint64_t> &FieldOffsets,
       llvm::DenseMap<const clang::CXXRecordDecl *, clang::CharUnits>

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

 //===-- ClangExternalASTSourceCallbacks.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 "Plugins/ExpressionParser/Clang/ClangExternalASTSourceCallbacks.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 
 #include "clang/AST/Decl.h"
+#include "clang/AST/DeclObjC.h"
 
 using namespace lldb_private;
 
 char ClangExternalASTSourceCallbacks::ID;
 
 void ClangExternalASTSourceCallbacks::CompleteType(clang::TagDecl *tag_decl) {
   m_ast.CompleteTagDecl(tag_decl);
 }
void ClangExternalASTSourceCallbacks::FindExternalLexicalDecls(
   if (decl_ctx) {
     clang::TagDecl *tag_decl = llvm::dyn_cast<clang::TagDecl>(
         const_cast<clang::DeclContext *>(decl_ctx));
     if (tag_decl)
       CompleteType(tag_decl);
   }
 }
 
+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())
+      if (omd->getDeclName() == Name)
+        decls.push_back(omd);
+  }
+  return !SetExternalVisibleDeclsForName(DC, Name, decls).empty();
+}
+
 OptionalClangModuleID
 ClangExternalASTSourceCallbacks::RegisterModule(clang::Module *module) {
   m_modules.push_back(module);
   unsigned id = m_modules.size();
   m_ids.insert({module, id});
   return OptionalClangModuleID(id);
 }
 

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

static void SetMemberOwningModule(clang::Decl *member,
 
   OptionalClangModuleID id(parent->getOwningModuleID());
   if (!id.HasValue())
     return;
 
   member->setFromASTFile();
   member->setOwningModuleID(id.GetValue());
   member->setModuleOwnershipKind(clang::Decl::ModuleOwnershipKind::Visible);
+  if (auto *nd = llvm::dyn_cast<clang::NamedDecl>(member))
+    if (auto *dc = llvm::dyn_cast<clang::DeclContext>(parent)) {
+      dc->setHasExternalVisibleStorage(true);
+      // This triggers ExternalASTSource::FindExternalVisibleDeclsByName() to be
+      // called when searching for members.
+      dc->setHasExternalLexicalStorage(true);
+    }
 }
 
 char TypeSystemClang::ID;
 
 bool TypeSystemClang::IsOperator(llvm::StringRef name,
                                  clang::OverloadedOperatorKind &op_kind) {
   // All operators have to start with "operator".
   if (!name.consume_front("operator"))

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

 Nested s4;
 // CHECK-DAG: CXXRecordDecl {{.*}} imported in A struct Nested
 // CHECK-DAG: -FieldDecl {{.*}} in A fromb 'StructB'
 
 Enum e1;
 // CHECK-DAG: EnumDecl {{.*}} imported in A {{.*}} Enum_e
 // FIXME: -EnumConstantDecl {{.*}} imported in A a
 
+@implementation SomeClass {
+  int private_ivar;
+}
+@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: |-ObjCPropertyDecl {{.*}} imported in A number 'int' readonly
-// CHECK-OBJC: | `-getter ObjCMethod {{.*}} 'number'
-// CHECK-OBJC: `-ObjCMethodDecl {{.*}} imported in A implicit - number 'int'
+// 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 {{.*}} struct Template
 
 Namespace::InNamespace<int> t3;
 // CHECK-DAG: ClassTemplateSpecializationDecl {{.*}} struct InNamespace
 
 Namespace::AlsoInNamespace<int> t4;
 // CHECK-DAG: ClassTemplateSpecializationDecl {{.*}} struct AlsoInNamespace