[PATCH] D74795: Make diagnostic reporting more robust in presence of corrupt PCH data.

[Neil MacIntosh via Phabricator via cfe-commits]

neilmacintosh created this revision.
neilmacintosh added reviewers: modocache, doug.gregor, hokein.
Herald added subscribers: cfe-commits, usaxena95, kadircet, dexonsmith, ilya-biryukov.
Herald added a project: clang.

When running clangd, I found that occasionally, accessing a
precompiled header would cause an LLVM assertion to fire.

This in turn would cause a diagnostic to try and be emitted,
when the diagnostics machinery would reach back to find 
information from the PCH, it would hit the same problem
that caused the initial assertion. Another assertion would fire and
another delayed diagnostic would be setup to report, and so on.

This recursive sequence would continue until stack overflow, which
would take down clangd.

This small patch works around that possibility by making a delayed
diagnostic more explicit (and therefore testable to avoid recursion).

I tested this against the files in our codebase that hit the
LLVM assertion with clangd and now the assertion clearly reports
and clangd is no longer exhausted by a stack overflow.

Tracking down the underlying problem (hitting the assertion)
is a separate task.

I'm not sure how (or if it's possible) to add a test that would
demonstrate the before/after here. Happy to take advice!


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74795

Files:
  clang/include/clang/Basic/Diagnostic.h
  clang/lib/Basic/Diagnostic.cpp


Index: clang/lib/Basic/Diagnostic.cpp
===================================================================
--- clang/lib/Basic/Diagnostic.cpp
+++ clang/lib/Basic/Diagnostic.cpp
@@ -132,6 +132,7 @@
   CurDiagID = std::numeric_limits<unsigned>::max();
   LastDiagLevel = DiagnosticIDs::Ignored;
   DelayedDiagID = 0;
+  CurDelayedDiagID = 0;
 
   // Clear state related to #pragma diagnostic.
   DiagStates.clear();
@@ -146,7 +147,7 @@
 
 void DiagnosticsEngine::SetDelayedDiagnostic(unsigned DiagID, StringRef Arg1,
                                              StringRef Arg2, StringRef Arg3) {
-  if (DelayedDiagID)
+  if (CurDelayedDiagID || DelayedDiagID)
     return;
 
   DelayedDiagID = DiagID;
@@ -156,9 +157,11 @@
 }
 
 void DiagnosticsEngine::ReportDelayed() {
-  unsigned ID = DelayedDiagID;
+  CurDelayedDiagID = DelayedDiagID;
   DelayedDiagID = 0;
-  Report(ID) << DelayedDiagArg1 << DelayedDiagArg2 << DelayedDiagArg3;
+  Report(CurDelayedDiagID) << DelayedDiagArg1 << DelayedDiagArg2
+                           << DelayedDiagArg3;
+  CurDelayedDiagID = 0;
 }
 
 void DiagnosticsEngine::DiagStateMap::appendFirst(DiagState *State) {
@@ -521,7 +524,9 @@
   Clear();
 
   // If there was a delayed diagnostic, emit it now.
-  if (!Force && DelayedDiagID)
+  // unless we are currently emitting a delayed diagnostic,
+  // as that leads to the potential of endless reporting.
+  if (!Force && DelayedDiagID && !CurDelayedDiagID)
     ReportDelayed();
 
   return Emitted;
Index: clang/include/clang/Basic/Diagnostic.h
===================================================================
--- clang/include/clang/Basic/Diagnostic.h
+++ clang/include/clang/Basic/Diagnostic.h
@@ -888,7 +888,10 @@
                             StringRef Arg2 = "", StringRef Arg3 = "");
 
   /// Clear out the current diagnostic.
-  void Clear() { CurDiagID = std::numeric_limits<unsigned>::max(); }
+  void Clear() {
+    CurDiagID = std::numeric_limits<unsigned>::max();
+    CurDelayedDiagID = 0;
+  }
 
   /// Return the value associated with this diagnostic flag.
   StringRef getFlagValue() const { return FlagValue; }
@@ -918,6 +921,11 @@
   /// diagnostic in flight.
   unsigned CurDiagID;
 
+  /// The ID of the current delayed diagnostic being reported.
+  //
+  // This is set to 0 when there is no delayed diagnostic in flight
+  unsigned CurDelayedDiagID;
+
   enum {
     /// The maximum number of arguments we can hold.
     ///


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D74795.245275.patch
Type: text/x-patch
Size: 2433 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200218/5c9021ea/attachment-0001.bin>