Make diagnostic reporting more robust in presence of corrupt PCH data.

[neilmacintosh]

Summary

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!

[dexonsmith]

Thanks for working on this! I have a couple of comments inline.

clang/include/clang/Basic/Diagnostic.h

918–927

CurDelayedDiagID has a different kind of "in-flight" lifetime than CurDiagID. I think the comment here should be more explicit about what precisely the variable is doing, and perhaps it should have another name. Do we actually need the ID kept as state? If not, this would be simpler as:

bool IsReportingDelayedDiag = false;

Also, a couple of nit-picks:

• Periods at the end of sentences in comments.
• You seem to have dropped out of doxygen comments (// instead of ///).

clang/lib/Basic/Diagnostic.cpp

160–161

If you take my suggestion, then you wouldn't need to change this line, just add:

IsReportingDelayedDiag = true;

but would these assertions be valid after your change?

assert(DelayedDiagID && "Called without a delayed diagnostic?");
assert(!IsReportingDelayedDiag &&
       "Called while reporting another delayed diagnostic?");

[bruno]

Thanks for working on this.

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!

Yea, those are some times hard. Did you try to write a unittest that hits the assertion and trigger this case? Perhaps clang/unittests/Frontend/PCHPreambleTest.cpp would be a good example to start off.

clang/include/clang/Basic/Diagnostic.h

public:
   ///
   /// \param Arg3 A string argument that will be provided to the
   /// diagnostic. A copy of this string will be stored in the
   /// DiagnosticsEngine object itself.
   void SetDelayedDiagnostic(unsigned DiagID, StringRef Arg1 = "",
                             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; }
 
 private:
   // This is private state used by DiagnosticBuilder.  We put it here instead of
   // in DiagnosticBuilder in order to keep DiagnosticBuilder a small lightweight
   // object.  This implementation choice means that we can only have one
   // diagnostic "in flight" at a time, but this seems to be a reasonable
   // tradeoff to keep these objects small.  Assertions verify that only one
   // diagnostic is in flight at a time.
   friend class Diagnostic;
   friend class DiagnosticBuilder;
   friend class DiagnosticErrorTrap;
   friend class DiagnosticIDs;
   friend class PartialDiagnostic;
 
   /// Report the delayed diagnostic.
   void ReportDelayed();
 
   /// The location of the current diagnostic that is in flight.
   SourceLocation CurDiagLoc;
 
   /// The ID of the current diagnostic that is in flight.
   ///
   /// This is set to std::numeric_limits<unsigned>::max() when there is no
   /// 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.
     ///
     /// We currently only support up to 10 arguments (%0-%9).  A single
     /// diagnostic with more than that almost certainly has to be simplified
     /// anyway.
     MaxArguments = 10,
   };

clang/lib/Basic/Diagnostic.cpp

void DiagnosticsEngine::Reset() {
   NumWarnings = 0;
   NumErrors = 0;
   TrapNumErrorsOccurred = 0;
   TrapNumUnrecoverableErrorsOccurred = 0;
 
   CurDiagID = std::numeric_limits<unsigned>::max();
   LastDiagLevel = DiagnosticIDs::Ignored;
   DelayedDiagID = 0;
+  CurDelayedDiagID = 0;
 
   // Clear state related to #pragma diagnostic.
   DiagStates.clear();
   DiagStatesByLoc.clear();
   DiagStateOnPushStack.clear();
 
   // Create a DiagState and DiagStatePoint representing diagnostic changes
   // through command-line.
   DiagStates.emplace_back();
   DiagStatesByLoc.appendFirst(&DiagStates.back());
 }
 
 void DiagnosticsEngine::SetDelayedDiagnostic(unsigned DiagID, StringRef Arg1,
                                              StringRef Arg2, StringRef Arg3) {
-  if (DelayedDiagID)
+  if (CurDelayedDiagID || DelayedDiagID)
     return;
 
   DelayedDiagID = DiagID;
   DelayedDiagArg1 = Arg1.str();
   DelayedDiagArg2 = Arg2.str();
   DelayedDiagArg3 = Arg3.str();
 }
 
 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) {
   assert(Files.empty() && "not first");
   FirstDiagState = CurDiagState = State;
   CurDiagStateLoc = SourceLocation();
 }
 
if (Force) {
     // DiagnosticConsumer.
     Emitted = ProcessDiag();
   }
 
   // Clear out the current diagnostic object.
   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;
 }
 
 DiagnosticConsumer::~DiagnosticConsumer() = default;
 
 void DiagnosticConsumer::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,