[PATCH] D85753: [clangd] Discard diagnostics from another SourceManager.

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz created this revision.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous.
Herald added a project: clang.
adamcz requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85753

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


Index: clang-tools-extra/clangd/unittests/ModulesTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ModulesTests.cpp
+++ clang-tools-extra/clangd/unittests/ModulesTests.cpp
@@ -39,6 +39,34 @@
   TU.index();
 }
 
+TEST(Modules, Diagnostic) {
+  // Produce a diagnostic while building an implicit module. Use
+  // -fmodules-strick-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
Index: clang-tools-extra/clangd/Diagnostics.h
===================================================================
--- clang-tools-extra/clangd/Diagnostics.h
+++ clang-tools-extra/clangd/Diagnostics.h
@@ -122,7 +122,8 @@
   // 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 @@
   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;
Index: clang-tools-extra/clangd/Diagnostics.cpp
===================================================================
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -518,13 +518,17 @@
 }
 
 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,12 @@
 
 void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                                   const clang::Diagnostic &Info) {
+  // If the diagnostic was generated for a different SourceManager, skip it.
+  // This can happen when using implicit modules.
+  if (OrigSrcMgr && Info.hasSourceManager() &&
+      OrigSrcMgr != &Info.getSourceManager())
+    return;
+
   DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
   bool OriginallyError =
       Info.getDiags()->getDiagnosticIDs()->isDefaultMappingAsError(


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D85753.284802.patch
Type: text/x-patch
Size: 3536 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200811/4cd9fd63/attachment.bin>