llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Cyndy Ishida (cyndyishida) <details> <summary>Changes</summary> Before this patch, the `-fmodule-map-file`s on an explicitly-built module's command line came solely from what that module's implicitly-built PCM recorded as input files. Since that is an observed trace, it can omit a direct dependency's module map. For example, if a dependency was already registered by an earlier PCM read, a later `@<!-- -->import` can short-circuit & nothing enters its module map locally as an input file. However, that's not always guaranteed. Like when relocation checking is enabled, the direct dep's module map can end up being entered, resulting in mismatched invocations/hashes. Instead, collect each direct dependency's module map, then let the trace contribute only what that didn't cover. The collection happens in the order of `MF.Imports`, which is stable across runs, this is important since modulemap order is baked into the explicit module's hash. After, collect any remaining module maps through the trace order, that is not guaranteed to be stable ordering but is so far not observable, and strictly less likely to occur now. The module maps are deduped by their `FileEntry`, and reporting is skipped under EagerLoadModules. This patch also adds more test coverage for order-sensitive modulemap command-line tracking. --- Full diff: https://github.com/llvm/llvm-project/pull/229611.diff 5 Files Affected: - (modified) clang/include/clang/DependencyScanning/ModuleDepCollector.h (+11-6) - (modified) clang/lib/DependencyScanning/ModuleDepCollector.cpp (+72-55) - (added) clang/test/ClangScanDeps/modules-direct-dep-modulemap-commandline.m (+62) - (modified) clang/test/ClangScanDeps/modules-extern-unrelated.m (+1) - (added) clang/test/ClangScanDeps/modules-module-map-parse-order-commandline.m (+55) ``````````diff diff --git a/clang/include/clang/DependencyScanning/ModuleDepCollector.h b/clang/include/clang/DependencyScanning/ModuleDepCollector.h index 6e896bd26f763a..b5708198e43e94 100644 --- a/clang/include/clang/DependencyScanning/ModuleDepCollector.h +++ b/clang/include/clang/DependencyScanning/ModuleDepCollector.h @@ -180,13 +180,18 @@ class ModuleDepCollector final : public DependencyCollector { const ModuleDeps &Deps, llvm::function_ref<void(CowCompilerInvocation &)> Optimize) const; - /// Collect module map files for given modules. - llvm::DenseSet<const FileEntry *> - collectModuleMapFiles(ArrayRef<ModuleID> ClangModuleDeps) const; + /// Collect the module maps for the module dependencies that are named with + /// -fmodule-file. + /// + /// \param ModuleMapFiles The list of module map files to append for the + /// invocation, if needed. + /// \param SeenModuleMapFiles The uniqued set of module maps accounted for. + /// \param ClangModuleDeps The module dependencies whose module maps to add. + void collectModuleMapFiles( + std::vector<std::string> &ModuleMapFiles, + llvm::SmallDenseSet<const FileEntry *, 16> &SeenModuleMapFiles, + ArrayRef<ModuleID> ClangModuleDeps) const; - /// Add module map files to the invocation, if needed. - void addModuleMapFiles(CompilerInvocation &CI, - ArrayRef<ModuleID> ClangModuleDeps) const; /// Add module files (pcm) to the invocation, if needed. void addModuleFiles(CompilerInvocation &CI, ArrayRef<ModuleID> ClangModuleDeps) const; diff --git a/clang/lib/DependencyScanning/ModuleDepCollector.cpp b/clang/lib/DependencyScanning/ModuleDepCollector.cpp index 5b38e5105bc315..36ff99464b70d4 100644 --- a/clang/lib/DependencyScanning/ModuleDepCollector.cpp +++ b/clang/lib/DependencyScanning/ModuleDepCollector.cpp @@ -324,38 +324,12 @@ ModuleDepCollector::getInvocationAdjustedForModuleBuildWithoutOutputs( CI.getMutFrontendOpts().Inputs.emplace_back(Deps.ClangModuleMapFile, ModuleMapInputKind); - auto CurrentModuleMapEntry = - ScanInstance.getFileManager().getOptionalFileRef(Deps.ClangModuleMapFile); - assert(CurrentModuleMapEntry && "module map file entry not found"); - // Remove directly passed modulemap files. They will get added back if they // were actually used. CI.getMutFrontendOpts().ModuleMapFiles.clear(); - auto DepModuleMapFiles = collectModuleMapFiles(Deps.ClangModuleDeps); - for (StringRef ModuleMapFile : Deps.ModuleMapFileDeps) { - // TODO: Track these as `FileEntryRef` to simplify the equality check below. - auto ModuleMapEntry = - ScanInstance.getFileManager().getOptionalFileRef(ModuleMapFile); - assert(ModuleMapEntry && "module map file entry not found"); - - // Don't report module maps describing eagerly-loaded dependency. This - // information will be deserialized from the PCM. - // TODO: Verify this works fine when modulemap for module A is eagerly - // loaded from A.pcm, and module map passed on the command line contains - // definition of a submodule: "explicit module A.Private { ... }". - if (Service.getOpts().EagerLoadModules && - DepModuleMapFiles.contains(*ModuleMapEntry)) - continue; - - // Don't report module map file of the current module unless it also - // describes a dependency (for symmetry). - if (*ModuleMapEntry == *CurrentModuleMapEntry && - !DepModuleMapFiles.contains(*ModuleMapEntry)) - continue; - + for (const std::string &ModuleMapFile : Deps.ModuleMapFileDeps) CI.getMutFrontendOpts().ModuleMapFiles.emplace_back(ModuleMapFile); - } // Report the prebuilt modules this module uses. for (const auto &PrebuiltModule : Deps.PrebuiltModuleDeps) @@ -378,9 +352,28 @@ ModuleDepCollector::getInvocationAdjustedForModuleBuildWithoutOutputs( return CI; } -llvm::DenseSet<const FileEntry *> ModuleDepCollector::collectModuleMapFiles( +static void +collectModuleMap(std::vector<std::string> &ModuleMapFiles, + llvm::SmallDenseSet<const FileEntry *, 16> &SeenModuleMapFiles, + FileEntryRef ModuleMapFile, StringRef Path, + const bool Report) { + if (!SeenModuleMapFiles.insert(ModuleMapFile).second) + return; + // Conditionalize reporting for modes like eager module loading. + // That mode will have this information deserialized from the PCM instead. + // TODO: Verify this works fine when modulemap for module A is eagerly + // loaded from A.pcm, and module map passed on the command line contains + // definition of a submodule: "explicit module A.Private { ... }". + if (Report) + ModuleMapFiles.emplace_back(Path); +} + +void ModuleDepCollector::collectModuleMapFiles( + std::vector<std::string> &ModuleMapFiles, + llvm::SmallDenseSet<const FileEntry *, 16> &SeenModuleMapFiles, ArrayRef<ModuleID> ClangModuleDeps) const { - llvm::DenseSet<const FileEntry *> ModuleMapFiles; + // Add each dependency's module map, in the order the PCM recorded its + // imports. This is expected to be a stable order across runs and workers. for (const ModuleID &MID : ClangModuleDeps) { ModuleDeps *MD = ModuleDepsByID.lookup(MID); assert(MD && "Inconsistent dependency info"); @@ -388,20 +381,12 @@ llvm::DenseSet<const FileEntry *> ModuleDepCollector::collectModuleMapFiles( auto FE = ScanInstance.getFileManager().getOptionalFileRef( MD->ClangModuleMapFile); assert(FE && "Missing module map file that was previously found"); - ModuleMapFiles.insert(*FE); - } - return ModuleMapFiles; -} - -void ModuleDepCollector::addModuleMapFiles( - CompilerInvocation &CI, ArrayRef<ModuleID> ClangModuleDeps) const { - if (Service.getOpts().EagerLoadModules) - return; // Only pcm is needed for eager load. + if (!FE) + continue; - for (const ModuleID &MID : ClangModuleDeps) { - ModuleDeps *MD = ModuleDepsByID.lookup(MID); - assert(MD && "Inconsistent dependency info"); - CI.getFrontendOpts().ModuleMapFiles.push_back(MD->ClangModuleMapFile); + collectModuleMap(ModuleMapFiles, SeenModuleMapFiles, *FE, + MD->ClangModuleMapFile, + /*Report=*/!Service.getOpts().EagerLoadModules); } } @@ -454,26 +439,29 @@ void ModuleDepCollector::applyDiscoveredDependencies(CompilerInvocation &CI) { if (llvm::any_of(CI.getFrontendOpts().Inputs, needsModules)) { Preprocessor &PP = ScanInstance.getPreprocessor(); - if (Module *CurrentModule = PP.getCurrentModuleImplementation()) - if (OptionalFileEntryRef CurrentModuleMap = - PP.getHeaderSearchInfo() - .getModuleMap() - .getModuleMapFileForUniquing(CurrentModule)) - CI.getFrontendOpts().ModuleMapFiles.emplace_back( - CurrentModuleMap->getNameAsRequested()); SmallVector<ModuleID> DirectDeps; for (const auto &KV : ModularDeps) if (DirectModularDeps.contains(KV.first)) DirectDeps.push_back(KV.second->ID); - // TODO: Report module maps the same way it's done for modular dependencies. - addModuleMapFiles(CI, DirectDeps); - addModuleFiles(CI, DirectDeps); for (const auto &KV : DirectPrebuiltModularDeps) CI.getFrontendOpts().ModuleFiles.push_back(KV.second.PCMFile); + + llvm::SmallDenseSet<const FileEntry *, 16> SeenModuleMapFiles; + const ModuleMap &ModMapInfo = PP.getHeaderSearchInfo().getModuleMap(); + if (Module *CurrentModule = PP.getCurrentModuleImplementation()) + if (OptionalFileEntryRef CurrentModuleMap = + ModMapInfo.getModuleMapFileForUniquing(CurrentModule)) + collectModuleMap(CI.getFrontendOpts().ModuleMapFiles, + SeenModuleMapFiles, *CurrentModuleMap, + CurrentModuleMap->getNameAsRequested(), + /*Report=*/true); + + collectModuleMapFiles(CI.getFrontendOpts().ModuleMapFiles, + SeenModuleMapFiles, DirectDeps); } } @@ -762,8 +750,10 @@ ModuleDepCollector::handleTopLevelModule(serialization::ModuleFile *MF) { ModuleMap &ModMapInfo = MDC.ScanInstance.getPreprocessor().getHeaderSearchInfo().getModuleMap(); - if (auto ModuleMap = ModMapInfo.getModuleMapFileForUniquing(M)) { - SmallString<128> Path = ModuleMap->getNameAsRequested(); + OptionalFileEntryRef CurrentModuleMapEntry = + ModMapInfo.getModuleMapFileForUniquing(M); + if (CurrentModuleMapEntry) { + SmallString<128> Path = CurrentModuleMapEntry->getNameAsRequested(); ModMapInfo.canonicalizeModuleMapPath(Path); MD.ClangModuleMapFile = std::string(Path); } @@ -786,6 +776,11 @@ ModuleDepCollector::handleTopLevelModule(serialization::ModuleFile *MF) { addAllModuleDeps(*MF, MD); + // Capture module map file dependencies. + llvm::SmallDenseSet<const FileEntry *, 16> SeenModuleMapFiles; + MDC.collectModuleMapFiles(MD.ModuleMapFileDeps, SeenModuleMapFiles, + MD.ClangModuleDeps); + SmallString<0> PathBuf; PathBuf.reserve(256); MDC.ScanInstance.getASTReader()->visitInputFileInfos( @@ -805,7 +800,29 @@ ModuleDepCollector::handleTopLevelModule(serialization::ModuleFile *MF) { auto ResolvedFilenameAsRequested = ASTReader::ResolveImportedPath( PathBuf, IFI.UnresolvedImportedFilenameAsRequested, MF->BaseDirectory); - MD.ModuleMapFileDeps.emplace_back(*ResolvedFilenameAsRequested); + + // TODO: Track these as `FileEntryRef` to simplify the equality check + // below. + auto ModuleMapEntry = + MDC.ScanInstance.getFileManager().getOptionalFileRef( + *ResolvedFilenameAsRequested); + assert(ModuleMapEntry && "module map file entry not found"); + if (!ModuleMapEntry) + return; + + // Don't report module map file of the current module unless it also + // describes a dependency in ModuleMapFileDeps (for symmetry). + if (CurrentModuleMapEntry && *ModuleMapEntry == *CurrentModuleMapEntry) + return; + + // Add additional modulemaps for input files the module + // dependencies did not already cover. e.g. A modulemap that only + // declares a submodule is recorded only as an input file. + // FIXME: It is not guaranteed to be in a stable order across runs or + // workers. + collectModuleMap(MD.ModuleMapFileDeps, SeenModuleMapFiles, + *ModuleMapEntry, *ResolvedFilenameAsRequested, + /*Report=*/true); }); bool IgnoreCWD = false; diff --git a/clang/test/ClangScanDeps/modules-direct-dep-modulemap-commandline.m b/clang/test/ClangScanDeps/modules-direct-dep-modulemap-commandline.m new file mode 100644 index 00000000000000..f10b6007160d1c --- /dev/null +++ b/clang/test/ClangScanDeps/modules-direct-dep-modulemap-commandline.m @@ -0,0 +1,62 @@ +// Check an explicit module's command line names the module map of every +// dependency it is given with -fmodule-file. + +// In this example, EntryPoint imports DirectOnly first, and loading DirectOnly's +// PCM populates DirectAndTransitive into memory. +// Then when EntryPoint imports DirectAndTransitive (through @import) itself, the +// module is already known, so the lookup short-circuits and never searches +// DirectAndTransitive's module map in EntryPoint's CompilerInstance as an input file. +// The scan should still register DirectAndTransitive's modulemap as a +// direct dependency to ensure the resulting explicit module invocation is stable +// as additional invalidation or relocation checks can cause EntryPoint's +// CompilerInstance to observe that modulemap as an input file dependency. + +// RUN: rm -rf %t +// RUN: split-file %s %t + +// RUN: clang-scan-deps -format experimental-full -- \ +// RUN: %clang -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \ +// RUN: -I %t/entrypoint -I %t/directonly -I %t/directandtransitive -c %t/tu.m -o %t/tu.o \ +// RUN: > %t/result.json + +// RUN: %deps-to-rsp %t/result.json --module-name=EntryPoint > %t/EntryPoint.rsp +// RUN: cat %t/EntryPoint.rsp | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t + +// CHECK: -fmodule-map-file=[[PREFIX]]/directonly/module.modulemap +// CHECK: -fmodule-map-file=[[PREFIX]]/directandtransitive/module.modulemap +// CHECK: -fmodule-file=DirectAndTransitive={{[^"]*}}.pcm +// CHECK: -fmodule-file=DirectOnly={{[^"]*}}.pcm + +// Verify round-trip compilation passes. +// RUN: %deps-to-rsp %t/result.json --module-name=DirectAndTransitive > %t/DirectAndTransitive.rsp +// RUN: %deps-to-rsp %t/result.json --module-name=DirectOnly > %t/DirectOnly.rsp +// RUN: %deps-to-rsp %t/result.json --tu-index=0 > %t/tu.rsp +// RUN: %clang @%t/DirectAndTransitive.rsp +// RUN: %clang @%t/DirectOnly.rsp +// RUN: %clang @%t/EntryPoint.rsp +// RUN: %clang @%t/tu.rsp + +//--- directandtransitive/module.modulemap +module DirectAndTransitive { header "direct-and-transitive.h" export * } + +//--- directandtransitive/direct-and-transitive.h +int directAndTransitive(void); + +//--- directonly/module.modulemap +module DirectOnly { header "direct-only.h" export * } + +//--- directonly/direct-only.h +#include "direct-and-transitive.h" +int directOnly(void); + +//--- entrypoint/module.modulemap +module EntryPoint { header "entry-point.h" export * } + +//--- entrypoint/entry-point.h +#include "direct-only.h" +@import DirectAndTransitive; +int entryPoint(void); + +//--- tu.m +#include "entry-point.h" +int tu(void) { return entryPoint(); } diff --git a/clang/test/ClangScanDeps/modules-extern-unrelated.m b/clang/test/ClangScanDeps/modules-extern-unrelated.m index 061d0912d086a1..c0e6fd1f4b2f36 100644 --- a/clang/test/ClangScanDeps/modules-extern-unrelated.m +++ b/clang/test/ClangScanDeps/modules-extern-unrelated.m @@ -92,6 +92,7 @@ // CHECK-NEXT: "clang-modulemap-file": "[[PREFIX]]/zeroth/module.modulemap", // CHECK-NEXT: "command-line": [ // CHECK-NOT: "-fmodule-map-file=[[PREFIX]]/second/module.modulemap" +// CHECK: "-fmodule-map-file=[[PREFIX]]/second/second.modulemap" // CHECK: ], // CHECK-NEXT: "context-hash": "{{.*}}", // CHECK-NEXT: "file-deps": [ diff --git a/clang/test/ClangScanDeps/modules-module-map-parse-order-commandline.m b/clang/test/ClangScanDeps/modules-module-map-parse-order-commandline.m new file mode 100644 index 00000000000000..d7f5c27d5e8f14 --- /dev/null +++ b/clang/test/ClangScanDeps/modules-module-map-parse-order-commandline.m @@ -0,0 +1,55 @@ +// Check that an explicit module's command line orders dependency module maps +// in the valid parse order. + +// In this example, Parent and Parent.Sub are declared in separate directories. +// For Client's explicit module, the top level Parent module must come before +// parsing Parent.Sub. +// The required ordering is the opposite of a lexicographical sort of those +// directories. + +// RUN: rm -rf %t +// RUN: split-file %s %t + +// RUN: clang-scan-deps -format experimental-full -- \ +// RUN: %clang -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \ +// RUN: -I %t/client -I %t/zzz -I %t/aaa -c %t/tu.m -o %t/tu.o \ +// RUN: > %t/result.json + +// The map declaring Parent must still be named before the one declaring +// Parent.Sub. +// RUN: %deps-to-rsp %t/result.json --module-name=Client > %t/Client.rsp +// RUN: cat %t/Client.rsp | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t + +// CHECK: -fmodule-map-file=[[PREFIX]]/zzz/module.modulemap +// CHECK-SAME: -fmodule-map-file=[[PREFIX]]/aaa/module.modulemap + +// Verify round-trip compilation passes. +// RUN: %deps-to-rsp %t/result.json --module-name=Parent > %t/Parent.rsp +// RUN: %deps-to-rsp %t/result.json --tu-index=0 > %t/tu.rsp +// RUN: %clang @%t/Parent.rsp +// RUN: %clang @%t/Client.rsp +// RUN: %clang @%t/tu.rsp + +//--- zzz/module.modulemap +module Parent { header "parent.h" } + +//--- zzz/parent.h + +//--- aaa/module.modulemap +explicit module Parent.Sub { + header "sub.h" + exclude header "sub-excluded.h" +} + +//--- aaa/sub.h +//--- aaa/sub-excluded.h + +//--- client/module.modulemap +module Client { header "client.h" } + +//--- client/client.h +#include "parent.h" +#include "sub-excluded.h" + +//--- tu.m +@import Client; `````````` </details> https://github.com/llvm/llvm-project/pull/229611 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
