https://github.com/cyndyishida created https://github.com/llvm/llvm-project/pull/197203
The relocation check used `IFVT >= session` while input validation used `IFVT > session`, silently skipping the relocation check when both landed in the same truncated second. Share a single helper with the strict comparison. This was noticed when the accompanying test was flaky on green dragon macOS bot. Also: * Drop the test's `sleep 1` timing dependency by future-dating the session timestamp directly. * Remove the dead `touch %t/session.timestamp` lines in the ClangScanDeps test (the scanner ignores the file's mtime). hopefully resolves: rdar://173816745 >From f4aeea072214309fa8e233feac01f6266f9fac02 Mon Sep 17 00:00:00 2001 From: Cyndy Ishida <[email protected]> Date: Tue, 12 May 2026 07:03:07 -0700 Subject: [PATCH] [clang][Modules] Use strict comparison at build-session validation boundary The relocation check used `IFVT >= session` while input validation used `IFVT > session`, silently skipping the relocation check when both landed in the same truncated second. Share a single helper with the strict comparison. Also: * Drop the test's `sleep 1` timing dependency by future-dating the session timestamp directly. * Remove the dead `touch %t/session.timestamp` lines in the ClangScanDeps test (the scanner ignores the file's mtime). hopefully resolves: rdar://173816745 --- clang/lib/Serialization/ASTReader.cpp | 30 +++++++++++++------ ...ild-session-validation-relocated-modules.c | 4 +-- ...ild-session-validation-relocated-modules.c | 8 ++--- 3 files changed, 26 insertions(+), 16 deletions(-) diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index dfd714dd53814..0f834859e982a 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -3179,6 +3179,18 @@ ASTReader::ASTReadResult ASTReader::ReadOptionsBlock( } } +/// Returns {build-session validation applies, MF was validated this session}. +static std::pair<bool, bool> +wasValidatedInBuildSession(const ModuleFile &MF, + const HeaderSearchOptions &HSOpts) { + const bool EnablesBSValidation = + HSOpts.ModulesValidateOncePerBuildSession && MF.Kind == MK_ImplicitModule; + const bool WasValidated = + EnablesBSValidation && + MF.InputFilesValidationTimestamp > HSOpts.BuildSessionTimestamp; + return {EnablesBSValidation, WasValidated}; +} + ASTReader::RelocationResult ASTReader::getModuleForRelocationChecks(ModuleFile &F, bool DirectoryCheck) { // Don't emit module relocation errors if we have -fno-validate-pch. @@ -3201,12 +3213,13 @@ ASTReader::getModuleForRelocationChecks(ModuleFile &F, bool DirectoryCheck) { // 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.ModTime) >= HSOpts.BuildSessionTimestamp) - return {std::nullopt, IgnoreError}; - } + auto [EnablesBSValidation, WasValidated] = + wasValidatedInBuildSession(F, HSOpts); + if (WasValidated) + return {std::nullopt, IgnoreError}; + if (EnablesBSValidation && + static_cast<uint64_t>(F.ModTime) >= HSOpts.BuildSessionTimestamp) + return {std::nullopt, IgnoreError}; Diag(diag::remark_module_check_relocation) << F.ModuleName << F.FileName; @@ -3294,9 +3307,8 @@ ASTReader::ReadControlBlock(ModuleFile &F, F.InputFilesValidationStatus = ValidateSystemInputs ? InputFilesValidation::AllFiles : InputFilesValidation::UserFiles; - if (HSOpts.ModulesValidateOncePerBuildSession && - F.InputFilesValidationTimestamp > HSOpts.BuildSessionTimestamp && - F.Kind == MK_ImplicitModule) { + auto [_, WasValidated] = wasValidatedInBuildSession(F, HSOpts); + if (WasValidated) { N = ForceValidateUserInputs ? NumUserInputs : 0; F.InputFilesValidationStatus = ForceValidateUserInputs diff --git a/clang/test/ClangScanDeps/build-session-validation-relocated-modules.c b/clang/test/ClangScanDeps/build-session-validation-relocated-modules.c index 47919229512d0..5a0744718685b 100644 --- a/clang/test/ClangScanDeps/build-session-validation-relocated-modules.c +++ b/clang/test/ClangScanDeps/build-session-validation-relocated-modules.c @@ -12,14 +12,12 @@ // 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/ @@ -46,6 +44,8 @@ } ] +//--- session.timestamp + //--- fallback_frameworks/MovedDep.framework/Modules/module.modulemap framework module MovedDep { header "MovedDep.h" } //--- fallback_frameworks/MovedDep.framework/Headers/MovedDep.h diff --git a/clang/test/Modules/build-session-validation-relocated-modules.c b/clang/test/Modules/build-session-validation-relocated-modules.c index 173d35528fb4f..5a1a249896040 100644 --- a/clang/test/Modules/build-session-validation-relocated-modules.c +++ b/clang/test/Modules/build-session-validation-relocated-modules.c @@ -26,11 +26,9 @@ // 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 +// Force session.timestamp into the future so it is strictly greater than any +// cached pcm or timestamp mtime, regardless of wall-clock resolution. +// RUN: %python -c "import os, time; t = time.time() + 3600; os.utime('%t/session.timestamp', (t, t))" // 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 \ _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
