Implement soft reset of the diagnostics engine.

[tapaswenipathak]

Summary

Implement soft reset of the diagnostics engine.

This patch implements soft reset and adds tests for soft reset success of the
diagnostics engine.

This patch adds unittest for soft reset success using #define public
private. Multiple options were used before arriving on using #define public
private as discussed in https://github.com/vgvassilev/clang/pull/1.

1. Enable getter for private variables distinguishing soft reset.
2. Using any existing locs
3. gtest:friend_test https://google.github.io/googletest/reference/testing.html#FRIEND_TEST
4. using #define public private accomapnied with #undef private before
and after the inclusion of `.h` file to access a few private members of
enclosing class in the test function.

Finally, a test helper friend function is added to test soft reset
success.

This patch also adds a clang-repl test and adapts clang:tools:clang-repl to
report unsuccess when running with -verify.

Screenshots of testing this patch can be found on the mentioned pull
request.

Co-authored-by: Vassil Vassilev <[e-mail]>

[rsmith]

I don't think we can live with the #define private public approach, not least because this violates the ODR and might lead to compile failures using modules as a result. As an alternative, how about:

• Adding a friend void DiagnosticTestHelper(); declaration to DiagnosticsEngine
• Defining that function in DiagnosticTest.cpp and calling it to do your checks of the diagnostics engine's state.

[tapaswenipathak]

Addresses review comment by @rsmith: https://reviews.llvm.org/D126183#3534846.

[tapaswenipathak]

runs clang-format on the diff.

[tapaswenipathak]

Fixes: https://buildkite.com/llvm-project/diff-checks/builds/106655#01810a71-0b5c-4b92-9d05-22a1ed950833

.

[tapaswenipathak]

Fixes: https://buildkite.com/llvm-project/premerge-checks/builds/95001#01810b0b-6313-400f-aaf0-35855916ec93

I pasted the wrong diff. sorry!

(have multiple build repository in local)

[tapaswenipathak]

This comment was removed by tapaswenipathak.

[tapaswenipathak]

this should be green. :)

[v.g.vassilev]

This looks good to me, modulo the inline comment.

Let's wait for @rsmith's green light.

clang/lib/Interpreter/IncrementalParser.cpp

201

Diags.Reset(/*soft=*/true);

[v.g.vassilev]

clang/lib/Interpreter/IncrementalParser.cpp

200

We should remove this fixme, too.

[tapaswenipathak]

Addresses review comments by @v.g.vassilev.

[mstorsjo]

FYI, this change caused warnings when built with GCC:

[1/1] Building CXX object tools/clang/unittests/Basic/CMakeFiles/BasicTests.dir/DiagnosticTest.cpp.o
../tools/clang/unittests/Basic/DiagnosticTest.cpp:17:6: warning: ‘void clang::DiagnosticsTestHelper(clang::DiagnosticsEngine&)’ has not been declared within ‘clang’
   17 | void clang::DiagnosticsTestHelper(DiagnosticsEngine &diag) {
      |      ^~~~~
In file included from ../tools/clang/unittests/Basic/DiagnosticTest.cpp:9:
../tools/clang/include/clang/Basic/Diagnostic.h:548:15: note: only here as a ‘friend’
  548 |   friend void DiagnosticsTestHelper(DiagnosticsEngine &);
      |               ^~~~~~~~~~~~~~~~~~~~~

clang/include/clang/Basic/Diagnostic.h

   DiagnosticsEngine &operator=(const DiagnosticsEngine &) = delete;
   ~DiagnosticsEngine();
 
+  friend void DiagnosticsTestHelper(DiagnosticsEngine &);
   LLVM_DUMP_METHOD void dump() const;
   LLVM_DUMP_METHOD void dump(StringRef DiagName) const;
 

     LastDiagLevel = Other.LastDiagLevel;
   }
 
-  /// Reset the state of the diagnostic object to its initial
-  /// configuration.
-  void Reset();
+  /// Reset the state of the diagnostic object to its initial configuration.
+  /// \param[in] soft - if true, doesn't reset the diagnostic mappings and state
+  void Reset(bool soft = false);
 
   //===--------------------------------------------------------------------===//
   // DiagnosticsEngine classification and reporting interfaces.

clang/lib/Basic/Diagnostic.cpp

   return true;
 }
 
