https://github.com/cyndyishida updated https://github.com/llvm/llvm-project/pull/181836
>From 21e391cde8096028fc8753b001a7d7ce0a316dc9 Mon Sep 17 00:00:00 2001 From: Cyndy Ishida <[email protected]> Date: Mon, 16 Feb 2026 14:49:00 -0800 Subject: [PATCH 1/3] [clang-scan-deps] Enable Module Relocation check * Fix tests that assumed no header search lookup was used. * Most of the test fixes were updating FileDeps that resolved files relative to search paths. --- .../DependencyScanning/DependencyScannerImpl.cpp | 3 --- .../Inputs/module_fmodule_name_cdb.json | 2 +- clang/test/ClangScanDeps/Inputs/modules_cdb.json | 8 ++++---- .../ClangScanDeps/Inputs/modules_cdb_clangcl.json | 8 ++++---- .../header-search-pruning-transitive.c | 4 ++-- .../ClangScanDeps/modules-relocated-mm-macro.c | 15 ++++++--------- .../modules-symlink-dir-from-module.c | 4 +++- clang/test/ClangScanDeps/optimize-vfs.m | 8 ++++---- 8 files changed, 24 insertions(+), 28 deletions(-) diff --git a/clang/lib/DependencyScanning/DependencyScannerImpl.cpp b/clang/lib/DependencyScanning/DependencyScannerImpl.cpp index 28fa2571a24dc..a9a77e5482d08 100644 --- a/clang/lib/DependencyScanning/DependencyScannerImpl.cpp +++ b/clang/lib/DependencyScanning/DependencyScannerImpl.cpp @@ -469,9 +469,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/test/ClangScanDeps/Inputs/module_fmodule_name_cdb.json b/clang/test/ClangScanDeps/Inputs/module_fmodule_name_cdb.json index dec425171875a..8ea160e5985fb 100644 --- a/clang/test/ClangScanDeps/Inputs/module_fmodule_name_cdb.json +++ b/clang/test/ClangScanDeps/Inputs/module_fmodule_name_cdb.json @@ -1,7 +1,7 @@ [ { "directory": "DIR", - "command": "clang -E DIR/modules-fmodule-name-no-module-built.m -IInputs -D INCLUDE_HEADER2 -MD -MF DIR/modules_cdb.d -fmodules -fcxx-modules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fmodule-name=header3 -fimplicit-module-maps", + "command": "clang -E DIR/modules-fmodule-name-no-module-built.m -IDIR/Inputs -D INCLUDE_HEADER2 -MD -MF DIR/modules_cdb.d -fmodules -fcxx-modules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fmodule-name=header3 -fimplicit-module-maps", "file": "DIR/modules-fmodule-name-no-module-built.m" } ] diff --git a/clang/test/ClangScanDeps/Inputs/modules_cdb.json b/clang/test/ClangScanDeps/Inputs/modules_cdb.json index a0c5123cd2124..42b6c44f16850 100644 --- a/clang/test/ClangScanDeps/Inputs/modules_cdb.json +++ b/clang/test/ClangScanDeps/Inputs/modules_cdb.json @@ -1,22 +1,22 @@ [ { "directory": "DIR", - "command": "clang -E DIR/modules_cdb_input2.cpp -IInputs -D INCLUDE_HEADER2 -MD -MF DIR/modules_cdb2.d -fmodules -fcxx-modules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps", + "command": "clang -E DIR/modules_cdb_input2.cpp -IDIR/Inputs -D INCLUDE_HEADER2 -MD -MF DIR/modules_cdb2.d -fmodules -fcxx-modules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps", "file": "DIR/modules_cdb_input2.cpp" }, { "directory": "DIR", - "command": "clang -E DIR/modules_cdb_input.cpp -IInputs -fmodules -fcxx-modules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps", + "command": "clang -E DIR/modules_cdb_input.cpp -IDIR/Inputs -fmodules -fcxx-modules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps", "file": "DIR/modules_cdb_input.cpp" }, { "directory": "DIR", - "command": "clang -E DIR/modules_cdb_input.cpp -IInputs -fmodules -fcxx-modules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps -o a.o", + "command": "clang -E DIR/modules_cdb_input.cpp -IDIR/Inputs -fmodules -fcxx-modules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps -o a.o", "file": "DIR/modules_cdb_input.cpp" }, { "directory": "DIR", - "command": "clang -E DIR/modules_cdb_input.cpp -IInputs -fmodules -fcxx-modules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps -o b.o", + "command": "clang -E DIR/modules_cdb_input.cpp -IDIR/Inputs -fmodules -fcxx-modules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps -o b.o", "file": "DIR/modules_cdb_input.cpp" } ] diff --git a/clang/test/ClangScanDeps/Inputs/modules_cdb_clangcl.json b/clang/test/ClangScanDeps/Inputs/modules_cdb_clangcl.json index a1f12867c45d5..bb67c46b0d422 100644 --- a/clang/test/ClangScanDeps/Inputs/modules_cdb_clangcl.json +++ b/clang/test/ClangScanDeps/Inputs/modules_cdb_clangcl.json @@ -1,22 +1,22 @@ [ { "directory": "DIR", - "command": "clang-cl /E /IInputs /D INCLUDE_HEADER2 /clang:-MD /clang:-MF /clang:DIR/modules_cdb2_clangcl.d /clang:-fmodules /clang:-fcxx-modules /clang:-fmodules-cache-path=DIR/module-cache_clangcl /clang:-fimplicit-modules /clang:-fimplicit-module-maps -- DIR/modules_cdb_input2.cpp", + "command": "clang-cl /E /IDIR/Inputs /D INCLUDE_HEADER2 /clang:-MD /clang:-MF /clang:DIR/modules_cdb2_clangcl.d /clang:-fmodules /clang:-fcxx-modules /clang:-fmodules-cache-path=DIR/module-cache_clangcl /clang:-fimplicit-modules /clang:-fimplicit-module-maps -- DIR/modules_cdb_input2.cpp", "file": "DIR/modules_cdb_input2.cpp" }, { "directory": "DIR", - "command": "clang-cl /E /IInputs /clang:-fmodules /clang:-fcxx-modules /clang:-fmodules-cache-path=DIR/module-cache_clangcl /clang:-fimplicit-modules /clang:-fimplicit-module-maps -- DIR/modules_cdb_input.cpp", + "command": "clang-cl /E /IDIR/Inputs /clang:-fmodules /clang:-fcxx-modules /clang:-fmodules-cache-path=DIR/module-cache_clangcl /clang:-fimplicit-modules /clang:-fimplicit-module-maps -- DIR/modules_cdb_input.cpp", "file": "DIR/modules_cdb_input.cpp" }, { "directory": "DIR", - "command": "clang-cl /E /IInputs /clang:-fmodules /clang:-fcxx-modules /clang:-fmodules-cache-path=DIR/module-cache_clangcl /clang:-fimplicit-modules /clang:-fimplicit-module-maps -o a.o -- DIR/modules_cdb_input.cpp", + "command": "clang-cl /E /IDIR/Inputs /clang:-fmodules /clang:-fcxx-modules /clang:-fmodules-cache-path=DIR/module-cache_clangcl /clang:-fimplicit-modules /clang:-fimplicit-module-maps -o a.o -- DIR/modules_cdb_input.cpp", "file": "DIR/modules_cdb_input.cpp" }, { "directory": "DIR", - "command": "clang-cl /E /IInputs /clang:-fmodules /clang:-fcxx-modules /clang:-fmodules-cache-path=DIR/module-cache_clangcl /clang:-fimplicit-modules /clang:-fimplicit-module-maps -o b.o -- DIR/modules_cdb_input.cpp", + "command": "clang-cl /E /IDIR/Inputs /clang:-fmodules /clang:-fcxx-modules /clang:-fmodules-cache-path=DIR/module-cache_clangcl /clang:-fimplicit-modules /clang:-fimplicit-module-maps -o b.o -- DIR/modules_cdb_input.cpp", "file": "DIR/modules_cdb_input.cpp" } ] diff --git a/clang/test/ClangScanDeps/header-search-pruning-transitive.c b/clang/test/ClangScanDeps/header-search-pruning-transitive.c index 1e829bb02ddc4..a6131f0632e6b 100644 --- a/clang/test/ClangScanDeps/header-search-pruning-transitive.c +++ b/clang/test/ClangScanDeps/header-search-pruning-transitive.c @@ -41,14 +41,14 @@ module X { header "X.h" } [{ "file": "DIR/test.c", "directory": "DIR", - "command": "clang -fsyntax-only test.c -fmodules -fimplicit-modules -fmodules-cache-path=DIR/module-cache -fimplicit-module-maps -Ibegin -Ia -Ib -Iend" + "command": "clang -fsyntax-only DIR/test.c -fmodules -fimplicit-modules -fmodules-cache-path=DIR/module-cache -fimplicit-module-maps -IDIR/begin -IDIR/a -IDIR/b -IDIR/end " }] //--- cdb_without_a.json.template [{ "file": "DIR/test.c", "directory": "DIR", - "command": "clang -fsyntax-only test.c -fmodules -fimplicit-modules -fmodules-cache-path=DIR/module-cache -fimplicit-module-maps -Ibegin -Ib -Iend" + "command": "clang -fsyntax-only DIR/test.c -fmodules -fimplicit-modules -fmodules-cache-path=DIR/module-cache -fimplicit-module-maps -IDIR/begin -IDIR/b -IDIR/end -IDIR" }] // RUN: sed -e "s|DIR|%/t|g" %t/cdb_with_a.json.template > %t/cdb_with_a.json 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/ClangScanDeps/optimize-vfs.m b/clang/test/ClangScanDeps/optimize-vfs.m index 2043a603179fe..16a1da828ec1f 100644 --- a/clang/test/ClangScanDeps/optimize-vfs.m +++ b/clang/test/ClangScanDeps/optimize-vfs.m @@ -74,22 +74,22 @@ [ { "directory": "DIR", - "command": "clang -c DIR/0.m -Imodules/A -Imodules/B -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-module-maps -ivfsoverlay build/unused-vfs.yaml -ivfsoverlay build/unused2-vfs.yaml -ivfsoverlay build/vfs.yaml", + "command": "clang -c DIR/0.m -IDIR/modules/A -IDIR/modules/B -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-module-maps -ivfsoverlay build/unused-vfs.yaml -ivfsoverlay build/unused2-vfs.yaml -ivfsoverlay build/vfs.yaml", "file": "DIR/0.m" }, { "directory": "DIR", - "command": "clang -c DIR/A.m -Imodules/A -Imodules/B -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-module-maps -ivfsoverlay build/vfs.yaml -ivfsoverlay build/unused-vfs.yaml", + "command": "clang -c DIR/A.m -IDIR/modules/A -IDIR/modules/B -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-module-maps -ivfsoverlay build/vfs.yaml -ivfsoverlay build/unused-vfs.yaml", "file": "DIR/A.m" }, { "directory": "DIR", - "command": "clang -c DIR/B.m -Imodules/B -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-module-maps -ivfsoverlay build/unused-vfs.yaml -ivfsoverlay build/vfs.yaml", + "command": "clang -c DIR/B.m -IDIR/modules/B -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-module-maps -ivfsoverlay build/unused-vfs.yaml -ivfsoverlay build/vfs.yaml", "file": "DIR/B.m" }, { "directory": "DIR", - "command": "clang -c DIR/C.m -Imodules/A -Imodules/B -Imodules/C -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-module-maps -ivfsoverlay build/vfs.yaml -ivfsoverlay build/unused-vfs.yaml", + "command": "clang -c DIR/C.m -IDIR/modules/A -IDIR/modules/B -IDIR/modules/C -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-module-maps -ivfsoverlay build/vfs.yaml -ivfsoverlay build/unused-vfs.yaml", "file": "DIR/C.m" } ] >From af7be12dcb23b609a355a4c4be814304b6e3b840 Mon Sep 17 00:00:00 2001 From: Cyndy Ishida <[email protected]> Date: Mon, 16 Feb 2026 09:19:44 -0800 Subject: [PATCH 2/3] [clang][Modules] Handle relocated modules during implicit module builds * 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 if new version of libraries are added within the same build session. --- .../Basic/DiagnosticSerializationKinds.td | 7 ++ clang/include/clang/Serialization/ASTReader.h | 7 ++ clang/lib/Serialization/ASTReader.cpp | 109 +++++++++++++----- ...ild-session-validation-relocated-modules.c | 68 +++++++++++ ...ild-session-validation-relocated-modules.c | 55 +++++++++ 5 files changed, 216 insertions(+), 30 deletions(-) create mode 100644 clang/test/ClangScanDeps/build-session-validation-relocated-modules.c create mode 100644 clang/test/Modules/build-session-validation-relocated-modules.c diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td b/clang/include/clang/Basic/DiagnosticSerializationKinds.td index 229f0bacbd796..998212ce4c019 100644 --- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td +++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td @@ -96,6 +96,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..fe02f7c6c7916 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -1590,6 +1590,13 @@ class ASTReader void ParseLineTable(ModuleFile &F, const RecordData &Record); llvm::Error ReadSourceManagerBlock(ModuleFile &F); SourceLocation getImportLocation(ModuleFile *F); + + /// If a relocation check happened, the optional holds a pointer to the + /// discovered Module. The second element determines whether to emit related + /// validation errors. + using RelocationCheckResult = std::pair<std::optional<Module *>, bool>; + RelocationCheckResult checkIfModuleRelocated(ModuleFile &F, + bool DirectoryCheck = false); ASTReadResult ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F, const ModuleFile *ImportedBy, unsigned ClientLoadCapabilities); diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index f3902d57e3d1f..a829419d04673 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -3158,6 +3158,49 @@ ASTReader::ASTReadResult ASTReader::ReadOptionsBlock( } } +ASTReader::RelocationCheckResult +ASTReader::checkIfModuleRelocated(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, @@ -3547,31 +3590,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] = + checkIfModuleRelocated(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: @@ -4564,19 +4612,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] = + checkIfModuleRelocated(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..18bbb55609191 --- /dev/null +++ b/clang/test/ClangScanDeps/build-session-validation-relocated-modules.c @@ -0,0 +1,68 @@ +// 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 2>&1 | 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: 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 2>&1 | 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/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> >From 80ceaeb80860b79a2a5bd0a52a7e67b21bf8968e Mon Sep 17 00:00:00 2001 From: Cyndy Ishida <[email protected]> Date: Tue, 17 Feb 2026 16:03:24 -0800 Subject: [PATCH 3/3] Address review comments * Speculatively fix failing test on different platforms --- clang/include/clang/Serialization/ASTReader.h | 20 +++++++++++++------ clang/lib/Serialization/ASTReader.cpp | 8 ++++---- ...ild-session-validation-relocated-modules.c | 8 +++++--- 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index fe02f7c6c7916..f254459ce933d 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -1591,12 +1591,20 @@ class ASTReader llvm::Error ReadSourceManagerBlock(ModuleFile &F); SourceLocation getImportLocation(ModuleFile *F); - /// If a relocation check happened, the optional holds a pointer to the - /// discovered Module. The second element determines whether to emit related - /// validation errors. - using RelocationCheckResult = std::pair<std::optional<Module *>, bool>; - RelocationCheckResult checkIfModuleRelocated(ModuleFile &F, - bool DirectoryCheck = false); + /// 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/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index a829419d04673..c5ef3f02a2aa3 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -3158,8 +3158,8 @@ ASTReader::ASTReadResult ASTReader::ReadOptionsBlock( } } -ASTReader::RelocationCheckResult -ASTReader::checkIfModuleRelocated(ModuleFile &F, bool DirectoryCheck) { +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 & @@ -3592,7 +3592,7 @@ ASTReader::ReadControlBlock(ModuleFile &F, F.BaseDirectory = std::string(Blob); auto [MaybeM, IgnoreError] = - checkIfModuleRelocated(F, /*DirectoryCheck=*/true); + getModuleForRelocationChecks(F, /*DirectoryCheck=*/true); if (!MaybeM.has_value()) break; @@ -4613,7 +4613,7 @@ ASTReader::ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F, assert(!F.ModuleName.empty() && "MODULE_NAME should come before MODULE_MAP_FILE"); auto [MaybeM, IgnoreError] = - checkIfModuleRelocated(F, /*DirectoryCheck=*/false); + 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. diff --git a/clang/test/ClangScanDeps/build-session-validation-relocated-modules.c b/clang/test/ClangScanDeps/build-session-validation-relocated-modules.c index 18bbb55609191..8b5a1cffc958e 100644 --- a/clang/test/ClangScanDeps/build-session-validation-relocated-modules.c +++ b/clang/test/ClangScanDeps/build-session-validation-relocated-modules.c @@ -12,19 +12,21 @@ // 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 2>&1 | FileCheck %s --check-prefix=DEPS1 +// 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: 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 2>&1 | FileCheck %s --check-prefix=DEPS2 +// 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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
