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 @@ -991,6 +991,10 @@ /// in a submodule. SubmoduleState *CurSubmoduleState; + /// The set of files that have been included in each submodule. + /// Files included outside of any module (e.g. in PCH) have nullptr key. + llvm::DenseMap IncludedFilesPerSubmodule; + /// The files that have been included. IncludedFilesSet IncludedFiles; @@ -1477,10 +1481,25 @@ return AffectingClangModules; } - /// Mark the file as included. + /// Mark the file as included in the current state. + /// Do not attribute it to the current module - this file was included by one + /// of its dependencies (transitively). + void markTransitivelyIncluded(const FileEntry *File) { + HeaderInfo.getFileInfo(File); + IncludedFiles.insert(File); + } + + /// Mark the file as included in the current state and attribute it to the + /// current module. /// Returns true if this is the first time the file was included. - bool markIncluded(const FileEntry *File) { + bool markDirectlyIncluded(const FileEntry *File) { HeaderInfo.getFileInfo(File); + + Module *M = !BuildingSubmoduleStack.empty() + ? BuildingSubmoduleStack.back().M + : getCurrentModule(); + IncludedFilesPerSubmodule[M].insert(File); + return IncludedFiles.insert(File).second; } @@ -1490,6 +1509,20 @@ return IncludedFiles.count(File); } + /// Invoke the callback for every module known to include the given file. + void forModulesIncluding(const FileEntry *FE, + llvm::function_ref Cb) { + // TODO: Maintain a data-structure that provides this more efficiently. + for (const auto &Entry : IncludedFilesPerSubmodule) + if (Entry.second.contains(FE)) + Cb(Entry.first); + } + + /// Get the set of files included in the given module. + IncludedFilesSet &getFilesIncludedIn(Module *M) { + return IncludedFilesPerSubmodule[M]; + } + /// Get the set of included files. IncludedFilesSet &getIncludedFiles() { return IncludedFiles; } const IncludedFilesSet &getIncludedFiles() const { return IncludedFiles; } @@ -1730,6 +1763,11 @@ return CurSubmoduleState->VisibleModules.getImportLoc(M); } + /// Determine whether the given module is currently visible. + bool isModuleCurrentlyVisible(Module *M) const { + return CurSubmoduleState->VisibleModules.isVisible(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/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp --- a/clang/lib/Lex/HeaderSearch.cpp +++ b/clang/lib/Lex/HeaderSearch.cpp @@ -1482,7 +1482,7 @@ } } - IsFirstIncludeOfFile = PP.markIncluded(File); + IsFirstIncludeOfFile = PP.markDirectlyIncluded(File); return true; } 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 @@ -559,7 +559,7 @@ // Tell the header info that the main file was entered. If the file is later // #imported, it won't be re-entered. if (const FileEntry *FE = SourceMgr.getFileEntryForID(MainFileID)) - markIncluded(FE); + markDirectlyIncluded(FE); } // Preprocess Predefines to populate the initial preprocessor state. @@ -1340,7 +1340,14 @@ void Preprocessor::makeModuleVisible(Module *M, SourceLocation Loc) { CurSubmoduleState->VisibleModules.setVisible( - M, Loc, [](Module *) {}, + M, Loc, + [&](Module *M) { + // The set of included files may be incomplete for modules coming from + // a PCM, since we're deserializing included files lazily. The rest of + // the files will be handled lazily, as we need them. + for (const FileEntry *FE : getFilesIncludedIn(M)) + markTransitivelyIncluded(FE); + }, [&](ArrayRef Path, Module *Conflict, StringRef Message) { // FIXME: Include the path in the diagnostic. // FIXME: Include the import location for the conflicting module. 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 @@ -1875,19 +1875,15 @@ return LocalID + I->second; } -const FileEntry *HeaderFileInfoTrait::getFile(const internal_key_type &Key) { +OptionalFileEntryRef +HeaderFileInfoTrait::getFile(const internal_key_type &Key) { FileManager &FileMgr = Reader.getFileManager(); - if (!Key.Imported) { - if (auto File = FileMgr.getFile(Key.Filename)) - return *File; - return nullptr; - } + if (!Key.Imported) + return FileMgr.getOptionalFileRef(Key.Filename); std::string Resolved = std::string(Key.Filename); Reader.ResolveImportedPath(M, Resolved); - if (auto File = FileMgr.getFile(Resolved)) - return *File; - return nullptr; + return FileMgr.getOptionalFileRef(Resolved); } unsigned HeaderFileInfoTrait::ComputeHash(internal_key_ref ikey) { @@ -1910,8 +1906,8 @@ return true; // Determine whether the actual files are equivalent. - const FileEntry *FEA = getFile(a); - const FileEntry *FEB = getFile(b); + OptionalFileEntryRef FEA = getFile(a); + OptionalFileEntryRef FEB = getFile(b); return FEA && FEA == FEB; } @@ -1937,17 +1933,15 @@ unsigned DataLen) { using namespace llvm::support; + OptionalFileEntryRef FE = getFile(key); + // We can only get here if the key of this entry is an absolute path that + // matches the name of an already known FileEntry, or the FileEntry given out + // by FileManager for this key compares equal to the alrleady known FileEntry. + assert(FE && "Missing FileEntry during HeaderFileInfo deserialization"); + const unsigned char *End = d + DataLen; HeaderFileInfo HFI; unsigned Flags = *d++; - - bool Included = (Flags >> 6) & 0x01; - if (Included) - if (const FileEntry *FE = getFile(key)) - // Not using \c Preprocessor::markIncluded(), since that would attempt to - // deserialize this header file info again. - Reader.getPreprocessor().getIncludedFiles().insert(FE); - // FIXME: Refactor with mergeHeaderFileInfo in HeaderSearch.cpp. HFI.isImport |= (Flags >> 5) & 0x01; HFI.isPragmaOnce |= (Flags >> 4) & 0x01; @@ -1955,6 +1949,18 @@ HFI.IndexHeaderMapHeader = Flags & 0x01; HFI.ControllingMacroID = Reader.getGlobalIdentifierID( M, endian::readNext(d)); + + Preprocessor &PP = Reader.getPreprocessor(); + auto IncludedInCount = endian::readNext(d); + for (uint32_t I = 0; I < IncludedInCount; ++I) { + uint32_t ModID = endian::readNext(d); + Module *Mod = Reader.getModule(ModID); + if (Mod) + PP.getFilesIncludedIn(Mod).insert(*FE); + if (!Mod || PP.isModuleCurrentlyVisible(Mod)) + PP.getIncludedFiles().insert(*FE); + } + if (unsigned FrameworkOffset = endian::readNext(d)) { // The framework offset is 1 greater than the actual offset, @@ -1974,18 +1980,11 @@ // implicit module import. SubmoduleID GlobalSMID = Reader.getGlobalSubmoduleID(M, LocalSMID); Module *Mod = Reader.getSubmodule(GlobalSMID); - FileManager &FileMgr = Reader.getFileManager(); - ModuleMap &ModMap = - Reader.getPreprocessor().getHeaderSearchInfo().getModuleMap(); - - std::string Filename = std::string(key.Filename); - if (key.Imported) - Reader.ResolveImportedPath(M, Filename); - if (auto FE = FileMgr.getOptionalFileRef(Filename)) { - // FIXME: NameAsWritten - Module::Header H = {std::string(key.Filename), "", *FE}; - ModMap.addHeader(Mod, H, HeaderRole, /*Imported=*/true); - } + ModuleMap &ModMap = PP.getHeaderSearchInfo().getModuleMap(); + + // FIXME: NameAsWritten + Module::Header H = {std::string(key.Filename), "", *FE}; + ModMap.addHeader(Mod, H, HeaderRole, /*Imported=*/true); HFI.isModuleHeader |= ModuleMap::isModular(HeaderRole); } diff --git a/clang/lib/Serialization/ASTReaderInternals.h b/clang/lib/Serialization/ASTReaderInternals.h --- a/clang/lib/Serialization/ASTReaderInternals.h +++ b/clang/lib/Serialization/ASTReaderInternals.h @@ -15,6 +15,7 @@ #include "MultiOnDiskHashTable.h" #include "clang/AST/DeclarationName.h" +#include "clang/Basic/FileEntry.h" #include "clang/Basic/LLVM.h" #include "clang/Serialization/ASTBitCodes.h" #include "llvm/ADT/DenseSet.h" @@ -27,7 +28,6 @@ namespace clang { class ASTReader; -class FileEntry; struct HeaderFileInfo; class HeaderSearch; class ObjCMethodDecl; @@ -278,7 +278,7 @@ data_type ReadData(internal_key_ref,const unsigned char *d, unsigned DataLen); private: - const FileEntry *getFile(const internal_key_type &Key); + OptionalFileEntryRef getFile(const internal_key_type &Key); }; /// The on-disk hash table used for known header files. diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -1762,7 +1762,7 @@ struct data_type { const HeaderFileInfo &HFI; - bool AlreadyIncluded; + SmallVector IncludedIn; ArrayRef KnownHeaders; UnresolvedModule Unresolved; }; @@ -1787,6 +1787,7 @@ DataLen += 4; if (Data.Unresolved.getPointer()) DataLen += 4; + DataLen += 4 + 4 * Data.IncludedIn.size(); return emitULEBKeyDataLength(KeyLen, DataLen, Out); } @@ -1808,8 +1809,7 @@ endian::Writer LE(Out, little); uint64_t Start = Out.tell(); (void)Start; - unsigned char Flags = (Data.AlreadyIncluded << 6) - | (Data.HFI.isImport << 5) + unsigned char Flags = (Data.HFI.isImport << 5) | (Data.HFI.isPragmaOnce << 4) | (Data.HFI.DirInfo << 1) | Data.HFI.IndexHeaderMapHeader; @@ -1820,6 +1820,10 @@ else LE.write(Writer.getIdentifierRef(Data.HFI.ControllingMacro)); + LE.write(Data.IncludedIn.size()); + for (uint32_t ModID : Data.IncludedIn) + LE.write(ModID); + unsigned Offset = 0; if (!Data.HFI.Framework.empty()) { // If this header refers into a framework, save the framework name. @@ -1910,7 +1914,7 @@ HeaderFileInfoTrait::key_type Key = { FilenameDup, *U.Size, IncludeTimestamps ? *U.ModTime : 0}; HeaderFileInfoTrait::data_type Data = { - Empty, false, {}, {M, ModuleMap::headerKindToRole(U.Kind)}}; + Empty, {}, {}, {M, ModuleMap::headerKindToRole(U.Kind)}}; // FIXME: Deal with cases where there are multiple unresolved header // directives in different submodules for the same header. Generator.insert(Key, Data, GeneratorTrait); @@ -1953,13 +1957,19 @@ SavedStrings.push_back(Filename.data()); } - bool Included = PP->alreadyIncluded(File); + SmallVector IncludedIn; + PP->forModulesIncluding(File, [&](Module *M) { + if ((!M && !WritingModule) || + (M && M->getTopLevelModule() == WritingModule)) + IncludedIn.push_back(getSubmoduleID(M)); + }); + llvm::sort(IncludedIn); HeaderFileInfoTrait::key_type Key = { Filename, File->getSize(), getTimestampForOutput(File) }; HeaderFileInfoTrait::data_type Data = { - *HFI, Included, HS.getModuleMap().findResolvedModulesForHeader(File), {} + *HFI, IncludedIn, HS.getModuleMap().findResolvedModulesForHeader(File), {} }; Generator.insert(Key, Data, GeneratorTrait); ++NumHeaderSearchEntries; diff --git a/clang/test/Modules/import-submodule-visibility.c b/clang/test/Modules/import-submodule-visibility.c new file mode 100644 --- /dev/null +++ b/clang/test/Modules/import-submodule-visibility.c @@ -0,0 +1,64 @@ +// This test checks that imports of headers that appeared in a different submodule than +// what is imported by the current TU don't affect the compilation. + +// RUN: rm -rf %t +// RUN: split-file %s %t + +//--- C/C.h +#include "Textual.h" +//--- C/module.modulemap +module C { header "C.h" } + +//--- D/D1.h +#include "Textual.h" +//--- D/D2.h +//--- D/module.modulemap +module D { + module D1 { header "D1.h" } + module D2 { header "D2.h" } +} + +//--- E/E1.h +#include "E2.h" +//--- E/E2.h +#include "Textual.h" +//--- E/module.modulemap +module E { + module E1 { header "E1.h" } + module E2 { header "E2.h" } +} + +//--- Textual.h +#define MACRO_TEXTUAL 1 + +//--- test_top.c +#import "Textual.h" +static int x = MACRO_TEXTUAL; + +//--- test_sub.c +#import "D/D2.h" +#import "Textual.h" +static int x = MACRO_TEXTUAL; + +//--- test_transitive.c +#import "E/E1.h" +#import "Textual.h" +static int x = MACRO_TEXTUAL; + +// Simply loading a PCM file containing top-level module including a header does +// not prevent inclusion of that header in the TU. +// +// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/C/module.modulemap -fmodule-name=C -o %t/C.pcm +// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_top.c -fmodule-file=%t/C.pcm + +// Loading a PCM file and importing its empty submodule does not prevent +// inclusion of headers included by invisible sibling submodules. +// +// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/D/module.modulemap -fmodule-name=D -o %t/D.pcm +// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_sub.c -fmodule-file=%t/D.pcm + +// Loading a PCM file and importing a submodule does not prevent inclusion of +// headers included by some of its transitive un-exported dependencies. +// +// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/E/module.modulemap -fmodule-name=E -o %t/E.pcm +// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_transitive.c -fmodule-file=%t/E.pcm