[clang][modules] Name the module map files on PCM file conflict (#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.)

Author: jansvoboda11

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'">,

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()); });
 }
 
 //===----------------------------------------------------------------------===//
@@ -3349,8 +3345,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;
@@ -4440,6 +4434,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)
@@ -6105,14 +6100,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>();
           }
         }
 

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" }