https://github.com/cyndyishida updated
https://github.com/llvm/llvm-project/pull/137068
>From bb548cb398714ae77b6fd2f782b172d8b9032a2f Mon Sep 17 00:00:00 2001
From: Cyndy Ishida <cyndy_ish...@apple.com>
Date: Wed, 23 Apr 2025 14:47:17 -0700
Subject: [PATCH 1/3] [clang][Modules] Diagnose mismatching pcm dependencies in
explicit builds
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 bugs caused from incorrect invocations that mismatch which module
file to load.
When this happens report it as a warning, to help with investigations, as
that should never occur in a well behaved build scheduled from the
dependency scanner.
The warning flag is off by default.
---
.../Basic/DiagnosticSerializationKinds.td | 5 ++
clang/lib/Serialization/ASTReader.cpp | 20 ++++--
clang/test/Modules/invalid-module-dep.c | 65 +++++++++++++++++++
3 files changed, 86 insertions(+), 4 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..3ff5490cf4168 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
@@ -139,6 +139,11 @@ def warn_decls_in_multiple_modules : Warning<
InGroup<DiagGroup<"decls-in-multiple-modules">>,
DefaultIgnore;
+def warn_lazy_pcm_mismatch
+ : Warning<"loaded module file '%0' conflicts with imported file '%1'">,
+ InGroup<DiagGroup<"lazy-pcm-mismatch">>,
+ DefaultIgnore;
+
def err_failed_to_find_module_file : Error<
"failed to find module file for module '%0'">;
} // let CategoryName
diff --git a/clang/lib/Serialization/ASTReader.cpp
b/clang/lib/Serialization/ASTReader.cpp
index 70b54b7296882..5761dc09a3e59 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
@@ -3311,11 +3312,22 @@ ASTReader::ReadControlBlock(ModuleFile &F,
SignatureBytes.end());
Blob = Blob.substr(ASTFileSignature::size);
+ // Use BaseDirectoryAsWritten to ensure we use the same path in the
+ // ModuleCache as when writing.
+ StoredFile = ReadPathBlob(BaseDirectoryAsWritten, Record, Idx, Blob);
if (ImportedFile.empty()) {
- // Use BaseDirectoryAsWritten to ensure we use the same path in the
- // ModuleCache as when writing.
- ImportedFile =
- ReadPathBlob(BaseDirectoryAsWritten, Record, Idx, Blob);
+ ImportedFile = StoredFile;
+ } else {
+ auto ImportedFileRef =
+ PP.getFileManager().getOptionalFileRef(ImportedFile);
+ auto StoredFileRef =
+ PP.getFileManager().getOptionalFileRef(StoredFile);
+ if ((ImportedFileRef && StoredFileRef) &&
+ (*ImportedFileRef != *StoredFileRef)) {
+ Diag(diag::warn_lazy_pcm_mismatch) << ImportedFile << StoredFile;
+ Diag(diag::note_module_file_imported_by)
+ << F.FileName << !F.ModuleName.empty() << F.ModuleName;
+ }
}
}
diff --git a/clang/test/Modules/invalid-module-dep.c
b/clang/test/Modules/invalid-module-dep.c
new file mode 100644
index 0000000000000..6891a8cbac4fe
--- /dev/null
+++ b/clang/test/Modules/invalid-module-dep.c
@@ -0,0 +1,65 @@
+/// 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 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
+
+// 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: -Wlazy-pcm-mismatch -verify %s
+
+
+#include <A/A.h>
+#include <B/B.h> // expected-warning {{conflicts with imported file}} \
+ // expected-note {{imported by module 'B'}} \
+ // expected-error {{out of date and needs to be rebuilt}} \
+ // expected-note {{imported by module '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 48914a174951090f9346583ce2166da6f2cbe139 Mon Sep 17 00:00:00 2001
From: Cyndy Ishida <cyndy_ish...@apple.com>
Date: Wed, 23 Apr 2025 17:21:06 -0700
Subject: [PATCH 2/3] Ask the diagnostic engine before emitting redundant
diagnostic
---
clang/lib/Serialization/ASTReader.cpp | 8 ++++++--
clang/test/Modules/invalid-module-dep.c | 3 +--
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/clang/lib/Serialization/ASTReader.cpp
b/clang/lib/Serialization/ASTReader.cpp
index 5761dc09a3e59..5d38ccbec5801 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -3287,6 +3287,7 @@ ASTReader::ReadControlBlock(ModuleFile &F,
ASTFileSignature StoredSignature;
std::string ImportedFile;
std::string StoredFile;
+ bool IgnoreImportedByNote = false;
// For prebuilt and explicit modules first consult the file map for
// an override. Note that here we don't search prebuilt module
@@ -3317,7 +3318,8 @@ ASTReader::ReadControlBlock(ModuleFile &F,
StoredFile = ReadPathBlob(BaseDirectoryAsWritten, Record, Idx, Blob);
if (ImportedFile.empty()) {
ImportedFile = StoredFile;
- } else {
+ } else if (!getDiags().isIgnored(diag::warn_lazy_pcm_mismatch,
+ CurrentImportLoc)) {
auto ImportedFileRef =
PP.getFileManager().getOptionalFileRef(ImportedFile);
auto StoredFileRef =
@@ -3327,6 +3329,7 @@ ASTReader::ReadControlBlock(ModuleFile &F,
Diag(diag::warn_lazy_pcm_mismatch) << ImportedFile << StoredFile;
Diag(diag::note_module_file_imported_by)
<< F.FileName << !F.ModuleName.empty() << F.ModuleName;
+ IgnoreImportedByNote = true;
}
}
}
@@ -3361,7 +3364,8 @@ ASTReader::ReadControlBlock(ModuleFile &F,
.getModuleCache()
.getInMemoryModuleCache()
.isPCMFinal(F.FileName);
- if (isDiagnosedResult(Result, Capabilities) || recompilingFinalized)
+ if (!IgnoreImportedByNote &&
+ (isDiagnosedResult(Result, Capabilities) || recompilingFinalized))
Diag(diag::note_module_file_imported_by)
<< F.FileName << !F.ModuleName.empty() << F.ModuleName;
diff --git a/clang/test/Modules/invalid-module-dep.c
b/clang/test/Modules/invalid-module-dep.c
index 6891a8cbac4fe..a3c7e110154cb 100644
--- a/clang/test/Modules/invalid-module-dep.c
+++ b/clang/test/Modules/invalid-module-dep.c
@@ -37,8 +37,7 @@
#include <A/A.h>
#include <B/B.h> // expected-warning {{conflicts with imported file}} \
// expected-note {{imported by module 'B'}} \
- // expected-error {{out of date and needs to be rebuilt}} \
- // expected-note {{imported by module 'B'}}
+ // expected-error {{out of date and needs to be rebuilt}}
//--- Sysroot/usr/include/A/module.modulemap
module A [system] {
>From 24714718c4aa411f5ad84bde132ba617cdf7090a Mon Sep 17 00:00:00 2001
From: Cyndy Ishida <cyndy_ish...@apple.com>
Date: Tue, 29 Apr 2025 08:21:28 -0700
Subject: [PATCH 3/3] Rename warning
---
clang/include/clang/Basic/DiagnosticSerializationKinds.td | 4 ++--
clang/lib/Serialization/ASTReader.cpp | 8 +++++---
clang/test/Modules/invalid-module-dep.c | 2 +-
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
index 3ff5490cf4168..7965da593f218 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
@@ -139,9 +139,9 @@ def warn_decls_in_multiple_modules : Warning<
InGroup<DiagGroup<"decls-in-multiple-modules">>,
DefaultIgnore;
-def warn_lazy_pcm_mismatch
+def warn_module_file_mapping_mismatch
: Warning<"loaded module file '%0' conflicts with imported file '%1'">,
- InGroup<DiagGroup<"lazy-pcm-mismatch">>,
+ InGroup<DiagGroup<"module-file-mapping-mismatch">>,
DefaultIgnore;
def err_failed_to_find_module_file : Error<
diff --git a/clang/lib/Serialization/ASTReader.cpp
b/clang/lib/Serialization/ASTReader.cpp
index 5d38ccbec5801..352558de79249 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -3318,15 +3318,17 @@ ASTReader::ReadControlBlock(ModuleFile &F,
StoredFile = ReadPathBlob(BaseDirectoryAsWritten, Record, Idx, Blob);
if (ImportedFile.empty()) {
ImportedFile = StoredFile;
- } else if (!getDiags().isIgnored(diag::warn_lazy_pcm_mismatch,
- CurrentImportLoc)) {
+ } else if (!getDiags().isIgnored(
+ diag::warn_module_file_mapping_mismatch,
+ CurrentImportLoc)) {
auto ImportedFileRef =
PP.getFileManager().getOptionalFileRef(ImportedFile);
auto StoredFileRef =
PP.getFileManager().getOptionalFileRef(StoredFile);
if ((ImportedFileRef && StoredFileRef) &&
(*ImportedFileRef != *StoredFileRef)) {
- Diag(diag::warn_lazy_pcm_mismatch) << ImportedFile << StoredFile;
+ Diag(diag::warn_module_file_mapping_mismatch)
+ << ImportedFile << StoredFile;
Diag(diag::note_module_file_imported_by)
<< F.FileName << !F.ModuleName.empty() << F.ModuleName;
IgnoreImportedByNote = true;
diff --git a/clang/test/Modules/invalid-module-dep.c
b/clang/test/Modules/invalid-module-dep.c
index a3c7e110154cb..2bcda8be5f1b6 100644
--- a/clang/test/Modules/invalid-module-dep.c
+++ b/clang/test/Modules/invalid-module-dep.c
@@ -31,7 +31,7 @@
// 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: -Wlazy-pcm-mismatch -verify %s
+// RUN: -Wmodule-file-mapping-mismatch -verify %s
#include <A/A.h>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
