[clang-tools-extra] b488935 - [clangd] Discard diagnostics from another SourceManager.

[Adam Czachorowski via cfe-commits]

Author: Adam Czachorowski
Date: 2020-08-21T13:11:21+02:00
New Revision: b4889353207aefd6f2641cef0301f78838c5b52e

URL: https://github.com/llvm/llvm-project/commit/b4889353207aefd6f2641cef0301f78838c5b52e
DIFF: https://github.com/llvm/llvm-project/commit/b4889353207aefd6f2641cef0301f78838c5b52e.diff

LOG: [clangd] Discard diagnostics from another SourceManager.

This can happen when building implicit modules, as demonstrated in test.
The CompilerInstance uses the same StoredDiags, but different
SourceManager. This used to crash clangd when it tried to relocate the
diagnostic to the main file, which, according to SourceManager from the
diagnostic, is a fake <module-includes> file.

Differential Revision: https://reviews.llvm.org/D85753

Added: 
    

Modified: 
    clang-tools-extra/clangd/Diagnostics.cpp
    clang-tools-extra/clangd/Diagnostics.h
    clang-tools-extra/clangd/unittests/ModulesTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index 674fa0dbf9ec..afa72f9d4051 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -518,13 +518,17 @@ std::vector<Diag> StoreDiags::take(const clang::tidy::ClangTidyContext *Tidy) {
 }
 
 void StoreDiags::BeginSourceFile(const LangOptions &Opts,
-                                 const Preprocessor *) {
+                                 const Preprocessor *PP) {
   LangOpts = Opts;
+  if (PP) {
+    OrigSrcMgr = &PP->getSourceManager();
+  }
 }
 
 void StoreDiags::EndSourceFile() {
   flushLastDiag();
   LangOpts = None;
+  OrigSrcMgr = nullptr;
 }
 
 /// Sanitizes a piece for presenting it in a synthesized fix message. Ensures
@@ -560,6 +564,16 @@ static void fillNonLocationData(DiagnosticsEngine::Level DiagLevel,
 
 void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                                   const clang::Diagnostic &Info) {
+  // If the diagnostic was generated for a 
diff erent SourceManager, skip it.
+  // This happens when a module is imported and needs to be implicitly built.
+  // The compilation of that module will use the same StoreDiags, but 
diff erent
+  // SourceManager.
+  if (OrigSrcMgr && Info.hasSourceManager() &&
+      OrigSrcMgr != &Info.getSourceManager()) {
+    IgnoreDiagnostics::log(DiagLevel, Info);
+    return;
+  }
+
   DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
   bool OriginallyError =
       Info.getDiags()->getDiagnosticIDs()->isDefaultMappingAsError(

diff  --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h
index 58f69287f256..4a00abfc450f 100644
--- a/clang-tools-extra/clangd/Diagnostics.h
+++ b/clang-tools-extra/clangd/Diagnostics.h
@@ -122,7 +122,8 @@ class StoreDiags : public DiagnosticConsumer {
   // The ClangTidyContext populates Source and Name for clang-tidy diagnostics.
   std::vector<Diag> take(const clang::tidy::ClangTidyContext *Tidy = nullptr);
 
-  void BeginSourceFile(const LangOptions &Opts, const Preprocessor *) override;
+  void BeginSourceFile(const LangOptions &Opts,
+                       const Preprocessor *PP) override;
   void EndSourceFile() override;
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const clang::Diagnostic &Info) override;
@@ -148,6 +149,7 @@ class StoreDiags : public DiagnosticConsumer {
   llvm::Optional<Diag> LastDiag;
   llvm::Optional<FullSourceLoc> LastDiagLoc; // Valid only when LastDiag is set.
   bool LastDiagOriginallyError = false;      // Valid only when LastDiag is set.
+  SourceManager *OrigSrcMgr = nullptr;
 
   llvm::DenseSet<std::pair<unsigned, unsigned>> IncludedErrorLocations;
   bool LastPrimaryDiagnosticWasSuppressed = false;

diff  --git a/clang-tools-extra/clangd/unittests/ModulesTests.cpp b/clang-tools-extra/clangd/unittests/ModulesTests.cpp
index a10b9e897a48..83d6b28d6dfc 100644
--- a/clang-tools-extra/clangd/unittests/ModulesTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ModulesTests.cpp
@@ -64,6 +64,34 @@ TEST(Modules, PreambleBuildVisibility) {
   EXPECT_TRUE(TU.build().getDiagnostics().empty());
 }
 
+TEST(Modules, Diagnostic) {
+  // Produce a diagnostic while building an implicit module. Use
+  // -fmodules-strict-decluse, but any non-silenced diagnostic will do.
+  TestTU TU = TestTU::withCode(R"cpp(
+    /*error-ok*/
+    #include "modular.h"
+
+    void bar() {}
+)cpp");
+  TU.OverlayRealFileSystemForModules = true;
+  TU.ExtraArgs.push_back("-fmodule-map-file=" + testPath("m.modulemap"));
+  TU.ExtraArgs.push_back("-fmodules");
+  TU.ExtraArgs.push_back("-fimplicit-modules");
+  TU.ExtraArgs.push_back("-fmodules-strict-decluse");
+  TU.AdditionalFiles["modular.h"] = R"cpp(
+    #include "non-modular.h"
+  )cpp";
+  TU.AdditionalFiles["non-modular.h"] = "";
+  TU.AdditionalFiles["m.modulemap"] = R"modulemap(
+    module M {
+      header "modular.h"
+    }
+)modulemap";
+
+  // Test that we do not crash.
+  TU.build();
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang