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(
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(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
