https://github.com/cyndyishida updated
https://github.com/llvm/llvm-project/pull/136612
>From 046b60179ee5eddc6db2193406bfc2ef10fda3df Mon Sep 17 00:00:00 2001
From: Cyndy Ishida <cyndy_ish...@apple.com>
Date: Mon, 21 Apr 2025 13:20:12 -0700
Subject: [PATCH 1/3] [clang][Modules] Report when` lookupModuleFile` fails and
theres an alternate module file to use
In an explicit build, the dependency scanner generates invocations with
dependencies to module files to use during compilation. The pcm's passed
in the invocations should match the ones that were imported by other
modules that share the same dependencies.
We have seen 'module out of date' bugs caused from incorrect invocations that
mismatch which module file to load.
When this happens & we have information about which modules were
imported from the AST file, add a note to help with investigations, as
that should never occur in a well behaved build scheduled from the
dependency scanner.
---
.../Basic/DiagnosticSerializationKinds.td | 2 +
clang/lib/Serialization/ASTReader.cpp | 23 ++++---
clang/test/Modules/invalid-module-dep.c | 62 +++++++++++++++++++
3 files changed, 79 insertions(+), 8 deletions(-)
create mode 100644 clang/test/Modules/invalid-module-dep.c
diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
index 5fc5937b80d35..3babc5850c013 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
@@ -76,6 +76,8 @@ def err_module_file_missing_top_level_submodule : Error<
"module file '%0' is missing its top-level submodule">, DefaultFatal;
def note_module_file_conflict : Note<
"compiled from '%0' and '%1'">;
+def note_alternate_module_file_imported
+ : Note<"alternate module file '%0' was imported from '%1'">;
def remark_module_import : Remark<
"importing module '%0'%select{| into '%3'}2 from '%1'">,
diff --git a/clang/lib/Serialization/ASTReader.cpp
b/clang/lib/Serialization/ASTReader.cpp
index 70b54b7296882..cf875715aa132 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -3286,6 +3286,7 @@ ASTReader::ReadControlBlock(ModuleFile &F,
time_t StoredModTime = 0;
ASTFileSignature StoredSignature;
std::string ImportedFile;
+ std::string StoredFile;
// For prebuilt and explicit modules first consult the file map for
// an override. Note that here we don't search prebuilt module
@@ -3293,7 +3294,9 @@ ASTReader::ReadControlBlock(ModuleFile &F,
// explicit name to file mappings. Also, we will still verify the
// size/signature making sure it is essentially the same file but
// perhaps in a different location.
- if (ImportedKind == MK_PrebuiltModule || ImportedKind ==
MK_ExplicitModule)
+ const bool UseFilemap = (ImportedKind == MK_PrebuiltModule ||
+ ImportedKind == MK_ExplicitModule);
+ if (UseFilemap)
ImportedFile = PP.getHeaderSearchInfo().getPrebuiltModuleFileName(
ImportedName, /*FileMapOnly*/ !IsImportingStdCXXModule);
@@ -3311,12 +3314,11 @@ ASTReader::ReadControlBlock(ModuleFile &F,
SignatureBytes.end());
Blob = Blob.substr(ASTFileSignature::size);
- if (ImportedFile.empty()) {
- // Use BaseDirectoryAsWritten to ensure we use the same path in the
- // ModuleCache as when writing.
- ImportedFile =
- ReadPathBlob(BaseDirectoryAsWritten, Record, Idx, Blob);
- }
+ // Use BaseDirectoryAsWritten to ensure we use the same path in the
+ // ModuleCache as when writing.
+ StoredFile = ReadPathBlob(BaseDirectoryAsWritten, Record, Idx, Blob);
+ if (ImportedFile.empty())
+ ImportedFile = StoredFile;
}
// If our client can't cope with us being out of date, we can't cope with
@@ -3349,9 +3351,14 @@ ASTReader::ReadControlBlock(ModuleFile &F,
.getModuleCache()
.getInMemoryModuleCache()
.isPCMFinal(F.FileName);
- if (isDiagnosedResult(Result, Capabilities) || recompilingFinalized)
+ if (isDiagnosedResult(Result, Capabilities) || recompilingFinalized) {
Diag(diag::note_module_file_imported_by)
<< F.FileName << !F.ModuleName.empty() << F.ModuleName;
+ if (UseFilemap && !F.ModuleName.empty() && !StoredFile.empty() &&
+ StoredFile != ImportedFile)
+ Diag(diag::note_alternate_module_file_imported)
+ << StoredFile << F.ModuleName;
+ }
switch (Result) {
case Failure: return Failure;
diff --git a/clang/test/Modules/invalid-module-dep.c
b/clang/test/Modules/invalid-module-dep.c
new file mode 100644
index 0000000000000..b04a853558376
--- /dev/null
+++ b/clang/test/Modules/invalid-module-dep.c
@@ -0,0 +1,62 @@
+/// This tests the expected error case when there is a mismatch between the
pcm dependencies passed in
+/// the command line with `fmodule-file` and whats encoded in the pcm.
+
+/// The steps are:
+/// 1. Build module (A-1) with no dependencies.
+/// 2. Build the same module with files that resolve from different search
paths. (A-2).
+/// 3. Build module (B) that depends on the earlier module (A-1).
+/// 4. Build client that depends on both modules (A & B) but depends on a
different version (A-2).
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -emit-module -x c -fmodules -fno-implicit-modules -isysroot
%t/Sysroot \
+// RUN: -I%t/Sysroot/usr/include \
+// RUN: -fmodule-name=A %t/Sysroot/usr/include/A/module.modulemap -o
%t/A-1.pcm
+
+// RUN: %clang_cc1 -emit-module -x c -fmodules -fno-implicit-modules -isysroot
%t/Sysroot \
+// RUN: -I%t/BuildDir \
+// RUN: -fmodule-name=A %t/BuildDir/A/module.modulemap -o %t/A-2.pcm
+
+// RUN: %clang_cc1 -emit-module -x c -fmodules -fno-implicit-modules -isysroot
%t/Sysroot \
+// RUN: -I%t/Sysroot/usr/include \
+// RUN: -fmodule-map-file=%t/Sysroot/usr/include/A/module.modulemap \
+// RUN: -fmodule-file=A=%t/A-1.pcm \
+// RUN: -fmodule-name=B %t/Sysroot/usr/include/B/module.modulemap -o
%t/B-1.pcm
+
+// RUN: %clang_cc1 -x c -fmodules -fno-implicit-modules -isysroot %t/Sysroot \
+// RUN: -I%t/BuildDir -I%t/Sysroot/usr/include \
+// RUN: -fmodule-map-file=%t/BuildDir/A/module.modulemap \
+// RUN: -fmodule-map-file=%t/Sysroot/usr/include/B/module.modulemap \
+// RUN: -fmodule-file=A=%t/A-2.pcm -fmodule-file=B=%t/B-1.pcm \
+// RUN: -verify %s
+
+
+#include <A/A.h>
+#include <B/B.h> // expected-error {{out of date and needs to be rebuilt}} \
+ // expected-note {{imported by module 'B'}} \
+ // expected-note-re {{alternate module file {{.*}}A-1.pcm'
was imported from 'B'}}
+
+//--- Sysroot/usr/include/A/module.modulemap
+module A [system] {
+ umbrella header "A.h"
+}
+//--- Sysroot/usr/include/A/A.h
+typedef int A_t;
+
+//--- Sysroot/usr/include/B/module.modulemap
+module B [system] {
+ umbrella header "B.h"
+}
+
+//--- Sysroot/usr/include/B/B.h
+#include <A/A.h>
+typedef int B_t;
+
+//--- BuildDir/A/module.modulemap
+module A [system] {
+ umbrella header "A.h"
+}
+
+//--- BuildDir/A/A.h
+typedef int A_t;
>From b01538ccf890278e0dc935eaa088c48cc789b42b Mon Sep 17 00:00:00 2001
From: Cyndy Ishida <cyndy_ish...@apple.com>
Date: Mon, 21 Apr 2025 13:34:19 -0700
Subject: [PATCH 2/3] [clang][Modules] Clarify error message when size check
fails in lookupModuleFile
---
clang/lib/Serialization/ModuleManager.cpp | 8 ++++++--
clang/test/Modules/explicit-build.cpp | 2 +-
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/clang/lib/Serialization/ModuleManager.cpp
b/clang/lib/Serialization/ModuleManager.cpp
index 61c4e9ed88e9d..d466ea06301a6 100644
--- a/clang/lib/Serialization/ModuleManager.cpp
+++ b/clang/lib/Serialization/ModuleManager.cpp
@@ -110,7 +110,9 @@ ModuleManager::addModule(StringRef FileName, ModuleKind
Type,
// Look for the file entry. This only fails if the expected size or
// modification time differ.
OptionalFileEntryRef Entry;
- if (Type == MK_ExplicitModule || Type == MK_PrebuiltModule) {
+ const bool IgnoreModTime =
+ (Type == MK_ExplicitModule || Type == MK_PrebuiltModule);
+ if (IgnoreModTime) {
// If we're not expecting to pull this file out of the module cache, it
// might have a different mtime due to being moved across filesystems in
// a distributed build. The size must still match, though. (As must the
@@ -120,7 +122,9 @@ ModuleManager::addModule(StringRef FileName, ModuleKind
Type,
// Note: ExpectedSize and ExpectedModTime will be 0 for MK_ImplicitModule
// when using an ASTFileSignature.
if (lookupModuleFile(FileName, ExpectedSize, ExpectedModTime, Entry)) {
- ErrorStr = "module file has a different size or mtime than expected";
+ ErrorStr = IgnoreModTime
+ ? "module file has a different size than expected"
+ : "module file has a different size or mtime than expected";
return OutOfDate;
}
diff --git a/clang/test/Modules/explicit-build.cpp
b/clang/test/Modules/explicit-build.cpp
index fb65508ccf091..50bba0d09966a 100644
--- a/clang/test/Modules/explicit-build.cpp
+++ b/clang/test/Modules/explicit-build.cpp
@@ -199,6 +199,6 @@
// RUN: -fmodule-file=%t/c.pcm \
// RUN: %s -DHAVE_A -DHAVE_B -DHAVE_C 2>&1 | FileCheck
--check-prefix=CHECK-MISMATCHED-B %s
//
-// CHECK-MISMATCHED-B: fatal error: module file '{{.*}}b.pcm' is out of
date and needs to be rebuilt: module file has a different size or mtime than
expected
+// CHECK-MISMATCHED-B: fatal error: module file '{{.*}}b.pcm' is out of
date and needs to be rebuilt: module file has a different size than expected
// CHECK-MISMATCHED-B-NEXT: note: imported by module 'c'
// CHECK-MISMATCHED-B-NOT: note:
>From b18c1f862e8d3a8accd9cf063e00b2d1c82139f5 Mon Sep 17 00:00:00 2001
From: Cyndy Ishida <cyndy_ish...@apple.com>
Date: Mon, 21 Apr 2025 16:07:14 -0700
Subject: [PATCH 3/3] Improve description in tests
---
clang/test/Modules/invalid-module-dep.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/clang/test/Modules/invalid-module-dep.c
b/clang/test/Modules/invalid-module-dep.c
index b04a853558376..16a435f4683cd 100644
--- a/clang/test/Modules/invalid-module-dep.c
+++ b/clang/test/Modules/invalid-module-dep.c
@@ -2,10 +2,12 @@
/// the command line with `fmodule-file` and whats encoded in the pcm.
/// The steps are:
-/// 1. Build module (A-1) with no dependencies.
-/// 2. Build the same module with files that resolve from different search
paths. (A-2).
-/// 3. Build module (B) that depends on the earlier module (A-1).
-/// 4. Build client that depends on both modules (A & B) but depends on a
different version (A-2).
+/// 1. Build the module A with no dependencies. The first variant to build is
A-1.pcm.
+/// 2. Build the same module with files that resolve from different search
paths.
+/// This variant is named A-2.pcm.
+/// 3. Build module B that depends on the earlier module A-1.pcm.
+/// 4. Build client that directly depends on both modules (A & B),
+/// but depends on a incompatible variant of A (A-2.pcm) for B to use.
// RUN: rm -rf %t
// RUN: split-file %s %t
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
