[Modules][Diagnostic] Improve diagnostics for stale module dependencies #167713
+66
−6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Devajith (devajithvs) ChangesWhen a module becomes out of date because its dependency has changed, Clang previously reported that the dependency itself was This patch introduces a new diagnostic that clearly indicates which module is out of date and which dependency has changed, making it easier for users to understand what needs to be rebuilt. Before: Full diff: https://github.com/llvm/llvm-project/pull/167713.diff 3 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
index b80aff385e01f..d79fbcd9b8a84 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
@@ -71,6 +71,9 @@ def err_ast_file_not_found : Error<
def err_ast_file_out_of_date : Error<
"%select{PCH|module|precompiled}0 file '%1' is out of date and "
"needs to be rebuilt%select{|: %3}2">, DefaultFatal;
+def err_ast_file_dependency_out_of_date : Error<
+ "%select{PCH|module|AST}0 file '%1' is out of date because "
+ "dependency '%2' has changed%select{|: %4}3">, DefaultFatal;
def err_ast_file_invalid : Error<
"file '%1' is not a valid %select{PCH|module|precompiled}0 file: %2">, DefaultFatal;
def note_module_file_imported_by : Note<
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 634bf991b2aee..d302fa0089f78 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -5134,10 +5134,17 @@ ASTReader::ReadASTCore(StringRef FileName,
if (ClientLoadCapabilities & ARR_OutOfDate)
return OutOfDate;
- // Otherwise, return an error.
- Diag(diag::err_ast_file_out_of_date)
- << moduleKindForDiagnostic(Type) << FileName << !ErrorStr.empty()
- << ErrorStr;
+ // Otherwise, report an error.
+ if (ImportedBy) {
+ // Report that the importer is out of date because the dependency changed.
+ Diag(diag::err_ast_file_dependency_out_of_date)
+ << moduleKindForDiagnostic(ImportedBy->Kind) << ImportedBy->FileName
+ << FileName << !ErrorStr.empty() << StringRef(ErrorStr);
+ } else {
+ Diag(diag::err_ast_file_out_of_date)
+ << moduleKindForDiagnostic(Type) << FileName << !ErrorStr.empty()
+ << ErrorStr;
+ }
return Failure;
}
diff --git a/clang/test/Modules/explicit-build.cpp b/clang/test/Modules/explicit-build.cpp
index c2fe2024a3629..3e86c8138362f 100644
--- a/clang/test/Modules/explicit-build.cpp
+++ b/clang/test/Modules/explicit-build.cpp
@@ -184,6 +184,46 @@
// CHECK-NO-FILE-INDIRECT-NEXT: note: imported by module 'c' in '{{.*}}c.pcm'
// CHECK-NO-FILE-INDIRECT-NOT: note:
+// -------------------------------
+// Check that we diagnose stale dependencies correctly when modules change.
+//
+// Trigger a rebuild of A with a different configuration (-DA_EXTRA_DEFINE) to make B and C out of date
+// RUN: mv %t/a.pcm %t/a-tmp.pcm
+// RUN: %clang_cc1 -x c++ -std=c++11 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -Rmodule-build -fno-modules-error-recovery \
+// RUN: -fmodule-name=a -emit-module %S/Inputs/explicit-build/module.modulemap -o %t/a.pcm \
+// RUN: -DA_EXTRA_DEFINE \
+// RUN: 2>&1 | FileCheck --check-prefix=CHECK-NO-IMPLICIT-BUILD %s --allow-empty
+
+// Try to use C, which depends on B, which depends on the now-changed A.
+// RUN: not %clang_cc1 -x c++ -std=c++11 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -Rmodule-build -fno-modules-error-recovery \
+// RUN: -I%S/Inputs/explicit-build \
+// RUN: -fmodule-file=%t/c.pcm \
+// RUN: %s -DHAVE_A -DHAVE_B -DHAVE_C 2>&1 | FileCheck --check-prefix=CHECK-OUT-OF-DATE-INDIRECT %s
+//
+// CHECK-OUT-OF-DATE-INDIRECT: fatal error: module file '{{.*}}b.pcm' is out of date because dependency '{{.*}}a.pcm' has changed
+// CHECK-OUT-OF-DATE-INDIRECT-NEXT: note: imported by module 'b' in '{{.*}}b.pcm'
+// CHECK-OUT-OF-DATE-INDIRECT-NEXT: note: imported by module 'c' in '{{.*}}c.pcm'
+
+// Rebuild B with the new A, leaving C out of date.
+// RUN: mv %t/b.pcm %t/b-tmp.pcm
+// RUN: %clang_cc1 -x c++ -std=c++11 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -Rmodule-build -fno-modules-error-recovery \
+// RUN: -fmodule-file=%t/a.pcm \
+// RUN: -fmodule-name=b -emit-module %S/Inputs/explicit-build/module.modulemap -o %t/b.pcm \
+// RUN: -DA_EXTRA_DEFINE \
+// RUN: 2>&1 | FileCheck --check-prefix=CHECK-NO-IMPLICIT-BUILD %s --allow-empty
+//
+// Now only C is out of date. Try to use C.
+// RUN: not %clang_cc1 -x c++ -std=c++11 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -Rmodule-build -fno-modules-error-recovery \
+// RUN: -I%S/Inputs/explicit-build \
+// RUN: -fmodule-file=%t/c.pcm \
+// RUN: %s -DHAVE_A -DHAVE_B -DHAVE_C 2>&1 | FileCheck --check-prefix=CHECK-OUT-OF-DATE-DIRECT %s
+//
+// CHECK-OUT-OF-DATE-DIRECT: fatal error: module file '{{.*}}c.pcm' is out of date because dependency '{{.*}}b.pcm' has changed
+// CHECK-OUT-OF-DATE-DIRECT-NOT: fatal error: module file '{{.*}}b.pcm' is out of date
+//
+// RUN: mv %t/a-tmp.pcm %t/a.pcm
+// RUN: mv %t/b-tmp.pcm %t/b.pcm
+
// -------------------------------
// Check that we don't get upset if B's timestamp is newer than C's.
// RUN: touch %t/b.pcm
@@ -202,6 +242,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 than expected
+// CHECK-MISMATCHED-B: fatal error: module file '{{.*}}c.pcm' is out of date because dependency '{{.*}}b.pcm' has changed: module file has a different size than expected
// CHECK-MISMATCHED-B-NEXT: note: imported by module 'c'
// CHECK-MISMATCHED-B-NOT: note:
|
devajithvs
force-pushed
the
module_file_dependency
branch
from
77eff31 to
0e417e3
Compare
Contributor
Author
|
Also see: root-project/root#7704 |
ChuanqiXu9
reviewed
Nov 13, 2025
| Diag(diag::err_ast_file_out_of_date) | ||
| << moduleKindForDiagnostic(Type) << FileName << !ErrorStr.empty() | ||
| << ErrorStr; | ||
| if (ImportedBy) { |
Member
There was a problem hiding this comment.
Why can we assume it is not out of date if ImportedBy is present?
Contributor
Author
There was a problem hiding this comment.
I was looking into this again:
My initial reasoning was using this example:
B -> A (B imports A)
When something uses B:
Bis loaded as a root module (ImportedBy == nullptrforB).Bthen loads its dependencyA(ImportedBy == B), using the size and mtime that were that was recorded previously.
StoredSize(A) = <old size of A>
If A.pcm has been rebuilt or changed since B.pcm was produced, the on-disk size/signature of A no longer matches what B recorded. lookupModuleFile() (or the signature check) detects this and returns OutOfDate. In this case when ImportedBy is non-null, the importer is the stale file, not the dependency.
The only other case (I missed this) where addModule returns OutOfDate is when we tried (and failed) to import it earlier (shouldBuildPCM() returns false). In this case, A is out of date. I'm not able to think of a way to trigger that codepath though.
Maybe, the proper fix is to add a new enum to say importer is out of date because in most cases the Importer is OutOfDate than the current module itself?
--- a/clang/include/clang/Serialization/ModuleManager.h
+++ b/clang/include/clang/Serialization/ModuleManager.h
@@ -200,6 +200,9 @@ public:
/// The module file is out-of-date.
OutOfDate
+
+ /// The importer file is out-of-date
+ ImporterOutOfDate
};
Member
There was a problem hiding this comment.
If A.pcm has been rebuilt or changed since B.pcm was produced, the on-disk size/signature of A no longer matches what B recorded.
This is not the case for named module, we didn't record the signature/size in named module.
devajithvs
force-pushed
the
module_file_dependency
branch
from
0e417e3 to
59895ba
Compare
When a module becomes out of date because its dependency has changed,
Clang previously reported that the dependency itself was `out of date and
needs to be rebuilt`, even when the dependency had just been rebuilt. This
was confusing because the real issue is that the importing module is stale
due to the dependency change.
This patch introduces a new diagnostic that clearly indicates which module
is out of date and which dependency has changed, making it easier for users
to understand what needs to be rebuilt.
Before:
`module file 'A.pcm' is out of date and needs to be rebuilt`
(even though A.pcm was just rebuilt)
After:
`module file 'B.pcm' is out of date because dependency 'A.pcm' has changed`
devajithvs
force-pushed
the
module_file_dependency
branch
from
59895ba to
be78df6
Compare
Contributor
Author
|
@ChuanqiXu9 Does this look good now? |
ChuanqiXu9
reviewed
Nov 19, 2025
| OutOfDate, | ||
|
|
||
| /// The importer file is out-of-date | ||
| ImporterOutOfDate |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a module becomes out of date because its dependency has changed, Clang previously reported that the dependency itself was
out of date and needs to be rebuilt, even when the dependency had just been rebuilt. This was confusing because the real issue is that the importing module is stale due to the dependency change.This patch introduces a new diagnostic that clearly indicates which module is out of date and which dependency has changed, making it easier for users to understand what needs to be rebuilt.
Before:
module file 'A.pcm' is out of date and needs to be rebuilt(even though A.pcm was just rebuilt)
After:
module file 'B.pcm' is out of date because dependency 'A.pcm' has changed