https://github.com/Bigcheese created
https://github.com/llvm/llvm-project/pull/202367
This adds `-f{no-}modules-validate-umbrella-dirs`. This makes implicitly built
modules detect that a module with an umbrella directory or umbrella header is
out of date if a new header is added to the directory the umbrella covers.
This is a type of "negative dependency", where moving from the absence of a
file to the presence of a file needs to trigger a module rebuild even though no
file in the .d output changed.
This is done by comparing the mtime of every header that should be part of the
module to the mtime of the PCM file.
>From 59f01aefb6882d7939dd678bc48aa5185f3fc568 Mon Sep 17 00:00:00 2001
From: Michael Spencer <[email protected]>
Date: Mon, 8 Jun 2026 17:28:27 +0200
Subject: [PATCH] [clang] Detect when headers are added to umbrella modules
This adds `-f{no-}modules-validate-umbrella-dirs`. This makes
implicitly built modules detect that a module with an umbrella
directory or umbrella header is out of date if a new header is added
to the directory the umbrella covers.
This is a type of "negative dependency", where moving from the absence
of a file to the presence of a file needs to trigger a module rebuild
even though no file in the .d output changed.
This is done by comparing the mtime of every header that should be
part of the module to the mtime of the PCM file.
---
.../Basic/DiagnosticSerializationKinds.td | 5 +
clang/include/clang/Lex/HeaderSearchOptions.h | 8 ++
clang/include/clang/Lex/ModuleMap.h | 4 +
clang/include/clang/Options/Options.td | 6 +
clang/include/clang/Serialization/ASTReader.h | 5 +
clang/lib/Frontend/FrontendAction.cpp | 5 +-
clang/lib/Lex/ModuleMap.cpp | 6 +
clang/lib/Lex/PPLexerChange.cpp | 6 +-
clang/lib/Serialization/ASTReader.cpp | 81 +++++++++++
clang/test/Modules/validate-umbrella-dir.c | 128 ++++++++++++++++++
10 files changed, 246 insertions(+), 8 deletions(-)
create mode 100644 clang/test/Modules/validate-umbrella-dir.c
diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
index 5c74caf010ad9..7283bd82724ad 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
@@ -112,6 +112,11 @@ def remark_module_check_relocation
def remark_module_relocated : Remark<"module '%0' relocated from '%1' to
'%2'">,
ShowInSystemHeader,
InGroup<ModuleValidation>;
+def remark_module_umbrella_out_of_date
+ : Remark<"module '%0' is out of date because header '%1' was added to its "
+ "umbrella %select{directory|header's directory}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/Lex/HeaderSearchOptions.h
b/clang/include/clang/Lex/HeaderSearchOptions.h
index fd46ea223bae1..8baa14607113c 100644
--- a/clang/include/clang/Lex/HeaderSearchOptions.h
+++ b/clang/include/clang/Lex/HeaderSearchOptions.h
@@ -222,6 +222,13 @@ class HeaderSearchOptions {
LLVM_PREFERRED_TYPE(bool)
unsigned ModulesForceValidateUserHeaders : 1;
+ /// Whether to re-stat umbrella directories when a module is loaded and treat
+ /// the module as out of date if a header has been added to one since it was
+ /// built. This catches a "negative dependency" that ordinary input-file
+ /// validation misses, because added headers were never recorded as inputs.
+ LLVM_PREFERRED_TYPE(bool)
+ unsigned ModulesValidateUmbrellaDirs : 1;
+
// Whether the content of input files should be hashed and used to
// validate consistency.
LLVM_PREFERRED_TYPE(bool)
@@ -297,6 +304,7 @@ class HeaderSearchOptions {
ModulesValidateOncePerBuildSession(false),
ModulesValidateSystemHeaders(false),
ModulesForceValidateUserHeaders(true),
+ ModulesValidateUmbrellaDirs(false),
ValidateASTInputFilesContent(false),
ForceCheckCXX20ModulesInputFiles(false), UseDebugInfo(false),
ModulesValidateDiagnosticOptions(true),
diff --git a/clang/include/clang/Lex/ModuleMap.h
b/clang/include/clang/Lex/ModuleMap.h
index 12f8dbb0b6090..2f2b11e76850b 100644
--- a/clang/include/clang/Lex/ModuleMap.h
+++ b/clang/include/clang/Lex/ModuleMap.h
@@ -512,6 +512,10 @@ class ModuleMap {
/// marked 'unavailable'.
bool isHeaderInUnavailableModule(FileEntryRef Header) const;
+ /// Has an extension that is treated as a header when collecting the contents
+ /// of an umbrella directory (see \c collectModuleHeaderIncludes).
+ static bool isModularHeaderExtension(StringRef FileName);
+
/// Determine whether the given header is unavailable as part
/// of the specified module.
bool isHeaderUnavailableInModule(FileEntryRef Header,
diff --git a/clang/include/clang/Options/Options.td
b/clang/include/clang/Options/Options.td
index 8451a3698ef17..4ce4be0c0c7cd 100644
--- a/clang/include/clang/Options/Options.td
+++ b/clang/include/clang/Options/Options.td
@@ -3632,6 +3632,12 @@ defm modules_validate_system_headers : BoolOption<"f",
"modules-validate-system-
PosFlag<SetTrue, [], [ClangOption, CC1Option],
"Validate the system headers that a module depends on when loading
the module">,
NegFlag<SetFalse, [], []>>, Group<i_Group>;
+defm modules_validate_umbrella_dirs : BoolOption<"f",
"modules-validate-umbrella-dirs",
+ HeaderSearchOpts<"ModulesValidateUmbrellaDirs">, DefaultFalse,
+ PosFlag<SetTrue, [], [ClangOption, CC1Option],
+ "Re-stat umbrella directories when loading a module and treat the "
+ "module as out of date if a header has been added">,
+ NegFlag<SetFalse, [], []>>, Group<i_Group>;
def fno_modules_validate_textual_header_includes :
Flag<["-"], "fno-modules-validate-textual-header-includes">,
Group<f_Group>, Flags<[]>, Visibility<[ClangOption, CC1Option]>,
diff --git a/clang/include/clang/Serialization/ASTReader.h
b/clang/include/clang/Serialization/ASTReader.h
index 03b8fa74a81fe..c821f81ac3192 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -1541,6 +1541,11 @@ class ASTReader : public ExternalPreprocessorSource,
SmallVectorImpl<ImportedModule> &Loaded,
const ModuleFile *ImportedBy,
unsigned ClientLoadCapabilities);
+
+ /// Returns true if a header has been added to one of \p F's umbrellas since
+ /// it was built, making the module out of date. Gated by
+ /// HeaderSearchOptions::ModulesValidateUmbrellaDirs.
+ bool isUmbrellaDirOutOfDate(ModuleFile &F);
static ASTReadResult
ReadOptionsBlock(llvm::BitstreamCursor &Stream, StringRef Filename,
unsigned ClientLoadCapabilities,
diff --git a/clang/lib/Frontend/FrontendAction.cpp
b/clang/lib/Frontend/FrontendAction.cpp
index 7754861fabaf0..28598192609f2 100644
--- a/clang/lib/Frontend/FrontendAction.cpp
+++ b/clang/lib/Frontend/FrontendAction.cpp
@@ -31,6 +31,7 @@
#include "clang/Frontend/Utils.h"
#include "clang/Lex/HeaderSearch.h"
#include "clang/Lex/LiteralSupport.h"
+#include "clang/Lex/ModuleMap.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Lex/PreprocessorOptions.h"
#include "clang/Parse/ParseAST.h"
@@ -628,9 +629,7 @@ static std::error_code collectModuleHeaderIncludes(
Dir != End && !EC; Dir.increment(EC)) {
// Check whether this entry has an extension typically associated with
// headers.
- if (!llvm::StringSwitch<bool>(llvm::sys::path::extension(Dir->path()))
- .Cases({".h", ".H", ".hh", ".hpp"}, true)
- .Default(false))
+ if (!ModMap.isModularHeaderExtension(Dir->path()))
continue;
// Compute the relative path from the directory to this file.
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 6c07386f89010..4ff4227d61b30 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -841,6 +841,12 @@ bool ModuleMap::isHeaderInUnavailableModule(FileEntryRef
Header) const {
return isHeaderUnavailableInModule(Header, nullptr);
}
+bool ModuleMap::isModularHeaderExtension(StringRef FileName) {
+ return llvm::StringSwitch<bool>(llvm::sys::path::extension(FileName))
+ .Cases({".h", ".H", ".hh", ".hpp"}, true)
+ .Default(false);
+}
+
bool ModuleMap::isHeaderUnavailableInModule(
FileEntryRef Header, const Module *RequestingModule) const {
resolveHeaderDirectives(Header);
diff --git a/clang/lib/Lex/PPLexerChange.cpp b/clang/lib/Lex/PPLexerChange.cpp
index 98ff9a9a04e7c..42997b5cf338e 100644
--- a/clang/lib/Lex/PPLexerChange.cpp
+++ b/clang/lib/Lex/PPLexerChange.cpp
@@ -295,13 +295,9 @@ void
Preprocessor::diagnoseMissingHeaderInUmbrellaDir(const Module &Mod) {
for (llvm::vfs::recursive_directory_iterator Entry(FS, Dir->getName(), EC),
End;
Entry != End && !EC; Entry.increment(EC)) {
- using llvm::StringSwitch;
-
// Check whether this entry has an extension typically associated with
// headers.
- if (!StringSwitch<bool>(llvm::sys::path::extension(Entry->path()))
- .Cases({".h", ".H", ".hh", ".hpp"}, true)
- .Default(false))
+ if (!ModMap.isModularHeaderExtension(Entry->path()))
continue;
if (auto Header = getFileManager().getOptionalFileRef(Entry->path()))
diff --git a/clang/lib/Serialization/ASTReader.cpp
b/clang/lib/Serialization/ASTReader.cpp
index 74a7b51368c28..5d0215c64418e 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -3234,6 +3234,82 @@ ASTReader::getModuleForRelocationChecks(ModuleFile &F,
bool DirectoryCheck) {
return {M, IgnoreError};
}
+/// Check whether the effective umbrella directory of \p M (an umbrella "dir",
+/// or the directory containing an umbrella header) contains a modular header
+/// newer than the module file timestamp \p ModTime, recursing into submodules.
+/// Such a header would have been collected on a clean build, so the module is
+/// out of date. On success the offending header, its module, and which
umbrella
+/// kind \p M has are reported via the out-parameters.
+///
+/// Comparing modification times rather than the recorded input set is what
lets
+/// this converge: an added umbrella-header sibling is not recorded as an
input,
+/// but after a rebuild the module file is newer than the header.
+static bool isUmbrellaContentsNewer(ModuleMap &ModMap, Module *M,
+ time_t ModTime, FileManager &FileMgr,
+ std::string &OutOfDateHeader,
+ Module *&OutOfDateModule,
+ bool &OutOfDateIsUmbrellaHeader) {
+ if (OptionalDirectoryEntryRef Dir = M->getEffectiveUmbrellaDir()) {
+ llvm::vfs::FileSystem &FS = FileMgr.getVirtualFileSystem();
+ SmallString<128> DirNative;
+ llvm::sys::path::native(Dir->getName(), DirNative);
+ std::error_code EC;
+ for (llvm::vfs::recursive_directory_iterator I(FS, DirNative, EC), End;
+ I != End && !EC; I.increment(EC)) {
+ if (!ModMap.isModularHeaderExtension(I->path()))
+ continue;
+
+ auto Header = FileMgr.getOptionalFileRef(I->path());
+ if (!Header)
+ continue;
+
+ // Match collectModuleHeaderIncludes: ignore headers a clean build would
+ // not collect (excluded or otherwise unavailable).
+ if (ModMap.isHeaderUnavailableInModule(*Header, M))
+ continue;
+
+ if (Header->getModificationTime() > ModTime) {
+ OutOfDateHeader = Header->getName().str();
+ OutOfDateModule = M;
+ OutOfDateIsUmbrellaHeader = !M->getUmbrellaDirAsWritten();
+ return true;
+ }
+ }
+ }
+
+ for (Module *Sub : M->submodules())
+ if (isUmbrellaContentsNewer(ModMap, Sub, ModTime, FileMgr, OutOfDateHeader,
+ OutOfDateModule, OutOfDateIsUmbrellaHeader))
+ return true;
+
+ return false;
+}
+
+bool ASTReader::isUmbrellaDirOutOfDate(ModuleFile &F) {
+ HeaderSearch &HS = PP.getHeaderSearchInfo();
+ // TODO: This is probably too slow due to the extra searching, we should use
+ // the submodule info from the PCM if the module isn't already in the
+ // module map.
+ Module *M = HS.lookupModule(F.ModuleName, F.ImportLoc, /*AllowSearch=*/true,
+ /*AllowExtraModuleMapSearch=*/true);
+ if (!M)
+ return false;
+
+ ModuleMap &ModMap = HS.getModuleMap();
+ std::string OutOfDateHeader;
+ Module *OutOfDateModule = nullptr;
+ bool IsUmbrellaHeader = false;
+ if (!isUmbrellaContentsNewer(ModMap, M, F.ModTime, PP.getFileManager(),
+ OutOfDateHeader, OutOfDateModule,
+ IsUmbrellaHeader))
+ return false;
+
+ Diag(diag::remark_module_umbrella_out_of_date)
+ << OutOfDateModule->getFullModuleName() << OutOfDateHeader
+ << IsUmbrellaHeader;
+ return true;
+}
+
ASTReader::ASTReadResult
ASTReader::ReadControlBlock(ModuleFile &F,
SmallVectorImpl<ImportedModule> &Loaded,
@@ -3325,6 +3401,11 @@ ASTReader::ReadControlBlock(ModuleFile &F,
if (!IF.getFile() || IF.isOutOfDate())
return OutOfDate;
}
+
+ // A header added to an umbrella since the module was built is a
+ // negative dependency that ordinary input-file validation misses.
+ if (HSOpts.ModulesValidateUmbrellaDirs && isUmbrellaDirOutOfDate(F))
+ return OutOfDate;
} else {
F.InputFilesValidationStatus = InputFilesValidation::Disabled;
}
diff --git a/clang/test/Modules/validate-umbrella-dir.c
b/clang/test/Modules/validate-umbrella-dir.c
new file mode 100644
index 0000000000000..256d9369d7a72
--- /dev/null
+++ b/clang/test/Modules/validate-umbrella-dir.c
@@ -0,0 +1,128 @@
+// Adding a header to an umbrella directory (or to an umbrella header's
+// directory) is a "negative dependency": a clean build would pick it up, but
+// ordinary input-file validation only re-stats headers that already were
+// inputs. Check that -fmodules-validate-umbrella-dirs catches it.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//===----------------------------------------------------------------------===//
+// Umbrella directory.
+//===----------------------------------------------------------------------===//
+
+//--- dir/module.modulemap
+module Umbrella {
+ umbrella "umbrella"
+ exclude header "umbrella/excluded.h"
+}
+//--- dir/umbrella/a.h
+//--- dir/umbrella/excluded.h
+#error "this header is excluded and should never be compiled"
+//--- dir/tu.c
+#include "umbrella/a.h"
+
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t/dir/cache -I %t/dir \
+// RUN: -Rmodule-build %t/dir/tu.c 2>&1 | FileCheck %s --check-prefix=BUILD
-DNAME=Umbrella
+
+// No changes: up to date with or without the flag.
+//
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t/dir/cache -I %t/dir \
+// RUN: -Rmodule-build %t/dir/tu.c 2>&1 | FileCheck %s
--check-prefix=NOBUILD --allow-empty -DNAME=Umbrella
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t/dir/cache -I %t/dir
-fmodules-validate-umbrella-dirs \
+// RUN: -Rmodule-build %t/dir/tu.c 2>&1 | FileCheck %s
--check-prefix=NOBUILD --allow-empty -DNAME=Umbrella
+
+// Touching an excluded header must not rebuild: a clean build ignores it.
+// (The sleep makes later additions strictly newer than the module file.)
+//
+// RUN: sleep 1
+// RUN: touch %t/dir/umbrella/excluded.h
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t/dir/cache -I %t/dir
-fmodules-validate-umbrella-dirs \
+// RUN: -Rmodule-build %t/dir/tu.c 2>&1 | FileCheck %s
--check-prefix=NOBUILD --allow-empty -DNAME=Umbrella
+
+// RUN: touch %t/dir/umbrella/b.h
+
+// Adding b.h rebuilds only under the flag.
+//
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t/dir/cache -I %t/dir \
+// RUN: -Rmodule-build %t/dir/tu.c 2>&1 | FileCheck %s
--check-prefix=NOBUILD --allow-empty -DNAME=Umbrella
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t/dir/cache -I %t/dir
-fmodules-validate-umbrella-dirs \
+// RUN: -Rmodule-build -Rmodule-validation %t/dir/tu.c 2>&1 | FileCheck %s
--check-prefixes=BUILD,DIR-REMARK -DNAME=Umbrella
+
+// Converges: the rebuilt module is newer than b.h, so no further rebuild.
+//
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t/dir/cache -I %t/dir
-fmodules-validate-umbrella-dirs \
+// RUN: -Rmodule-build %t/dir/tu.c 2>&1 | FileCheck %s
--check-prefix=NOBUILD --allow-empty -DNAME=Umbrella
+
+//===----------------------------------------------------------------------===//
+// Umbrella header. A new sibling changes -Wincomplete-umbrella, so a clean
+// build differs and the module should rebuild under the flag.
+//===----------------------------------------------------------------------===//
+
+//--- hdr/module.modulemap
+module UH { umbrella header "UH.h" }
+//--- hdr/UH.h
+#include "a.h"
+//--- hdr/a.h
+//--- hdr/tu.c
+#include "UH.h"
+
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t/hdr/cache -I %t/hdr \
+// RUN: -Rmodule-build %t/hdr/tu.c 2>&1 | FileCheck %s --check-prefix=BUILD
-DNAME=UH
+
+// RUN: sleep 1
+// RUN: touch %t/hdr/b.h
+
+// Adding a sibling rebuilds only under the flag.
+//
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t/hdr/cache -I %t/hdr \
+// RUN: -Rmodule-build %t/hdr/tu.c 2>&1 | FileCheck %s
--check-prefix=NOBUILD --allow-empty -DNAME=UH
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t/hdr/cache -I %t/hdr
-fmodules-validate-umbrella-dirs \
+// RUN: -Rmodule-build -Rmodule-validation %t/hdr/tu.c 2>&1 | FileCheck %s
--check-prefixes=BUILD,HDR-REMARK -DNAME=UH
+
+// Converges.
+//
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t/hdr/cache -I %t/hdr
-fmodules-validate-umbrella-dirs \
+// RUN: -Rmodule-build %t/hdr/tu.c 2>&1 | FileCheck %s
--check-prefix=NOBUILD --allow-empty -DNAME=UH
+
+//===----------------------------------------------------------------------===//
+// Transitive module: the TU imports Top, which pulls in Leaf. Adding to Leaf's
+// umbrella must invalidate Leaf even though the TU never names it (its module
+// map is only parsed via the umbrella check's own lookup).
+//===----------------------------------------------------------------------===//
+
+//--- trans/module.modulemap
+module Top { umbrella "top" export * }
+module Leaf { umbrella "leaf" export * }
+//--- trans/top/top.h
+#include "leaf/leaf.h"
+//--- trans/leaf/leaf.h
+//--- trans/tu.c
+#include "top/top.h"
+
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t/trans/cache -I %t/trans \
+// RUN: -Rmodule-build %t/trans/tu.c 2>&1 | FileCheck %s
--check-prefix=BUILD -DNAME=Leaf
+
+// RUN: sleep 1
+// RUN: touch %t/trans/leaf/leaf2.h
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t/trans/cache -I %t/trans
-fmodules-validate-umbrella-dirs \
+// RUN: -Rmodule-build -Rmodule-validation %t/trans/tu.c 2>&1 | FileCheck %s
--check-prefixes=BUILD,TRANS-REMARK -DNAME=Leaf
+
+// DIR-REMARK: module 'Umbrella' is out of date because header '{{.*}}b.h' was
added to its umbrella directory
+// HDR-REMARK: module 'UH' is out of date because header '{{.*}}b.h' was added
to its umbrella header's directory
+// TRANS-REMARK: module 'Leaf' is out of date because header '{{.*}}leaf2.h'
was added to its umbrella directory
+
+// BUILD: building module '[[NAME]]'
+// NOBUILD-NOT: building module '[[NAME]]'
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
