[PATCH] D151321: [clangd] Dont run raw-lexer for OOB source locations

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: hokein.
Herald added a subscriber: arphaman.
Herald added a project: All.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

We can get stale source locations from preamble, make sure we don't
access those locations without checking first.

Fixes https://github.com/clangd/clangd/issues/1636.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151321

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


Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -829,6 +829,37 @@
     auto AST = createPatchedAST(Code.code(), NewCode.code());
     EXPECT_THAT(*AST->getDiagnostics(), IsEmpty());
   }
+  {
+    Annotations Code(R"(
+#ifndef FOO
+#define FOO
+void foo();
+#endif)");
+    // This code will emit a diagnostic for unterminated #ifndef (as stale
+    // preamble has the conditional but main file doesn't terminate it).
+    // We shouldn't emit any diagnotiscs (and shouldn't crash).
+    Annotations NewCode("");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    EXPECT_THAT(*AST->getDiagnostics(), IsEmpty());
+  }
+  {
+    Annotations Code(R"(
+#ifndef FOO
+#define FOO
+void foo();
+#endif)");
+    // This code will emit a diagnostic for unterminated #ifndef (as stale
+    // preamble has the conditional but main file doesn't terminate it).
+    // We shouldn't emit any diagnotiscs (and shouldn't crash).
+    // FIXME: Patch/ignore diagnostics in such cases.
+    Annotations NewCode(R"(
+i[[nt]] xyz;
+    )");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    EXPECT_THAT(
+        *AST->getDiagnostics(),
+        ElementsAre(Diag(NewCode.range(), "pp_unterminated_conditional")));
+  }
 }
 
 MATCHER_P2(Mark, Range, Text, "") {
Index: clang-tools-extra/clangd/Diagnostics.cpp
===================================================================
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -96,7 +96,8 @@
 
 // Clang diags have a location (shown as ^) and 0 or more ranges (~~~~).
 // LSP needs a single range.
-Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) {
+std::optional<Range> diagnosticRange(const clang::Diagnostic &D,
+                                     const LangOptions &L) {
   auto &M = D.getSourceManager();
   auto PatchedRange = [&M](CharSourceRange &R) {
     R.setBegin(translatePreamblePatchLocation(R.getBegin(), M));
@@ -115,13 +116,18 @@
     if (locationInRange(Loc, R, M))
       return halfOpenToRange(M, PatchedRange(R));
   }
+  // Source locations from stale preambles might become OOB.
+  // FIXME: These diagnostics might point to wrong locations even when they're
+  // not OOB.
+  auto [FID, Offset] = M.getDecomposedLoc(Loc);
+  if (Offset > M.getBufferData(FID).size())
+    return std::nullopt;
   // If the token at the location is not a comment, we use the token.
   // If we can't get the token at the location, fall back to using the location
   auto R = CharSourceRange::getCharRange(Loc);
   Token Tok;
-  if (!Lexer::getRawToken(Loc, Tok, M, L, true) && Tok.isNot(tok::comment)) {
+  if (!Lexer::getRawToken(Loc, Tok, M, L, true) && Tok.isNot(tok::comment))
     R = CharSourceRange::getTokenRange(Tok.getLocation(), Tok.getEndLoc());
-  }
   return halfOpenToRange(M, PatchedRange(R));
 }
 
@@ -697,7 +703,10 @@
     SourceLocation PatchLoc =
         translatePreamblePatchLocation(Info.getLocation(), SM);
     D.InsideMainFile = isInsideMainFile(PatchLoc, SM);
-    D.Range = diagnosticRange(Info, *LangOpts);
+    if (auto DRange = diagnosticRange(Info, *LangOpts))
+      D.Range = *DRange;
+    else
+      D.Severity = DiagnosticsEngine::Ignored;
     auto FID = SM.getFileID(Info.getLocation());
     if (const auto FE = SM.getFileEntryRefForID(FID)) {
       D.File = FE->getName().str();


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D151321.525119.patch
Type: text/x-patch
Size: 3562 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230524/df20d804/attachment-0001.bin>