[clang-repl] Fix incorrect return code

[junaire]

Summary

Without this patch, clang-repl incorrectly pass some tests when there's
error occured.

Signed-off-by: Jun Zhang <[e-mail]>

[v.g.vassilev]

clang/test/Interpreter/fail.cpp

7

Why this is marked as XFAIL? If it is only for the return code we can add the not command in front to make the error code succeed.

[junaire]

Use not

[v.g.vassilev]

Thanks for working on this. This is one of the problems where is hard to find the right behavior. For example, if a repl running in interactive issues an error and then successfully recovers what's the right process return code success or a failure? If we decide it is a success then if we ran in non-interactive mode eg clang-repl "err" then the exit code should be a failure.

Either way, this patch improves our current state in which we cannot detect failing tests in -verify mode. Let's accept it.

[junaire]

In D130422#3689392, @v.g.vassilev wrote:

Thanks for working on this. This is one of the problems where is hard to find the right behavior. For example, if a repl running in interactive issues an error and then successfully recovers what's the right process return code success or a failure? If we decide it is a success then if we ran in non-interactive mode eg clang-repl "err" then the exit code should be a failure.

Either way, this patch improves our current state in which we cannot detect failing tests in -verify mode. Let's accept it.

Maybe add a FIXME in the tests?

[v.g.vassilev]

In D130422#3689397, @junaire wrote:

In D130422#3689392, @v.g.vassilev wrote:

Thanks for working on this. This is one of the problems where is hard to find the right behavior. For example, if a repl running in interactive issues an error and then successfully recovers what's the right process return code success or a failure? If we decide it is a success then if we ran in non-interactive mode eg clang-repl "err" then the exit code should be a failure.

Either way, this patch improves our current state in which we cannot detect failing tests in -verify mode. Let's accept it.

Maybe add a FIXME in the tests?

That's probably a good idea.

[junaire]

Rebase and add a FIXME

[junaire]

Fix some typos

[junaire]

@v.g.vassilev if you can take another look at the wording of the FIXME, and make sure you're happy about it, that will be awesome.

[v.g.vassilev]

clang/tools/clang-repl/ClangRepl.cpp

138

This seems not needed. We have already llvm::llvm_shutdown_obj Y;

[junaire]

clang/tools/clang-repl/ClangRepl.cpp

138

This seems not needed. We have already llvm::llvm_shutdown_obj Y;

Oh good catch, I think I messed it in the rebase :(

clang/test/Interpreter/fail.cpp

+// FIXME: There're some inconsistencies between interactive and non-interactive
+// modes. For example, when clang-repl runs in the interactive mode, issues an
+// error, and then successfully recovers if we decide it's a success then for
+// the non-interactive mode the exit code should be a failure.
+// RUN: clang-repl "int x = 10;" "int y=7; err;" "int y = 10;"
+// RUN: clang-repl "int i = 10;" 'extern "C" int printf(const char*,...);' \
+// RUN:            'auto r1 = printf("i = %d\n", i);' | FileCheck --check-prefix=CHECK-DRIVER %s
+// REQUIRES: host-supports-jit
+// UNSUPPORTED: system-aix
+// CHECK-DRIVER: i = 10
+// RUN: cat %s | not clang-repl | FileCheck %s
+BOOM!
+extern "C" int printf(const char *, ...);
+int i = 42;
+auto r1 = printf("i = %d\n", i);
+// CHECK: i = 42
+%quit

clang/tools/clang-repl/ClangRepl.cpp

static void LLVMErrorHandler(void *UserData, const char *Message,
   // 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) {
+static int checkDiagErrors(const clang::CompilerInstance *CI, bool HasError) {
   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;
+  return (Errs || HasError) ? EXIT_FAILURE : EXIT_SUCCESS;
 }
 
 llvm::ExitOnError ExitOnErr;
 int main(int argc, const char **argv) {
   ExitOnErr.setBanner("clang-repl: ");
   llvm::cl::ParseCommandLineOptions(argc, argv);
 
   llvm::llvm_shutdown_obj Y; // Call llvm_shutdown() on exit.
int main(int argc, const char **argv) {
   CI->LoadRequestedPlugins();
 
   auto Interp = ExitOnErr(clang::Interpreter::create(std::move(CI)));
   for (const std::string &input : OptInputs) {
     if (auto Err = Interp->ParseAndExecute(input))
       llvm::logAllUnhandledErrors(std::move(Err), llvm::errs(), "error: ");
   }
 
+  bool HasError = false;
+
   if (OptInputs.empty()) {
     llvm::LineEditor LE("clang-repl");
     // FIXME: Add LE.setListCompleter
     while (llvm::Optional<std::string> Line = LE.readLine()) {
       if (*Line == R"(%quit)")
         break;
       if (*Line == R"(%undo)") {
-        if (auto Err = Interp->Undo())
+        if (auto Err = Interp->Undo()) {
           llvm::logAllUnhandledErrors(std::move(Err), llvm::errs(), "error: ");
+          HasError = true;
+        }
         continue;
       }
 
-      if (auto Err = Interp->ParseAndExecute(*Line))
+      if (auto Err = Interp->ParseAndExecute(*Line)) {
         llvm::logAllUnhandledErrors(std::move(Err), llvm::errs(), "error: ");
+        HasError = true;
+      }
     }
   }
 
   // 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();
 
-  return checkDiagErrors(Interp->getCompilerInstance());
+  return checkDiagErrors(Interp->getCompilerInstance(), HasError);
 }