diff --git a/clang/include/clang/Lex/Preprocessor.h b/clang/include/clang/Lex/Preprocessor.h --- a/clang/include/clang/Lex/Preprocessor.h +++ b/clang/include/clang/Lex/Preprocessor.h @@ -776,6 +776,12 @@ llvm::DenseMap> LeafModuleMacros; + // Not the same as visible modules as ignores visibility controls and tracks + // transitively imported submodules. Should be tracked in SubmoduleState but + // for the proof of concept staying away from Local Submodule Visibility + // shenanigans. + llvm::DenseSet AllImportedSubModules; + /// Macros that we want to warn because they are not used at the end /// of the translation unit. /// @@ -1458,6 +1464,10 @@ return CurSubmoduleState->VisibleModules.getImportLoc(M); } + bool isModuleImported(const Module *M) const { + return AllImportedSubModules.count(M); + } + /// Lex a string literal, which may be the concatenation of multiple /// string literals and may even come from macro expansion. /// \returns true on success, false if a error diagnostic has been generated. diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -406,6 +406,11 @@ bool OwnsDeserializationListener = false; + // Special types are deserialized before we have a chance to mark a submodule + // as visible or hidden. Add a hacky way to allow deserialization of types' + // decls even if we don't know if a corresponding module is visible. + bool HackyIsDeserializingSpecialTypes = false; + SourceManager &SourceMgr; FileManager &FileMgr; const PCHContainerReader &PCHContainerRdr; diff --git a/clang/lib/Lex/Preprocessor.cpp b/clang/lib/Lex/Preprocessor.cpp --- a/clang/lib/Lex/Preprocessor.cpp +++ b/clang/lib/Lex/Preprocessor.cpp @@ -1304,8 +1304,9 @@ } void Preprocessor::makeModuleVisible(Module *M, SourceLocation Loc) { + bool HasAddedMoreModules = false; CurSubmoduleState->VisibleModules.setVisible( - M, Loc, [](Module *) {}, + M, Loc, [&HasAddedMoreModules](Module *) { HasAddedMoreModules = true; }, [&](ArrayRef Path, Module *Conflict, StringRef Message) { // FIXME: Include the path in the diagnostic. // FIXME: Include the import location for the conflicting module. @@ -1318,6 +1319,42 @@ // Add this module to the imports list of the currently-built submodule. if (!BuildingSubmoduleStack.empty() && M != BuildingSubmoduleStack.back().M) BuildingSubmoduleStack.back().M->Imports.insert(M); + + std::function VisitModule = [&](const Module *VisM) { + if (VisM->IsUnimportable) + return; + if (AllImportedSubModules.count(VisM)) + return; + AllImportedSubModules.insert(VisM); + + for (const Module *ImportedM : VisM->Imports) + VisitModule(ImportedM); + + // Need to traverse also exported modules to handle implicitly-imported + // submodules like + // module Parent { + // export * + // module Child { header "child.h" } + // } + // Where `@import Parent` also pulls in the module "Parent.Child". + SmallVector Exports; + VisM->getExportedModules(Exports); + for (const Module *ExportedM : Exports) + VisitModule(ExportedM); + }; + VisitModule(M); + + // EXPERIMENTAL: Setting a (sub)module visible allows to deserialize its + // decls. For this reason marking the identifiers out-of-date like + // in `ASTReader::ReadAST`. + if (HasAddedMoreModules && getExternalSource()) { + if (!getLangOpts().CPlusPlus) { + for (IdentifierTable::iterator Id = getIdentifierTable().begin(), + IdEnd = getIdentifierTable().end(); + Id != IdEnd; ++Id) + Id->second->setOutOfDate(true); + } + } } bool Preprocessor::FinishLexStringLiteral(Token &Result, std::string &String, diff --git a/clang/lib/Sema/SemaModule.cpp b/clang/lib/Sema/SemaModule.cpp --- a/clang/lib/Sema/SemaModule.cpp +++ b/clang/lib/Sema/SemaModule.cpp @@ -407,6 +407,10 @@ } void Sema::BuildModuleInclude(SourceLocation DirectiveLoc, Module *Mod) { + // Make the module visible before adding a module initializer. + getModuleLoader().makeModuleVisible(Mod, Module::AllVisible, DirectiveLoc); + VisibleModules.setVisible(Mod, DirectiveLoc); + // Determine whether we're in the #include buffer for a module. The #includes // in that buffer do not qualify as module imports; they're just an // implementation detail of us building the module. @@ -430,9 +434,6 @@ TU->addDecl(ImportD); Consumer.HandleImplicitImportDecl(ImportD); } - - getModuleLoader().makeModuleVisible(Mod, Module::AllVisible, DirectiveLoc); - VisibleModules.setVisible(Mod, DirectiveLoc); } void Sema::ActOnModuleBegin(SourceLocation DirectiveLoc, Module *Mod) { diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -4862,6 +4862,7 @@ // built-in types. Right now, we just ignore the problem. // Load the special types. + HackyIsDeserializingSpecialTypes = true; if (SpecialTypes.size() >= NumSpecialTypeIDs) { if (unsigned String = SpecialTypes[SPECIAL_TYPE_CF_CONSTANT_STRING]) { if (!Context.CFConstantStringTypeDecl) @@ -4964,6 +4965,7 @@ } } } + HackyIsDeserializingSpecialTypes = false; ReadPragmaDiagnosticMappings(Context.getDiagnostics()); @@ -7628,9 +7630,9 @@ SmallVector Decls; llvm::SmallPtrSet Found; for (DeclID ID : It->second.Table.find(Name)) { - NamedDecl *ND = cast(GetDecl(ID)); - if (ND->getDeclName() == Name && Found.insert(ND).second) - Decls.push_back(ND); + if (NamedDecl *ND = cast_or_null(GetDecl(ID))) + if (ND->getDeclName() == Name && Found.insert(ND).second) + Decls.push_back(ND); } ++NumVisibleDeclContextsRead; @@ -8474,18 +8476,20 @@ continue; } - NamedDecl *D = cast(GetDecl(DeclIDs[I])); + if (Decl *DeserializedD = GetDecl(DeclIDs[I])) { + NamedDecl *D = cast(DeserializedD); - // If we're simply supposed to record the declarations, do so now. - if (Decls) { - Decls->push_back(D); - continue; - } + // If we're simply supposed to record the declarations, do so now. + if (Decls) { + Decls->push_back(D); + continue; + } - // Introduce this declaration into the translation-unit scope - // and add it to the declaration chain for this identifier, so - // that (unqualified) name lookup will find it. - pushExternalDeclIntoScope(D, II); + // Introduce this declaration into the translation-unit scope + // and add it to the declaration chain for this identifier, so + // that (unqualified) name lookup will find it. + pushExternalDeclIntoScope(D, II); + } } } diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/lib/Serialization/ASTReaderDecl.cpp @@ -46,6 +46,7 @@ #include "clang/Basic/PragmaKinds.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/Specifiers.h" +#include "clang/Lex/Preprocessor.h" #include "clang/Sema/IdentifierResolver.h" #include "clang/Serialization/ASTBitCodes.h" #include "clang/Serialization/ASTRecordReader.h" @@ -3870,6 +3871,15 @@ llvm::report_fatal_error( Twine("ASTReader::readDeclRecord failed reading decl code: ") + toString(MaybeDeclCode.takeError())); + if (unsigned DeclSubmoduleID = getGlobalSubmoduleID(*Loc.F, Record.readInt())) { + if (!HackyIsDeserializingSpecialTypes) { + Module *DeclOwner = getSubmodule(DeclSubmoduleID); + if (DeclOwner && !PP.isModuleImported(DeclOwner)) { + // Skip deserialization of Decls from a hidden module. + return nullptr; + } + } + } switch ((DeclCode)MaybeDeclCode.get()) { case DECL_CONTEXT_LEXICAL: case DECL_CONTEXT_VISIBLE: @@ -4356,9 +4366,11 @@ // we should instead generate one loop per kind and dispatch up-front? Decl *MostRecent = FirstLocal; for (unsigned I = 0, N = Record.size(); I != N; ++I) { - auto *D = GetLocalDecl(*M, Record[N - I - 1]); - ASTDeclReader::attachPreviousDecl(*this, D, MostRecent, CanonDecl); - MostRecent = D; + // A local decl is from the same module but can be from a hidden submodule. + if (Decl *D = GetLocalDecl(*M, Record[N - I - 1])) { + ASTDeclReader::attachPreviousDecl(*this, D, MostRecent, CanonDecl); + MostRecent = D; + } } ASTDeclReader::attachLatestDecl(CanonDecl, MostRecent); } diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp --- a/clang/lib/Serialization/ASTWriterDecl.cpp +++ b/clang/lib/Serialization/ASTWriterDecl.cpp @@ -269,6 +269,7 @@ } void ASTDeclWriter::Visit(Decl *D) { + Record.push_back(Writer.getSubmoduleID(D->getOwningModule())); DeclVisitor::Visit(D); // Source locations require array (variable-length) abbreviations. The @@ -1911,6 +1912,7 @@ // Abbreviation for DECL_FIELD Abv = std::make_shared(); Abv->Add(BitCodeAbbrevOp(serialization::DECL_FIELD)); + Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID // Decl Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // DeclContext Abv->Add(BitCodeAbbrevOp(0)); // LexicalDeclContext @@ -1944,6 +1946,7 @@ // Abbreviation for DECL_OBJC_IVAR Abv = std::make_shared(); Abv->Add(BitCodeAbbrevOp(serialization::DECL_OBJC_IVAR)); + Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID // Decl Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // DeclContext Abv->Add(BitCodeAbbrevOp(0)); // LexicalDeclContext @@ -1980,6 +1983,7 @@ // Abbreviation for DECL_ENUM Abv = std::make_shared(); Abv->Add(BitCodeAbbrevOp(serialization::DECL_ENUM)); + Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID // Redeclarable Abv->Add(BitCodeAbbrevOp(0)); // No redeclaration // Decl @@ -2030,6 +2034,7 @@ // Abbreviation for DECL_RECORD Abv = std::make_shared(); Abv->Add(BitCodeAbbrevOp(serialization::DECL_RECORD)); + Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID // Redeclarable Abv->Add(BitCodeAbbrevOp(0)); // No redeclaration // Decl @@ -2092,6 +2097,7 @@ // Abbreviation for DECL_PARM_VAR Abv = std::make_shared(); Abv->Add(BitCodeAbbrevOp(serialization::DECL_PARM_VAR)); + Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID // Redeclarable Abv->Add(BitCodeAbbrevOp(0)); // No redeclaration // Decl @@ -2140,6 +2146,7 @@ // Abbreviation for DECL_TYPEDEF Abv = std::make_shared(); Abv->Add(BitCodeAbbrevOp(serialization::DECL_TYPEDEF)); + Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID // Redeclarable Abv->Add(BitCodeAbbrevOp(0)); // No redeclaration // Decl @@ -2169,6 +2176,7 @@ // Abbreviation for DECL_VAR Abv = std::make_shared(); Abv->Add(BitCodeAbbrevOp(serialization::DECL_VAR)); + Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID // Redeclarable Abv->Add(BitCodeAbbrevOp(0)); // No redeclaration // Decl @@ -2221,6 +2229,7 @@ // Abbreviation for DECL_CXX_METHOD Abv = std::make_shared(); Abv->Add(BitCodeAbbrevOp(serialization::DECL_CXX_METHOD)); + Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID // RedeclarableDecl Abv->Add(BitCodeAbbrevOp(0)); // CanonicalDecl // Decl diff --git a/clang/test/Modules/ambiguous-enum-lookup.m b/clang/test/Modules/ambiguous-enum-lookup.m new file mode 100644 --- /dev/null +++ b/clang/test/Modules/ambiguous-enum-lookup.m @@ -0,0 +1,42 @@ +// RUN: rm -rf %t +// RUN: split-file %s %t +// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache -F %t/Frameworks %t/test.m + +//--- Frameworks/NonModular.framework/Headers/NonModular.h +#ifndef NonModular_NonModular_h +#define NonModular_NonModular_h +struct SomeStruct { + int x; +}; + +enum SomeEnum { + kEnumValue = 1, +}; +#endif + +//--- Frameworks/PiecewiseVisible.framework/Headers/InitiallyVisible.h +// empty + +//--- Frameworks/PiecewiseVisible.framework/Headers/InitiallyHidden.h +#include + +//--- Frameworks/PiecewiseVisible.framework/Modules/module.modulemap +framework module PiecewiseVisible { + header "InitiallyVisible.h" + export * + + explicit module HiddenPiece { + header "InitiallyHidden.h" + export * + } +} + +//--- test.m +#include +#include +#include + +void test() { + struct SomeStruct s; + s.x = kEnumValue; +}