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

public:
   explicit DiagnosticsEngine(IntrusiveRefCntPtr<DiagnosticIDs> Diags,
                              IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts,
                              DiagnosticConsumer *client = nullptr,
                              bool ShouldOwnClient = true);
   DiagnosticsEngine(const DiagnosticsEngine &) = delete;
   DiagnosticsEngine &operator=(const DiagnosticsEngine &) = delete;
   ~DiagnosticsEngine();
 
+  friend void DiagnosticsTestHelper(DiagnosticsEngine &);
   LLVM_DUMP_METHOD void dump() const;
   LLVM_DUMP_METHOD void dump(StringRef DiagName) const;
 
   const IntrusiveRefCntPtr<DiagnosticIDs> &getDiagnosticIDs() const {
     return Diags;
   }
 
   /// Retrieve the diagnostic options.
public:
   }
 
   /// Note that the prior diagnostic was emitted by some other
   /// \c DiagnosticsEngine, and we may be attaching a note to that diagnostic.
   void notePriorDiagnosticFrom(const DiagnosticsEngine &Other) {
     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.
   //
 
   /// Determine whether the diagnostic is known to be ignored.
   ///
   /// This can be used to opportunistically avoid expensive checks when it's

clang/lib/Basic/Diagnostic.cpp

bool DiagnosticsEngine::popMappings(SourceLocation Loc) {
   if (DiagStateOnPushStack.back() != GetCurDiagState()) {
     // State changed at some point between push/pop.
     PushDiagStatePoint(DiagStateOnPushStack.back(), Loc);
   }
   DiagStateOnPushStack.pop_back();
   return true;
 }
 
-void DiagnosticsEngine::Reset() {
+void DiagnosticsEngine::Reset(bool soft /*=false*/) {
   ErrorOccurred = false;
   UncompilableErrorOccurred = false;
   FatalErrorOccurred = false;
   UnrecoverableErrorOccurred = false;
 
   NumWarnings = 0;
   NumErrors = 0;
   TrapNumErrorsOccurred = 0;
   TrapNumUnrecoverableErrorsOccurred = 0;
 
   CurDiagID = std::numeric_limits<unsigned>::max();
   LastDiagLevel = DiagnosticIDs::Ignored;
   DelayedDiagID = 0;
 
+  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,
                                              StringRef Arg2, StringRef Arg3) {
   if (DelayedDiagID)
     return;
 
   DelayedDiagID = DiagID;
   DelayedDiagArg1 = Arg1.str();

clang/lib/Interpreter/IncrementalParser.cpp

if (Diags.hasErrorOccurred()) {
 
     // Clean up the lookup table
     if (StoredDeclsMap *Map = PreviousTU->getPrimaryContext()->getLookupPtr()) {
       for (auto I = Map->begin(); I != Map->end(); ++I) {
         StoredDeclsList &List = I->second;
         DeclContextLookupResult R = List.getLookupResult();
         for (NamedDecl *D : R)
           if (D->getTranslationUnitDecl() == MostRecentTU)
             List.remove(D);
         if (List.isNull())
           Map->erase(I);
       }
     }
 
-    // FIXME: Do not reset the pragma handlers.
-    Diags.Reset();
+    Diags.Reset(/*soft=*/true);
+    Diags.getClient()->clear();
     return llvm::make_error<llvm::StringError>("Parsing failed.",
                                                std::error_code());
   }
 
   // Process any TopLevelDecls generated by #pragma weak.
   for (Decl *D : S.WeakTopLevelDecls()) {
     DeclGroupRef DGR(D);
     Consumer->HandleTopLevelDecl(DGR);

clang/tools/clang-repl/ClangRepl.cpp

static void LLVMErrorHandler(void *UserData, const char *Message,
 
   // We cannot recover from llvm errors.  When reporting a fatal error, exit
   // with status 70 to generate crash diagnostics.  For BSD systems this is
   // defined as an internal software error. Otherwise, exit with status 1.
 
   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::cl::ParseCommandLineOptions(argc, argv);
 
   std::vector<const char *> ClangArgv(ClangArgs.size());
   std::transform(ClangArgs.begin(), ClangArgs.end(), ClangArgv.begin(),
                  [](const std::string &s) -> const char * { return s.data(); });
int main(int argc, const char **argv) {
 
   // Our error handler depends on the Diagnostics object, which we're
   // potentially about to delete. Uninstall the handler now so that any
   // later errors use the default handling behavior instead.
   llvm::remove_fatal_error_handler();
 
   llvm::llvm_shutdown();
 
-  return 0;
+  return checkDiagErrors(Interp->getCompilerInstance());
 }

clang/unittests/Basic/DiagnosticTest.cpp

 //===- unittests/Basic/DiagnosticTest.cpp -- Diagnostic engine tests ------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticError.h"
 #include "clang/Basic/DiagnosticIDs.h"
 #include "gtest/gtest.h"
 
 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.
 TEST(DiagnosticTest, suppressAndTrap) {
   DiagnosticsEngine Diags(new DiagnosticIDs(),
                           new DiagnosticOptions,
                           new IgnoringDiagConsumer());
   Diags.setSuppressAllDiagnostics(true);
for (unsigned FatalsAsError = 0; FatalsAsError != 2; ++FatalsAsError) {
     EXPECT_TRUE(Diags.hasUncompilableErrorOccurred());
     EXPECT_TRUE(Diags.hasUnrecoverableErrorOccurred());
 
     // The warning should be emitted and counted only if we're not suppressing
     // after fatal errors.
     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());
   PartialDiagnostic::DiagStorageAllocator Alloc;
   llvm::Expected<std::pair<int, int>> Value = DiagnosticError::create(
       SourceLocation(), PartialDiagnostic(diag::err_cannot_open_file, Alloc)
                             << "file"
                             << "error");