[clang] [clang][modules] Name the module map files on PCM file conflict (PR #134475)
via cfe-commits
cfe-commits at lists.llvm.org
Fri Apr 4 19:41:46 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Jan Svoboda (jansvoboda11)
<details>
<summary>Changes</summary>
With implicitly-built modules, seeing something like:
```
fatal error: module 'X' is defined in both '<cache>/HASH1/X-HASH2.pcm' and '<cache>/HASH1/X-HASH3.pcm'
```
is super confusing and not actionable, because the module cache tends to be hidden from the developer.
This PR adds a note to that diagnostic that names the module map files the PCM files were compiled from, hopefully giving a good enough hint for further investigation:
```
note: compiled from '<build>/X.framework/Modules/module.modulemap' and '<SDK>/X.framework/Modules/module.modulemap'
```
(I had to replace the mechanism used to convert `DiagnosticError` into something `DiagnosticsEngine` can understand, because it seemingly did not support notes.)
---
Full diff: https://github.com/llvm/llvm-project/pull/134475.diff
3 Files Affected:
- (modified) clang/include/clang/Basic/DiagnosticSerializationKinds.td (+1-2)
- (modified) clang/lib/Serialization/ASTReader.cpp (+34-32)
- (modified) clang/test/ClangScanDeps/modules-relocated-mm-macro.c (+5-4)
``````````diff
diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
index 3914d3930bec7..5fc5937b80d35 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
@@ -75,8 +75,7 @@ def err_module_file_not_module : Error<
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<
- "this is generally caused by modules with the same name found in multiple "
- "paths">;
+ "compiled from '%0' and '%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 d8d77e7f55232..6ee51122ea180 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -44,7 +44,6 @@
#include "clang/Basic/ASTSourceDescriptor.h"
#include "clang/Basic/CommentOptions.h"
#include "clang/Basic/Diagnostic.h"
-#include "clang/Basic/DiagnosticError.h"
#include "clang/Basic/DiagnosticIDs.h"
#include "clang/Basic/DiagnosticOptions.h"
#include "clang/Basic/DiagnosticSema.h"
@@ -1552,30 +1551,27 @@ void ASTReader::Error(unsigned DiagID, StringRef Arg1, StringRef Arg2,
Diag(DiagID) << Arg1 << Arg2 << Arg3;
}
+namespace {
+struct AlreadyReportedDiagnosticError
+ : llvm::ErrorInfo<AlreadyReportedDiagnosticError> {
+ static char ID;
+
+ void log(raw_ostream &OS) const override {
+ llvm_unreachable("reporting an already-reported diagnostic error");
+ }
+
+ std::error_code convertToErrorCode() const override {
+ return llvm::inconvertibleErrorCode();
+ }
+};
+
+char AlreadyReportedDiagnosticError::ID = 0;
+} // namespace
+
void ASTReader::Error(llvm::Error &&Err) const {
- llvm::Error RemainingErr =
- handleErrors(std::move(Err), [this](const DiagnosticError &E) {
- auto Diag = E.getDiagnostic().second;
-
- // Ideally we'd just emit it, but have to handle a possible in-flight
- // diagnostic. Note that the location is currently ignored as well.
- auto NumArgs = Diag.getStorage()->NumDiagArgs;
- assert(NumArgs <= 3 && "Can only have up to 3 arguments");
- StringRef Arg1, Arg2, Arg3;
- switch (NumArgs) {
- case 3:
- Arg3 = Diag.getStringArg(2);
- [[fallthrough]];
- case 2:
- Arg2 = Diag.getStringArg(1);
- [[fallthrough]];
- case 1:
- Arg1 = Diag.getStringArg(0);
- }
- Error(Diag.getDiagID(), Arg1, Arg2, Arg3);
- });
- if (RemainingErr)
- Error(toString(std::move(RemainingErr)));
+ handleAllErrors(
+ std::move(Err), [](AlreadyReportedDiagnosticError &) {},
+ [&](llvm::ErrorInfoBase &E) { return Error(E.message()); });
}
//===----------------------------------------------------------------------===//
@@ -3328,8 +3324,6 @@ ASTReader::ReadControlBlock(ModuleFile &F,
if (isDiagnosedResult(Result, Capabilities) || recompilingFinalized)
Diag(diag::note_module_file_imported_by)
<< F.FileName << !F.ModuleName.empty() << F.ModuleName;
- if (recompilingFinalized)
- Diag(diag::note_module_file_conflict);
switch (Result) {
case Failure: return Failure;
@@ -4419,6 +4413,7 @@ ASTReader::ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F,
// This module was defined by an imported (explicit) module.
Diag(diag::err_module_file_conflict) << F.ModuleName << F.FileName
<< ASTFE->getName();
+ // TODO: Add a note with the module map paths if they differ.
} else {
// This module was built with a different module map.
Diag(diag::err_imported_module_not_found)
@@ -6067,14 +6062,21 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
if (OptionalFileEntryRef CurFile = CurrentModule->getASTFile()) {
// Don't emit module relocation error if we have -fno-validate-pch
if (!bool(PP.getPreprocessorOpts().DisablePCHOrModuleValidation &
- DisableValidationForModuleKind::Module) &&
- CurFile != F.File) {
- auto ConflictError =
- PartialDiagnostic(diag::err_module_file_conflict,
- ContextObj->DiagAllocator)
+ DisableValidationForModuleKind::Module)) {
+ assert(CurFile != F.File && "ModuleManager did not de-duplicate");
+
+ Diag(diag::err_module_file_conflict)
<< CurrentModule->getTopLevelModuleName() << CurFile->getName()
<< F.File.getName();
- return DiagnosticError::create(CurrentImportLoc, ConflictError);
+
+ auto CurModMapFile =
+ ModMap.getContainingModuleMapFile(CurrentModule);
+ auto ModMapFile = FileMgr.getOptionalFileRef(F.ModuleMapPath);
+ if (CurModMapFile && ModMapFile && CurModMapFile != ModMapFile)
+ Diag(diag::note_module_file_conflict)
+ << CurModMapFile->getName() << ModMapFile->getName();
+
+ return llvm::make_error<AlreadyReportedDiagnosticError>();
}
}
diff --git a/clang/test/ClangScanDeps/modules-relocated-mm-macro.c b/clang/test/ClangScanDeps/modules-relocated-mm-macro.c
index 17f479d9e0046..87fbad0c72131 100644
--- a/clang/test/ClangScanDeps/modules-relocated-mm-macro.c
+++ b/clang/test/ClangScanDeps/modules-relocated-mm-macro.c
@@ -13,13 +13,14 @@
// RUN: cp -r %t/frameworks2/A.framework %t/frameworks1
-// RUN: not clang-scan-deps -format experimental-full -o %t/deps2.json -- \
+// RUN: not clang-scan-deps -format experimental-full -o %t/deps2.json 2>%t/errs -- \
// RUN: %clang -fmodules -fmodules-cache-path=%t/cache \
// RUN: -F %t/frameworks1 -F %t/frameworks2 \
-// RUN: -c %t/tu2.m -o %t/tu2.o \
-// RUN: 2>&1 | FileCheck %s
+// RUN: -c %t/tu2.m -o %t/tu2.o
+// RUN: FileCheck --input-file=%t/errs %s
-// CHECK: fatal error: module 'A' is defined in both '{{.*}}.pcm' and '{{.*}}.pcm'
+// CHECK: fatal error: module 'A' is defined in both '{{.*}}.pcm' and '{{.*}}.pcm'
+// CHECK-NEXT: note: compiled from '{{.*}}frameworks1{{.*}}' and '{{.*}}frameworks2{{.*}}'
//--- frameworks2/A.framework/Modules/module.modulemap
framework module A { header "A.h" }
``````````
</details>
https://github.com/llvm/llvm-project/pull/134475
More information about the cfe-commits
mailing list