-void DiagnosticsEngine::Reset() {
+void DiagnosticsEngine::Reset(bool soft /*=false*/) {
   ErrorOccurred = false;
   UncompilableErrorOccurred = false;
   FatalErrorOccurred = false;

   LastDiagLevel = DiagnosticIDs::Ignored;
   DelayedDiagID = 0;
 
-  // Clear state related to #pragma diagnostic.
-  DiagStates.clear();
-  DiagStatesByLoc.clear();
-  DiagStateOnPushStack.clear();
+  if (!soft) {
+    // 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,

clang/lib/Interpreter/IncrementalParser.cpp

       }
     }
 
-    // FIXME: Do not reset the pragma handlers.
-    Diags.Reset();
+    Diags.Reset(/*soft=*/true);
     return llvm::make_error<llvm::StringError>("Parsing failed.",
                                                std::error_code());
   }

clang/test/Interpreter/error-recovery-pragmas.cpp

+// RUN: cat %s | clang-repl -Xcc -Xclang -Xcc -verify
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wmultichar"
+// Reset should not delete #pragmas
+error; // expected-error {{a type specifier is required for all declarations}}
+void no_diag_multichar(void) { char c = (char)'ab'; }
+quit

clang/tools/clang-repl/ClangRepl.cpp

   exit(GenCrashDiag ? 70 : 1);
 }
 
+// If we are running with -verify a reported has to be returned as unsuccess.
+// This is relevant especially for the test suite.
+static int checkDiagErrors(const clang::CompilerInstance *CI) {
+  unsigned Errs = CI->getDiagnostics().getClient()->getNumErrors();
+  if (CI->getDiagnosticOpts().VerifyDiagnostics) {
+    // If there was an error that came from the verifier we must return 1 as
+    // an exit code for the process. This will make the test fail as expected.
+    clang::DiagnosticConsumer *Client = CI->getDiagnostics().getClient();
+    Client->EndSourceFile();
+    Errs = Client->getNumErrors();
+
+    // The interpreter expects BeginSourceFile/EndSourceFiles to be balanced.
+    Client->BeginSourceFile(CI->getLangOpts(), &CI->getPreprocessor());
+  }
+  return Errs ? EXIT_FAILURE : EXIT_SUCCESS;
+}
+
 llvm::ExitOnError ExitOnErr;
 int main(int argc, const char **argv) {
   ExitOnErr.setBanner("clang-repl: ");

 
   llvm::llvm_shutdown();
 
-  return 0;
+  return checkDiagErrors(Interp->getCompilerInstance());
 }

clang/unittests/Basic/DiagnosticTest.cpp

 using namespace llvm;
 using namespace clang;
 
+void clang::DiagnosticsTestHelper(DiagnosticsEngine &diag) {
+  unsigned delayedDiagID = 0U;
+
+  EXPECT_EQ(diag.DelayedDiagID, delayedDiagID);
+  EXPECT_FALSE(diag.DiagStates.empty());
+  EXPECT_TRUE(diag.DiagStatesByLoc.empty());
+  EXPECT_TRUE(diag.DiagStateOnPushStack.empty());
+}
+
 namespace {
 
 // Check that DiagnosticErrorTrap works with SuppressAllDiagnostics.

     EXPECT_EQ(Diags.getNumWarnings(), FatalsAsError);
   }
 }
+
+// Check that soft RESET works as intended
+TEST(DiagnosticTest, softReset) {
+  DiagnosticsEngine Diags(new DiagnosticIDs(), new DiagnosticOptions,
+                          new IgnoringDiagConsumer());
+
+  unsigned numWarnings = 0U, numErrors = 0U;
+
+  Diags.Reset(true);
+  // Check For ErrorOccurred and TrapNumErrorsOccurred
+  EXPECT_FALSE(Diags.hasErrorOccurred());
+  EXPECT_FALSE(Diags.hasFatalErrorOccurred());
+  EXPECT_FALSE(Diags.hasUncompilableErrorOccurred());
+  // Check for UnrecoverableErrorOccurred and TrapNumUnrecoverableErrorsOccurred
+  EXPECT_FALSE(Diags.hasUnrecoverableErrorOccurred());
+
+  EXPECT_EQ(Diags.getNumWarnings(), numWarnings);
+  EXPECT_EQ(Diags.getNumErrors(), numErrors);
+
+  // Check for private variables of DiagnosticsEngine differentiating soft reset
+  DiagnosticsTestHelper(Diags);
+
+  EXPECT_FALSE(Diags.isDiagnosticInFlight());
+  EXPECT_TRUE(Diags.isLastDiagnosticIgnored());
+}
+
 TEST(DiagnosticTest, diagnosticError) {
   DiagnosticsEngine Diags(new DiagnosticIDs(), new DiagnosticOptions,
                           new IgnoringDiagConsumer());