Author: Cyndy Ishida Date: 2026-03-02T08:52:46-08:00 New Revision: cf8597bd3b87aeed6696454f22311e86ed70138f
URL: https://github.com/llvm/llvm-project/commit/cf8597bd3b87aeed6696454f22311e86ed70138f DIFF: https://github.com/llvm/llvm-project/commit/cf8597bd3b87aeed6696454f22311e86ed70138f.diff LOG: [clang][Modules] Handle relocated modules during implicit module builds (#181836) * To avoid the build time overhead of checking for relocated modules, only check it once per build session. * Enable relocated module checks in the dependency scanner. * Add remarks to know when this is happening with `-Rmodule-validation` This check is necessary to be able to handle new libraries appearing in earlier search paths. This is a valid scenario when dependency info changes between incremental builds of the same scheme, thus new build sessions. It is still malformed to expect new versions of libraries to be added within the same build session. resolves: rdar://169174750 Added: clang/test/ClangScanDeps/build-session-validation-relocated-modules.c clang/test/Modules/build-session-validation-relocated-modules.c Modified: clang/include/clang/Basic/DiagnosticSerializationKinds.td clang/include/clang/Serialization/ASTReader.h clang/lib/DependencyScanning/DependencyScannerImpl.cpp clang/lib/Serialization/ASTReader.cpp clang/test/ClangScanDeps/modules-relocated-mm-macro.c clang/test/ClangScanDeps/modules-symlink-dir-from-module.c Removed: ################################################################################ diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td b/clang/include/clang/Basic/DiagnosticSerializationKinds.td index b602a9e55cb7d..475c19957c4be 100644 --- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td +++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td @@ -101,6 +101,13 @@ def remark_module_validation : Remark< "validating %0 input files in module '%1' from '%2'">, ShowInSystemHeader, InGroup<ModuleValidation>; +def remark_module_check_relocation + : Remark<"checking if module '%0' from '%1' has relocated">, + ShowInSystemHeader, + InGroup<ModuleValidation>; +def remark_module_relocated : Remark<"module '%0' relocated from '%1' to '%2'">, + ShowInSystemHeader, + InGroup<ModuleValidation>; def err_imported_module_not_found : Error< "module '%0' in precompiled file '%1' %select{(imported by precompiled file '%2') |}4" diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index 1c17e91b51d7e..f254459ce933d 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -1590,6 +1590,21 @@ class ASTReader void ParseLineTable(ModuleFile &F, const RecordData &Record); llvm::Error ReadSourceManagerBlock(ModuleFile &F); SourceLocation getImportLocation(ModuleFile *F); + + /// The first element is `std::nullopt` if relocation check should be skipped. + /// Otherwise, the optional holds a pointer to the discovered module. + /// The pointer can be `nullptr` if the discovery was unsuccessful. + /// The second element determines whether to emit related errors. + using RelocationResult = std::pair<std::optional<Module *>, bool>; + + /// Determine whether a relocation check for a module should be performed + /// by attempting to resolve the same module via lookup. + /// If so, also determine whether to emit errors for the relocation. + /// A relocated module is defined as a module that is either no longer + /// resolvable from the modulemap or search path it originally compiled it's + /// definition from. + RelocationResult getModuleForRelocationChecks(ModuleFile &F, + bool DirectoryCheck = false); ASTReadResult ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F, const ModuleFile *ImportedBy, unsigned ClientLoadCapabilities); diff --git a/clang/lib/DependencyScanning/DependencyScannerImpl.cpp b/clang/lib/DependencyScanning/DependencyScannerImpl.cpp index 5e1d12e95c162..8af349319d333 100644 --- a/clang/lib/DependencyScanning/DependencyScannerImpl.cpp +++ b/clang/lib/DependencyScanning/DependencyScannerImpl.cpp @@ -470,9 +470,6 @@ createScanCompilerInvocation(const CompilerInvocation &Invocation, true; ScanInvocation->getHeaderSearchOpts().ModulesForceValidateUserHeaders = false; - // Avoid some checks and module map parsing when loading PCM files. - ScanInvocation->getPreprocessorOpts().ModulesCheckRelocated = false; - // Ensure that the scanner does not create new dependency collectors, // and thus won't write out the extra '.d' files to disk. ScanInvocation->getDependencyOutputOpts() = {}; diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index bde000234a062..b82ae971bc84d 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -3162,6 +3162,49 @@ ASTReader::ASTReadResult ASTReader::ReadOptionsBlock( } } +ASTReader::RelocationResult +ASTReader::getModuleForRelocationChecks(ModuleFile &F, bool DirectoryCheck) { + // Don't emit module relocation errors if we have -fno-validate-pch. + const bool IgnoreError = + bool(PP.getPreprocessorOpts().DisablePCHOrModuleValidation & + DisableValidationForModuleKind::Module); + + if (!PP.getPreprocessorOpts().ModulesCheckRelocated) + return {std::nullopt, IgnoreError}; + + const bool IsImplicitModule = F.Kind == MK_ImplicitModule; + + if (!DirectoryCheck && + (!IsImplicitModule || ModuleMgr.begin()->Kind == MK_MainFile)) + return {std::nullopt, IgnoreError}; + + const HeaderSearchOptions &HSOpts = + PP.getHeaderSearchInfo().getHeaderSearchOpts(); + + // When only validating modules once per build session, + // Skip check if the timestamp is up to date or module was built in same build + // session. + if (HSOpts.ModulesValidateOncePerBuildSession && IsImplicitModule) { + if (F.InputFilesValidationTimestamp >= HSOpts.BuildSessionTimestamp) + return {std::nullopt, IgnoreError}; + if (static_cast<uint64_t>(F.File.getModificationTime()) >= + HSOpts.BuildSessionTimestamp) + return {std::nullopt, IgnoreError}; + } + + Diag(diag::remark_module_check_relocation) << F.ModuleName << F.FileName; + + // If we've already loaded a module map file covering this module, we may + // have a better path for it (relative to the current build if doing directory + // check). + Module *M = PP.getHeaderSearchInfo().lookupModule( + F.ModuleName, DirectoryCheck ? SourceLocation() : F.ImportLoc, + /*AllowSearch=*/DirectoryCheck, + /*AllowExtraModuleMapSearch=*/DirectoryCheck); + + return {M, IgnoreError}; +} + ASTReader::ASTReadResult ASTReader::ReadControlBlock(ModuleFile &F, SmallVectorImpl<ImportedModule> &Loaded, @@ -3561,31 +3604,36 @@ ASTReader::ReadControlBlock(ModuleFile &F, assert(!F.ModuleName.empty() && "MODULE_DIRECTORY found before MODULE_NAME"); F.BaseDirectory = std::string(Blob); - if (!PP.getPreprocessorOpts().ModulesCheckRelocated) + + auto [MaybeM, IgnoreError] = + getModuleForRelocationChecks(F, /*DirectoryCheck=*/true); + if (!MaybeM.has_value()) break; - // If we've already loaded a module map file covering this module, we may - // have a better path for it (relative to the current build). - Module *M = PP.getHeaderSearchInfo().lookupModule( - F.ModuleName, SourceLocation(), /*AllowSearch*/ true, - /*AllowExtraModuleMapSearch*/ true); - if (M && M->Directory) { - // If we're implicitly loading a module, the base directory can't - // change between the build and use. - // Don't emit module relocation error if we have -fno-validate-pch - if (!bool(PP.getPreprocessorOpts().DisablePCHOrModuleValidation & - DisableValidationForModuleKind::Module) && - F.Kind != MK_ExplicitModule && F.Kind != MK_PrebuiltModule) { - auto BuildDir = PP.getFileManager().getOptionalDirectoryRef(Blob); - if (!BuildDir || *BuildDir != M->Directory) { - if (!canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities)) - Diag(diag::err_imported_module_relocated) - << F.ModuleName << Blob << M->Directory->getName(); - return OutOfDate; - } - } + + Module *M = MaybeM.value(); + if (!M || !M->Directory) + break; + if (IgnoreError) { F.BaseDirectory = std::string(M->Directory->getName()); + break; } - break; + if ((F.Kind == MK_ExplicitModule) || (F.Kind == MK_PrebuiltModule)) + break; + + // If we're implicitly loading a module, the base directory can't + // change between the build and use. + auto BuildDir = PP.getFileManager().getOptionalDirectoryRef(Blob); + if (BuildDir && (*BuildDir == M->Directory)) { + F.BaseDirectory = std::string(M->Directory->getName()); + break; + } + Diag(diag::remark_module_relocated) + << F.ModuleName << Blob << M->Directory->getName(); + + if (!canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities)) + Diag(diag::err_imported_module_relocated) + << F.ModuleName << Blob << M->Directory->getName(); + return OutOfDate; } case MODULE_MAP_FILE: @@ -4578,19 +4626,20 @@ ASTReader::ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F, // usable header search context. assert(!F.ModuleName.empty() && "MODULE_NAME should come before MODULE_MAP_FILE"); - if (PP.getPreprocessorOpts().ModulesCheckRelocated && - F.Kind == MK_ImplicitModule && ModuleMgr.begin()->Kind != MK_MainFile) { + auto [MaybeM, IgnoreError] = + getModuleForRelocationChecks(F, /*DirectoryCheck=*/false); + if (MaybeM.has_value()) { // An implicitly-loaded module file should have its module listed in some // module map file that we've already loaded. - Module *M = - PP.getHeaderSearchInfo().lookupModule(F.ModuleName, F.ImportLoc); + Module *M = MaybeM.value(); auto &Map = PP.getHeaderSearchInfo().getModuleMap(); OptionalFileEntryRef ModMap = M ? Map.getModuleMapFileForUniquing(M) : std::nullopt; - // Don't emit module relocation error if we have -fno-validate-pch - if (!bool(PP.getPreprocessorOpts().DisablePCHOrModuleValidation & - DisableValidationForModuleKind::Module) && - !ModMap) { + if (!IgnoreError && !ModMap) { + if (M && M->Directory) + Diag(diag::remark_module_relocated) + << F.ModuleName << F.BaseDirectory << M->Directory->getName(); + if (!canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities)) { if (auto ASTFE = M ? M->getASTFile() : std::nullopt) { // This module was defined by an imported (explicit) module. diff --git a/clang/test/ClangScanDeps/build-session-validation-relocated-modules.c b/clang/test/ClangScanDeps/build-session-validation-relocated-modules.c new file mode 100644 index 0000000000000..47919229512d0 --- /dev/null +++ b/clang/test/ClangScanDeps/build-session-validation-relocated-modules.c @@ -0,0 +1,71 @@ +// Check that a relocated module is rebuilt and no error occurs. +// This is required when a incremental build adds a newer version of an +// already built library into a preexisting search path. +// In this example, on the second scan, `DepThatLoadsOldPCMs` is resolved first and +// populates `MovedDep` into memory. +// Then when `InvalidatedDep` is loaded, it's input file is out of date +// and requires a rebuild. +// When that happens the compiler notices `MovedDep` is in a earlier search path. + + +// RUN: rm -rf %t +// RUN: split-file %s %t +// RUN: sed -e "s|DIR|%/t|g" %t/compile-commands.json.in > %t/compile-commands.json + +// RUN: touch %t/session.timestamp +// RUN: clang-scan-deps -format experimental-full -j 1 \ +// RUN: -compilation-database %t/compile-commands.json -o %t/deps1.json +// RUN: cat %t/deps1.json | FileCheck %s --check-prefix=DEPS1 + +// Model update where same framework appears in earlier search path. +// This can occur on an incremental build where dependency relationships are updated. +// RUN: touch %t/session.timestamp +// RUN: sleep 1 +// RUN: mkdir %t/preferred_frameworks/ +// RUN: cp -r %t/fallback_frameworks/MovedDep.framework %t/preferred_frameworks/ +// RUN: touch %t/fallback_frameworks/InvalidatedDep.framework/Modules/module.modulemap + +// RUN: clang-scan-deps -format experimental-full -j 1 \ +// RUN: -compilation-database %t/compile-commands.json -o %t/deps2.json +// RUN: cat %t/deps2.json | FileCheck %s --check-prefix=DEPS2 + +// DEPS1: "clang-module-deps": [], +// DEPS1-NEXT: "clang-modulemap-file": "{{.*}}fallback_frameworks{{.*}}MovedDep.framework +// DEPS1: "name": "MovedDep" + +// DEPS2: "clang-module-deps": [], +// DEPS2-NEXT: "clang-modulemap-file": "{{.*}}preferred_frameworks{{.*}}MovedDep.framework +// DEPS2: "name": "MovedDep" + +//--- compile-commands.json.in +[ +{ + "directory": "DIR", + "command": "clang -c DIR/tu1.c -fmodules -fmodules-cache-path=DIR/cache -FDIR/preferred_frameworks -FDIR/fallback_frameworks -fbuild-session-file=DIR/session.timestamp -fmodules-validate-once-per-build-session -o DIR/tu1.o ", + "file": "DIR/tu1.c" +} +] + +//--- fallback_frameworks/MovedDep.framework/Modules/module.modulemap +framework module MovedDep { header "MovedDep.h" } +//--- fallback_frameworks/MovedDep.framework/Headers/MovedDep.h +int foo(void); + +//--- fallback_frameworks/InvalidatedDep.framework/Modules/module.modulemap +framework module InvalidatedDep { header "InvalidatedDep.h" } +//--- fallback_frameworks/InvalidatedDep.framework/Headers/InvalidatedDep.h +#include <MovedDep/MovedDep.h> + +//--- fallback_frameworks/DirectDep.framework/Modules/module.modulemap +framework module DirectDep { header "DirectDep.h" } +//--- fallback_frameworks/DirectDep.framework/Headers/DirectDep.h +#include <DepThatLoadsOldPCMs/DepThatLoadsOldPCMs.h> +#include <InvalidatedDep/InvalidatedDep.h> + +//--- fallback_frameworks/DepThatLoadsOldPCMs.framework/Modules/module.modulemap +framework module DepThatLoadsOldPCMs { header "DepThatLoadsOldPCMs.h" } +//--- fallback_frameworks/DepThatLoadsOldPCMs.framework/Headers/DepThatLoadsOldPCMs.h +#include <MovedDep/MovedDep.h> + +//--- tu1.c +#include <DirectDep/DirectDep.h> diff --git a/clang/test/ClangScanDeps/modules-relocated-mm-macro.c b/clang/test/ClangScanDeps/modules-relocated-mm-macro.c index 87fbad0c72131..4d78f01cf9d8c 100644 --- a/clang/test/ClangScanDeps/modules-relocated-mm-macro.c +++ b/clang/test/ClangScanDeps/modules-relocated-mm-macro.c @@ -1,5 +1,5 @@ // This test checks that we don't crash when we load two conflicting PCM files -// and instead emit the appropriate diagnostics. +// and instead use the newer copy. // RUN: rm -rf %t // RUN: split-file %s %t @@ -13,14 +13,11 @@ // RUN: cp -r %t/frameworks2/A.framework %t/frameworks1 -// RUN: not clang-scan-deps -format experimental-full -o %t/deps2.json 2>%t/errs -- \ +// RUN: clang-scan-deps -format experimental-full -o %t/deps2.json 2>&1 -- \ // RUN: %clang -fmodules -fmodules-cache-path=%t/cache \ // RUN: -F %t/frameworks1 -F %t/frameworks2 \ -// RUN: -c %t/tu2.m -o %t/tu2.o -// RUN: FileCheck --input-file=%t/errs %s - -// CHECK: fatal error: module 'A' is defined in both '{{.*}}.pcm' and '{{.*}}.pcm' -// CHECK-NEXT: note: compiled from '{{.*}}frameworks1{{.*}}' and '{{.*}}frameworks2{{.*}}' +// RUN: -c %t/tu2.m -o %t/tu2.o \ +// RUN: | FileCheck %s --allow-empty --implicit-check-not="warning" //--- frameworks2/A.framework/Modules/module.modulemap framework module A { header "A.h" } @@ -37,7 +34,7 @@ framework module B { header "B.h" } //--- tu2.m #include <A/A.h> -#include <B/B.h> // This results in a conflict and a fatal loader error. +#include <B/B.h> // This results in a rebuild of B. -#if MACRO_A // This crashes with lexer that does not respect `cutOfLexing()`. +#if MACRO_A // This previously crashed with lexer that does not respect `cutOfLexing()`. #endif diff --git a/clang/test/ClangScanDeps/modules-symlink-dir-from-module.c b/clang/test/ClangScanDeps/modules-symlink-dir-from-module.c index 85f5f1acc3793..edfc218bb3c9e 100644 --- a/clang/test/ClangScanDeps/modules-symlink-dir-from-module.c +++ b/clang/test/ClangScanDeps/modules-symlink-dir-from-module.c @@ -10,6 +10,8 @@ // RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.in > %t/cdb.json // RUN: ln -s module %t/include/symlink-to-module +// RUN: touch %t/session.timestamp + // RUN: clang-scan-deps -compilation-database %t/cdb.json -j 1 \ // RUN: -format experimental-full -mode=preprocess-dependency-directives \ // RUN: -optimize-args=all -module-files-dir %t/build > %t/deps.json @@ -28,7 +30,7 @@ //--- cdb.json.in [{ "directory": "DIR", - "command": "clang -fsyntax-only DIR/test.c -F DIR/Frameworks -I DIR/include -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache", + "command": "clang -fsyntax-only DIR/test.c -F DIR/Frameworks -I DIR/include -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache -fbuild-session-file=DIR/session.timestamp -fmodules-validate-once-per-build-session", "file": "DIR/test.c" }] diff --git a/clang/test/Modules/build-session-validation-relocated-modules.c b/clang/test/Modules/build-session-validation-relocated-modules.c new file mode 100644 index 0000000000000..173d35528fb4f --- /dev/null +++ b/clang/test/Modules/build-session-validation-relocated-modules.c @@ -0,0 +1,55 @@ +// RUN: rm -rf %t +// RUN: split-file %s %t + +// RUN: touch %t/session.timestamp +// RUN: %clang -fmodules -fimplicit-module-maps -fsyntax-only %t/tu1.c \ +// RUN: -fmodules-cache-path=%t/cache -F%t/preferred_frameworks -F%t/fallback_frameworks \ +// RUN: -fbuild-session-file=%t/session.timestamp -fmodules-validate-once-per-build-session \ +// RUN: -Rmodule-validation 2>&1 | FileCheck %s --check-prefix=NO_RELOC + +// NO_RELOC-NOT: checking if module {{.*}} has relocated + +// RUN: mkdir %t/preferred_frameworks/ +// RUN: cp -r %t/fallback_frameworks/IndirectDep.framework %t/preferred_frameworks/ + +// Verify no relocation checks happen because pcms are newer than timestamp. +// RUN: %clang -fmodules -fimplicit-module-maps -fsyntax-only %t/tu1.c \ +// RUN: -fmodules-cache-path=%t/cache -F%t/preferred_frameworks -F%t/fallback_frameworks \ +// RUN: -fbuild-session-file=%t/session.timestamp -fmodules-validate-once-per-build-session \ +// RUN: -Rmodule-validation 2>&1 | FileCheck %s --check-prefix=NO_RELOC + +// Verify no relocation checks happen even when the build session is new because it is explicitly +// disabled. +// RUN: touch %t/session.timestamp +// RUN: %clang -fmodules -fimplicit-module-maps -fsyntax-only %t/tu1.c \ +// RUN: -fmodules-cache-path=%t/cache -F%t/preferred_frameworks -F%t/fallback_frameworks \ +// RUN: -fbuild-session-file=%t/session.timestamp -fmodules-validate-once-per-build-session \ +// RUN: -Xclang -fno-modules-check-relocated -Rmodule-validation 2>&1 | FileCheck %s --check-prefix=NO_RELOC + +// Ensure future new timestamp doesn't have same time as older one. +// RUN: sleep 1 + +// Now remove the disablement and check. +// RUN: touch %t/session.timestamp +// RUN: %clang -fmodules -fimplicit-module-maps -fsyntax-only %t/tu1.c \ +// RUN: -fmodules-cache-path=%t/cache -F%t/preferred_frameworks -F%t/fallback_frameworks \ +// RUN: -fbuild-session-file=%t/session.timestamp -fmodules-validate-once-per-build-session \ +// RUN: -Rmodule-validation 2>&1 | FileCheck %s --check-prefix=RELOC + +// NO_RELOC-NOT: checking if module {{.*}} has relocated + +// RELOC: checking if module {{.*}} has relocated +// RELOC: module 'IndirectDep' relocated from {{.*}}fallback_frameworks{{.*}} to {{.*}}preferred_frameworks + +//--- fallback_frameworks/DirectDep.framework/Modules/module.modulemap +framework module DirectDep { header "DirectDep.h" } +//--- fallback_frameworks/DirectDep.framework/Headers/DirectDep.h +#include <IndirectDep/IndirectDep.h> + +//--- fallback_frameworks/IndirectDep.framework/Modules/module.modulemap +framework module IndirectDep { header "IndirectDep.h" } +//--- fallback_frameworks/IndirectDep.framework/Headers/IndirectDep.h +int foo(void); + +//--- tu1.c +#include <DirectDep/DirectDep.h> _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
