[lldb][gmodules] Enable ExternalVisibleStorage for Decls from modules

[teemperor]

Summary

When we have a DeclContext from a module (i.e., the isFromASTFile flag is set on the respective Decl),
Clang expects ExternalASTSource:: FindExternalVisibleDeclsByName to build the lookup in and set
ExternalVisibleStorage as described by the comment in DeclContext::buildLookupImpl:

// If it's from an AST file, don't add it now. It'll get handled by
// FindExternalVisibleDeclsByName if needed. [...]

We already mostly implemented FindExternalVisibleDeclsByName in LLDB and we already set ExternalVisibleStorage
when building the AST for our symbol file. However, when we import a Decl from one AST to another and
copy over the Decl, we only set the IsFromASTFile bit but didn't enable ExternalVisibleStorage if the
Decl is also a DeclContext.

This patch sets ExternalVisibleStorage for all DeclContexts that are from a module (which seems to be expected
by Clang). I also expanded the implementation of FindExternalVisibleDeclsByName to cover all Decl kinds
and not just Objective-C methods.

I also removed that when we "Complete" a type that we unset the ExternalVisibleStorage flag. The reason for that
is that even when we have all Decls loaded in a DeclContext and we consider the type "Complete" from LLDB's
perspective, we still might need to build the lookup for some declarations from the ExternalASTSource.

One strange situation that I found while debugging this is that we set the 'owning module' in LLDB after we imported
a declaration. However we have some situations where between the Import and the Imported callback some part of Clang
decides to inspect a Declaration we just added. In that case we build the lookup as if the Decl is not from a module.
So the lookup failure we were seeing only appear if Clang doesn't decide to do a build the lookup before LLDB actually
marks an imported Decl as being owned by a module/coming from an AST file.

The change to module-ownership.mm is that I removed that we allow the <undeserialized declarations> in the test.
It seems the idea was to make the test pass even when there are no <undeserialized declarations> in that struct,
but because of the extra space after {{.*}} the test fails when we don't have <undeserialized declarations> in the
output. In any case, given that the struct has only one member, there shouldn't be any <undeserialized declarations>
when we dump it and see that one member, so the previous test was anyway too permissive. I'm not sure why that
case didn't work before, but given that it apparently fixes this test, I didn't investigate this yet.

[aprantl]

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

1247

That makes sense, thanks!

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

if (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)) {
-    clang::ObjCContainerDecl::method_range noload_methods(oid->noload_decls());
-    for (auto *omd : noload_methods)
-      if (omd->getDeclName() == Name)
-        decls.push_back(omd);
+  // SetExternalVisibleDeclsForName() should make all declarations visible
+  // that are from an AST file.
+  for (auto *d : DC->noload_decls())
+    if (clang::NamedDecl *nd = llvm::dyn_cast<clang::NamedDecl>(d)) {
+      if (nd->isFromASTFile() && nd->getDeclName() == Name)
+        decls.push_back(nd);
     }
   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});

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

 void TypeSystemClang::SetOwningModule(clang::Decl *decl,
                                       OptionalClangModuleID owning_module) {
   if (!decl || !owning_module.HasValue())
     return;
 
   decl->setFromASTFile();
   decl->setOwningModuleID(owning_module.GetValue());
   decl->setModuleOwnershipKind(clang::Decl::ModuleOwnershipKind::Visible);
+
+  // Declarations that are marked as setFromASTFile are expected to be added to
+  // the lookup by the ExternalASTSource. See DeclContext::buildLookupImpl.
+  if (DeclContext *dc = dyn_cast<DeclContext>(decl))
+    dc->setHasExternalVisibleStorage(true);
 }
 
 OptionalClangModuleID
 TypeSystemClang::GetOrCreateClangModule(llvm::StringRef name,
                                         OptionalClangModuleID parent,
                                         bool is_framework, bool is_explicit) {
   // Get the external AST source which holds the modules.
   auto *ast_source = llvm::dyn_cast_or_null<ClangExternalASTSourceCallbacks>(
if (auto *cxx_record_decl = llvm::dyn_cast<CXXRecordDecl>(tag_decl)) {
         if (cxx_record_decl->needsImplicitCopyAssignment())
           cxx_record_decl->setImplicitCopyAssignmentIsDeleted();
       }
 
       if (!cxx_record_decl->isCompleteDefinition())
         cxx_record_decl->completeDefinition();
       cxx_record_decl->setHasLoadedFieldsFromExternalStorage(true);
       cxx_record_decl->setHasExternalLexicalStorage(false);
-      cxx_record_decl->setHasExternalVisibleStorage(false);
       return true;
     }
   }
 
   const clang::EnumType *enutype = qual_type->getAs<clang::EnumType>();
 
   if (!enutype)
     return false;

lldb/test/API/lang/cpp/gmodules-field-types-from-different-modules/Makefile

+CXX_SOURCES = main.cpp
+CFLAGS_EXTRAS := $(MODULE_DEBUG_INFO_FLAGS)
+
+include Makefile.rules

lldb/test/API/lang/cpp/gmodules-field-types-from-different-modules/TestGModulesFieldTypesFromDifferentModules.py

+"""
+Tests that a record with types from two modules works with -gmodules.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def test(self):
+        self.build()
+        self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+        self.expect_expr("mix_modules.u1.c.i", result_type="int", result_value="0")

lldb/test/API/lang/cpp/gmodules-field-types-from-different-modules/categories

+gmodules

lldb/test/API/lang/cpp/gmodules-field-types-from-different-modules/common.h

+#ifndef COMMON_H
+#define COMMON_H
+struct Common {
+  int i;
+};
+#endif // COMMON_H

lldb/test/API/lang/cpp/gmodules-field-types-from-different-modules/main.cpp

+#include "mod1.h"
+#include "mod2.h"
+
+struct Mix {
+  Use1 u1;
+  Use2 u2;
+};
+
+// A record that has FieldDecls with types from two different modules.
+Mix mix_modules;
+
+int main(int argc, const char *argv[]) { return mix_modules.u1.c.i; }

lldb/test/API/lang/cpp/gmodules-field-types-from-different-modules/mod1.h

+#include "common.h"
+
+struct Use1 {
+  Common c;
+};

lldb/test/API/lang/cpp/gmodules-field-types-from-different-modules/mod2.h

+#include "common.h"
+
+struct Use2 {
+  Common y;
+};

lldb/test/API/lang/cpp/gmodules-field-types-from-different-modules/mod3.h

+#include "mod1.h"
+#include "mod2.h"
+
+struct Mix {
+  Foo f;
+  Bar b;
+};

lldb/test/API/lang/cpp/gmodules-field-types-from-different-modules/module.modulemap

+module M1 { header "mod1.h" export * }
+module M2 { header "mod2.h" export * }

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

 // RUN:   | FileCheck %s --check-prefix=CHECK-TOPLEVELSTRUCT
 // CHECK-TOPLEVELSTRUCT: CXXRecordDecl {{.*}} imported in A struct TopLevelStruct
 // CHECK-TOPLEVELSTRUCT: -FieldDecl {{.*}} in A a 'int'
 
 Struct s2;
 // CHECK-ANON-S1: CXXRecordDecl {{.*}} imported in A struct
 
 StructB s3;
-// CHECK-ANON-S2: CXXRecordDecl {{.*}} imported in A.B {{.*}} struct
+// CHECK-ANON-S2: CXXRecordDecl {{.*}} imported in A.B struct
 // CHECK-ANON-S2: -FieldDecl {{.*}} in A.B anon_field_b 'int'
 
 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