llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-modules 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
