https://github.com/jansvoboda11 created
https://github.com/llvm/llvm-project/pull/134475
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.)
>From 89c7c19b7faa6ed2b174df41841991667a01a57c Mon Sep 17 00:00:00 2001
From: Jan Svoboda <jan_svob...@apple.com>
Date: Fri, 4 Apr 2025 19:33:18 -0700
Subject: [PATCH] [clang][modules] Name the module map files on PCM file
conflict
---
.../Basic/DiagnosticSerializationKinds.td | 3 +-
clang/lib/Serialization/ASTReader.cpp | 66 ++++++++++---------
.../modules-relocated-mm-macro.c | 9 +--
3 files changed, 40 insertions(+), 38 deletions(-)
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" }
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
