[lldb/Driver] Exit with a non-zero exit code in batch mode when stopping because of an error.

[JDevlieghere]

Summary

We have the option to stop running commands in batch mode when an error occurs. Currently we still exit with a zero exit code when that happens. This is counter intuitive and not very POSIX-y.

[vsk]

Can we delete an override of SBDebugger::RunCommandInterpreter, or are they all part of the stable API? If we can, it'd be nice to get rid of the one with 6 args.

Otherwise this lgtm.

[labath]

lldb/source/API/SBDebugger.cpp

1205–1209

What's the relationship between num_errors and stopped_for_error? Could we infer that we stopped because of an error if num_errors>0 ?

[labath]

In D78825#2002598, @vsk wrote:

Can we delete an override of SBDebugger::RunCommandInterpreter, or are they all part of the stable API?

They are. The way that has been handled so far is that once a function starts to get too many overloads, we create a overload taking a class like SBFuncOptions, which contains all the arguments and is extensible. That seems to have kind of happened here already, as we have the SBCommandInterpreterRunOptions class. However, in this case, that struct only contains "input" arguments, which is why I am reluctant to recommend shove output arguments there.

That said, I am wondering about the semantics of the num_errors argument. If it is nonzero only in cases where we "stop because of an error" (or we can adjust it so that this is the case), then maybe we don't need a new api for this...

[JDevlieghere]

lldb/source/API/SBDebugger.cpp

1205–1209

I considered that, m_num_errors is incremented unconditionally, while stopped_for_error is only set when eHandleCommandFlagStopOnError is set. If you know that "stop on error" is set you could infer it based on the value being non-zero, but to me that sounds a bit fragile, and given that we already have stopped_for_crash I preferred the symmetry. I'm not married to this approach so if you feel strongly about it I'm happy to change it.

[clayborg]

In D78825#2002598, @vsk wrote:

Can we delete an override of SBDebugger::RunCommandInterpreter, or are they all part of the stable API? If we can, it'd be nice to get rid of the one with 6 args.

We can't remove any existing API because it is stable, yes.

See my inline comment and let me know your thoughts.

lldb/include/lldb/API/SBDebugger.h

300–303 ↗

(On Diff #259965)

I'd vote to add a new function like:

SBCommandInterpreterRunResults RunCommandInterpreter(SBCommandInterpreterRunOptions &options);

We would need to add "auto_handle_events" and "spawn_thread" to SBCommandInterpreterRunOptions:

class LLDB_API SBCommandInterpreterRunOptions {
  friend class SBDebugger;
  friend class SBCommandInterpreter;

public:
  bool GetAutoHandleEvents() const;
  void SetAutoHandleEvents(bool);

  bool GetSpawnThread() const;
  void SetSpawnThread(bool);

and then we have an object: SBCommandInterpreterRunResults that we can query for number of errors, quit requested, stopped_for_crash, and stopped_for_error using accessors. This allows us to add new accessors without changing the API.

[labath]

(I made this comment yesterday, but it seems I didn't click send)

lldb/source/API/SBDebugger.cpp

1205–1209

Yea, needing to remember what you set as the value for "stop on error" does make this fairly finicky. OTOH, one could argue that errors shouldn't be counted as real errors (and reported here) unless stop-on-error is set. That would make checking for error-stops easier.

I don't feel strongly about any of that, but I can't say I am thrilled by the prospect of introducing another overload. So, if we do that, I'd like to ensure some thought goes into it.

Greg's idea of making a struct for the return values sounds like a good one -- having so many by-ref results is not very nice (I don't like having even one tbh). My question there would be should we have separate bool flags for the "stop reasons". It seems like these conditions should be mutually exclusive, so it may be better to have an enum to indicate a reason for stopping (+ an accessor for the error count, I guess -- though I am not exactly sure how useful it is to know the exact number of errors without being able to access them in any way).

[JDevlieghere]

Use SBCommandInterpreterRunResults

[JDevlieghere]

Greg, you requested changes to this revision, I guess because of the dependencies. Anything here you'd like to see changed?

[labath]

In D78825#2015326, @JDevlieghere wrote:

Greg, you requested changes to this revision, I guess because of the dependencies. Anything here you'd like to see changed?

I'm guessing the answer is no, but it would certainly help of you rebased the patch to see how the patch looks like after the changes in the previous patches.

[JDevlieghere]

In D78825#2017204, @labath wrote:

In D78825#2015326, @JDevlieghere wrote:

Greg, you requested changes to this revision, I guess because of the dependencies. Anything here you'd like to see changed?

I'm guessing the answer is no, but it would certainly help of you rebased the patch to see how the patch looks like after the changes in the previous patches.

Thanks, I thought I had.

[clayborg]

LGTM now. Pavel?

[labath]

I note that the call to exit(1) bypasses some of the finalization code in (most notably, reset_stdin_termios). That does not seem ideal, but it looks like a pre-existing problem.

lldb/include/lldb/API/SBCommandInterpreterRunOptions.h

 
 public:
   SBCommandInterpreterRunResults();
   ~SBCommandInterpreterRunResults() = default;
 
   int GetNumberOfErrors() const;
   bool GetQuitRequested() const;
   bool GetStoppedForCrash() const;
+  bool GetStoppedForError() const;
 
 protected:
   void SetNumberOfErrors(int);
   void SetQuitRequested(bool);
   void SetStoppedForCrash(bool);
+  void SetStoppedForError(bool);
 
 private:
   int m_num_errors;
   bool m_quit_requested;
   bool m_stopped_for_crash;
+  bool m_stopped_for_error;
 };
 
 } // namespace lldb
 
 #endif // LLDB_API_SBCOMMANDINTERPRETERRUNOPTIONS_H

lldb/include/lldb/Interpreter/CommandInterpreter.h

public:
   bool GetQuitRequested() const { return m_quit_requested; }
 
   lldb::IOHandlerSP
   GetIOHandler(bool force_create = false,
                CommandInterpreterRunOptions *options = nullptr);
 
   bool GetStoppedForCrash() const { return m_stopped_for_crash; }
 
+  bool GetStoppedForError() const { return m_stopped_for_error; }
+
   bool GetSpaceReplPrompts() const;
 
 protected:
   friend class Debugger;
 
   // IOHandlerDelegate functions
   void IOHandlerInputComplete(IOHandler &io_handler,
                               std::string &line) override;
private:
   ChildrenTruncatedWarningStatus m_truncation_warning; // Whether we truncated
                                                        // children and whether
                                                        // the user has been told
   uint32_t m_command_source_depth;
   std::vector<uint32_t> m_command_source_flags;
   uint32_t m_num_errors;
   bool m_quit_requested;
   bool m_stopped_for_crash;
+  bool m_stopped_for_error;
 
   // The exit code the user has requested when calling the 'quit' command.
   // No value means the user hasn't set a custom exit code so far.
   llvm::Optional<int> m_quit_exit_code;
   // If the driver is accepts custom exit codes for the 'quit' command.
   bool m_allow_exit_code = false;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_INTERPRETER_COMMANDINTERPRETER_H

lldb/source/API/SBCommandInterpreterRunOptions.cpp

 
 bool SBCommandInterpreterRunResults::GetStoppedForCrash() const {
   LLDB_RECORD_METHOD_CONST_NO_ARGS(bool, SBCommandInterpreterRunResults,
                                    GetStoppedForCrash);
 
   return m_stopped_for_crash;
 }
 
+bool SBCommandInterpreterRunResults::GetStoppedForError() const {
+  LLDB_RECORD_METHOD_CONST_NO_ARGS(bool, SBCommandInterpreterRunResults,
+                                   GetStoppedForError);
+
+  return m_stopped_for_error;
+}
+
 void SBCommandInterpreterRunResults::SetNumberOfErrors(int num_errors) {
   m_num_errors = num_errors;
 }
 
 void SBCommandInterpreterRunResults::SetQuitRequested(bool quit_requested) {
   m_quit_requested = quit_requested;
 }
 void SBCommandInterpreterRunResults::SetStoppedForCrash(
     bool stopped_for_crash) {
   m_stopped_for_crash = stopped_for_crash;
 }
 
+void SBCommandInterpreterRunResults::SetStoppedForError(
+    bool stopped_for_error) {
+  m_stopped_for_error = stopped_for_error;
+}
+
 namespace lldb_private {
 namespace repro {
 
 template <> void RegisterMethods<SBCommandInterpreterRunOptions>(Registry &R) {
   LLDB_REGISTER_CONSTRUCTOR(SBCommandInterpreterRunOptions, ());
   LLDB_REGISTER_METHOD_CONST(bool, SBCommandInterpreterRunOptions,
                              GetStopOnContinue, ());
   LLDB_REGISTER_METHOD(void, SBCommandInterpreterRunOptions, SetStopOnContinue,
LLDB_REGISTER_METHOD(void, SBCommandInterpreterRunOptions, SetSpawnThread,
                        (bool));
   LLDB_REGISTER_CONSTRUCTOR(SBCommandInterpreterRunResults, ());
   LLDB_REGISTER_METHOD_CONST(int, SBCommandInterpreterRunResults,
                              GetNumberOfErrors, ());
   LLDB_REGISTER_METHOD_CONST(bool, SBCommandInterpreterRunResults,
                              GetQuitRequested, ());
   LLDB_REGISTER_METHOD_CONST(bool, SBCommandInterpreterRunResults,
                              GetStoppedForCrash, ());
+  LLDB_REGISTER_METHOD_CONST(bool, SBCommandInterpreterRunResults,
+                             GetStoppedForError, ());
 }
 
 } // namespace repro
 } // namespace lldb_private

lldb/source/API/SBDebugger.cpp

   }
 }
 
 SBCommandInterpreterRunResults SBDebugger::RunCommandInterpreter(
     const SBCommandInterpreterRunOptions &options) {
   if (!m_opaque_sp)
     return {};
 
   CommandInterpreter &interp = m_opaque_sp->GetCommandInterpreter();
   interp.RunCommandInterpreter(options.ref());
 
   SBCommandInterpreterRunResults results;
   results.SetNumberOfErrors(interp.GetNumErrors());
   results.SetQuitRequested(interp.GetQuitRequested());
   results.SetStoppedForCrash(interp.GetStoppedForCrash());
+  results.SetStoppedForError(interp.GetStoppedForError());
 
   return results;
 }
 
 SBError SBDebugger::RunREPL(lldb::LanguageType language,
                             const char *repl_options) {
   LLDB_RECORD_METHOD(lldb::SBError, SBDebugger, RunREPL,
                      (lldb::LanguageType, const char *), language,

lldb/source/Interpreter/CommandInterpreter.cpp

       Properties(OptionValuePropertiesSP(
           new OptionValueProperties(ConstString("interpreter")))),
       IOHandlerDelegate(IOHandlerDelegate::Completion::LLDBCommand),
       m_debugger(debugger), m_synchronous_execution(true),
       m_skip_lldbinit_files(false), m_skip_app_init_files(false),
       m_command_io_handler_sp(), m_comment_char('#'),
       m_batch_command_mode(false), m_truncation_warning(eNoTruncation),
       m_command_source_depth(0), m_num_errors(0), m_quit_requested(false),
-      m_stopped_for_crash(false) {
+      m_stopped_for_crash(false), m_stopped_for_error(false) {
   SetEventName(eBroadcastBitThreadShouldExit, "thread-should-exit");
   SetEventName(eBroadcastBitResetPrompt, "reset-prompt");
   SetEventName(eBroadcastBitQuitCommandReceived, "quit");
   SetSynchronous(synchronous_execution);
   CheckInWithManager();
   m_collection_sp->Initialize(g_interpreter_properties);
 }
 

   case eReturnStatusSuccessContinuingNoResult:
   case eReturnStatusSuccessContinuingResult:
     if (io_handler.GetFlags().Test(eHandleCommandFlagStopOnContinue))
       io_handler.SetIsDone(true);
     break;
 
   case eReturnStatusFailed:
     m_num_errors++;
-    if (io_handler.GetFlags().Test(eHandleCommandFlagStopOnError))
+    if (io_handler.GetFlags().Test(eHandleCommandFlagStopOnError)) {
       io_handler.SetIsDone(true);
+      m_stopped_for_error = true;
+    }
     break;
 
   case eReturnStatusQuit:
     m_quit_requested = true;
     io_handler.SetIsDone(true);
     break;
   }
 

lldb/test/Shell/Commands/command-source.test

 # Check that stop command source on error.
 
-# RUN: %lldb -x -b -o "command source -e 1 %s" 2>&1 | FileCheck %s --check-prefix STOP
+# RUN: not %lldb -x -b -o "command source -e 1 %s" 2>&1 | FileCheck %s --check-prefix STOP
 # RUN: %lldb -x -b -o "command source -e 0 %s" 2>&1 | FileCheck %s --check-prefix CONTINUE
-# RUN: %lldb -x -b -o 'settings set interpreter.stop-command-source-on-error true' -o "command source %s" 2>&1 | FileCheck %s --check-prefix STOP
+# RUN: not %lldb -x -b -o 'settings set interpreter.stop-command-source-on-error true' -o "command source %s" 2>&1 | FileCheck %s --check-prefix STOP
 # RUN: %lldb -x -b -o 'settings set interpreter.stop-command-source-on-error false' -o "command source %s" 2>&1 | FileCheck %s --check-prefix CONTINUE
 
 bogus
 p 10+1
 
 # CONTINUE: $0 = 11
 # STOP-NOT: $0 = 11

lldb/test/Shell/Driver/TestProcessAttach.test

-# RUN: %lldb -x -b -S %S/Inputs/process_attach_pid.in 2>&1 | FileCheck %s
+# RUN: not %lldb -x -b -S %S/Inputs/process_attach_pid.in 2>&1 | FileCheck %s
 # CHECK: last option requires an argument

lldb/test/Shell/Host/TestCustomShell.test

 # This test applies to POSIX.
 # UNSUPPORTED: system-windows
 
 # FIXME: ShellExpandArguments is unimplemented on the following targets.
 # UNSUPPORTED: system-linux
 # XFAIL: system-freebsd
 # XFAIL: system-netbsd
 # XFAIL: system-openbsd
 
 # RUN: %clang_host %S/Inputs/simple.c -g -o %t.out
-# RUN: SHELL=bogus %lldb %t.out -b -o 'run' 2>&1 | FileCheck %s --check-prefix ERROR
+# RUN: SHELL=bogus not %lldb %t.out -b -o 'run' 2>&1 | FileCheck %s --check-prefix ERROR
 # RUN: env -i %lldb %t.out -b -o 'run' 2>&1 | FileCheck %s
 
 # ERROR: error: shell expansion failed
 # CHECK-NOT: error: shell expansion failed

lldb/test/Shell/Quit/TestQuitExitCodeNonInt.test

 # UNSUPPORTED: system-windows
-# RUN: %lldb -b -s %s 2>&1 | FileCheck %s
+# RUN: not %lldb -b -s %s 2>&1 | FileCheck %s
 q str
 // CHECK: Couldn't parse 'str'

lldb/test/Shell/Quit/TestQuitExitCodeTooManyArgs.test

 # UNSUPPORTED: system-windows
-# RUN: %lldb -b -s %s 2>&1 | FileCheck %s
+# RUN: not %lldb -b -s %s 2>&1 | FileCheck %s
 q 1 2
 // CHECK: Too many arguments for 'quit'

lldb/test/Shell/Reproducer/TestDiscard.test

 # This ensures that the reproducer properly cleans up after itself.
 
 # Build the inferior.
 # RUN: mkdir -p %t
 # RUN: rm -rf %t.repro
 # RUN: %clang_host %S/Inputs/simple.c -g -o %t/reproducer.out
 
 # Capture but don't generate the reproducer.
-# RUN: %lldb -x -b -s %S/Inputs/Discard.in --capture --capture-path %t.repro %t/reproducer.out
+# RUN: not %lldb -x -b -s %S/Inputs/Discard.in --capture --capture-path %t.repro %t/reproducer.out
 
 # Make sure the directory doesn't exist.
 # RUN: mkdir %t.repro

lldb/test/Shell/Reproducer/TestDump.test

 
 # RUN: %lldb -b -o 'reproducer dump -p gdb -f %t.repro' | FileCheck %s --check-prefix GDB
 # GDB: send packet: $QStartNoAckMode#b0
 # GDB: read packet: $OK#9a
 
 # RUN: %lldb --replay %t.repro | FileCheck %s --check-prefix FILES
 
 # RUN: rm %t.repro/gdb-remote.yaml
-# RUN: %lldb -b -o 'reproducer dump -p gdb -f %t.repro' 2>&1 | FileCheck %s --check-prefix GDB-ERROR
+# RUN: not %lldb -b -o 'reproducer dump -p gdb -f %t.repro' 2>&1 | FileCheck %s --check-prefix GDB-ERROR
 # GDB-ERROR: error: Unable to create GDB loader.
 
 # RUN: rm %t.repro/command-interpreter.yaml
-# RUN: %lldb -b -o 'reproducer dump -p commands -f %t.repro' 2>&1 | FileCheck %s --check-prefix COMMANDS-ERROR
+# RUN: not %lldb -b -o 'reproducer dump -p commands -f %t.repro' 2>&1 | FileCheck %s --check-prefix COMMANDS-ERROR
 # COMMANDS-ERROR: error: Unable to create command loader.

lldb/test/Shell/Settings/TestSettingsSet.test

 # This tests setting setting values.
 
 # Check that setting an empty value with -f(orce) clears the value.
-# RUN: %lldb -b -s %s 2>&1 | FileCheck %s
+# RUN: not %lldb -b -s %s 2>&1 | FileCheck %s
 
 settings set tab-size 16
 settings show tab-size
 # CHECK: tab-size (unsigned) = 16
 
 settings set -f tab-size
 settings show tab-size
 # CHECK: tab-size (unsigned) = 4
 
 settings set tab-size
 # CHECK: error: 'settings set' takes more arguments

lldb/test/Shell/Settings/TestStopCommandSourceOnError.test

 # Modifying the interpreter settings is tricky because they don't take effect
 # until we create a new command interpreter, which should be merely an
 # implementation detail. This leads to confusing and unexpected scenarios.
 #
 # Below are a few scenarios that we should fix.
 
 # CONTINUE: 123456789
 # STOP-NOT: 111111111
 # STOP-NOT: 123456789
 
 # FIXME: Should stop
 # RUN: %lldb -b -o 'settings set interpreter.stop-command-source-on-error false' -s %S/Inputs/StopCommandSource.in | FileCheck %s --check-prefix CONTINUE
 
 # FIXME: Should continue
-# RUN: %lldb -b -s %S/Inputs/DontStopCommandSource.in -o 'bogus' -o 'print 111100000 + 11111' | FileCheck %s --check-prefix STOP
+# RUN: not %lldb -b -s %S/Inputs/DontStopCommandSource.in -o 'bogus' -o 'print 111100000 + 11111' | FileCheck %s --check-prefix STOP
 
 # FIXME: Should continue
-# RUN: %lldb -b -o 'settings set interpreter.stop-command-source-on-error false' -o 'bogus' -o 'print 123400000 + 56789'  | FileCheck %s --check-prefix STOP
+# RUN: not %lldb -b -o 'settings set interpreter.stop-command-source-on-error false' -o 'bogus' -o 'print 123400000 + 56789'  | FileCheck %s --check-prefix STOP
 
 # FIXME: Should continue
-# RUN: %lldb -b -s %S/Inputs/DontStopCommandSource.in | FileCheck %s --check-prefix STOP
+# RUN: not %lldb -b -s %S/Inputs/DontStopCommandSource.in | FileCheck %s --check-prefix STOP
 
 # FIXME: Should continue
-# RUN: %lldb -b -o 'settings set interpreter.stop-command-source-on-error true' -s %S/Inputs/DontStopCommandSource.in | FileCheck %s --check-prefix STOP
+# RUN: not %lldb -b -o 'settings set interpreter.stop-command-source-on-error true' -s %S/Inputs/DontStopCommandSource.in | FileCheck %s --check-prefix STOP

lldb/test/Shell/SymbolFile/DWARF/debug-types-missing-signature.test

 Create a dangling DW_AT_signature reference by stripping the debug_types
 section, and make sure lldb does something reasonable.
 RUN: %clangxx -target x86_64-pc-linux %S/Inputs/debug-types-basic.cpp \
 RUN:   -g -gdwarf-4 -fdebug-types-section -c -o %t.o
 RUN: llvm-objcopy --remove-section=.debug_types %t.o %t
 
 
 RUN: %lldb %t -b -o "type lookup A" | FileCheck --check-prefix=LOOKUPA %s
 LOOKUPA: no type was found matching 'A'
 
 RUN: %lldb %t -b -o "type lookup E" | FileCheck --check-prefix=LOOKUPE %s
 LOOKUPE: no type was found matching 'E'
 
 RUN: %lldb %t -b -o "type lookup EC" | FileCheck --check-prefix=LOOKUPEC %s
 LOOKUPEC: no type was found matching 'EC'
 
-RUN: %lldb %t -b -o "print (E) 1" 2>&1 | FileCheck --check-prefix=PRINTE %s
+RUN: not %lldb %t -b -o "print (E) 1" 2>&1 | FileCheck --check-prefix=PRINTE %s
 PRINTE: use of undeclared identifier 'E'
 
-RUN: %lldb %t -b -o "print (EC) 1" 2>&1 | FileCheck --check-prefix=PRINTEC %s
+RUN: not %lldb %t -b -o "print (EC) 1" 2>&1 | FileCheck --check-prefix=PRINTEC %s
 PRINTEC: use of undeclared identifier 'EC'
 
 RUN: %lldb %t -b -o "target variable a e ec" | FileCheck --check-prefix=VARS %s
 VARS: (const (anonymous struct)) a = {}
 VARS: (const (anonymous enum)) e = 0x1
 VARS: (const (anonymous enum)) ec = 0x1

lldb/test/Shell/Unwind/thread-step-out-ret-addr-check.test

 # Test that `thread step-out` fails when the "return address"
 # points to non-executable memory.
 
 # REQUIRES: target-x86_64
 # UNSUPPORTED: system-windows
 
 # RUN: %clang_host %p/Inputs/call-asm.c -x assembler-with-cpp %p/Inputs/thread-step-out-ret-addr-check.s -o %t
-# RUN: %lldb %t -s %s -b 2>&1 | FileCheck %s
+# RUN: not %lldb %t -s %s -b 2>&1 | FileCheck %s
 
 breakpoint set -n nonstandard_stub
 # CHECK: Breakpoint 1: where = {{.*}}`nonstandard_stub
 
 process launch
 # CHECK: stop reason = breakpoint 1.1
 
 thread step-out
 # CHECK: Could not create return address breakpoint.
 # CHECK: Return address (0x{{[a-f0-9]*}}) did not point to executable memory.

lldb/tools/driver/Driver.cpp

if (m_option_data.m_batch && results.GetStoppedForCrash() &&
       commands_file =
           PrepareCommandsForSourcing(crash_commands_data, crash_commands_size);
       if (commands_file != nullptr) {
         m_debugger.SetInputFileHandle(commands_file, true);
         SBCommandInterpreterRunResults local_results =
             m_debugger.RunCommandInterpreter(options);
         if (local_results.GetQuitRequested())
           go_interactive = false;
+
+        // When running in batch mode and an error occurred while sourcing
+        // the crash commands, exit with a non-zero exit status.
+        if (m_option_data.m_batch && local_results.GetStoppedForError())
+          exit(1);
       }
     }
+
+    // When running in batch mode and stopped because of an error, exit with
+    // a non-zero exit status.
+    if (m_option_data.m_batch && results.GetStoppedForError())
+      exit(1);
+
     m_debugger.SetAsync(old_async);
   }
 
   // Now set the input file handle to STDIN and run the command interpreter
   // again in interactive mode or repl mode and let the debugger take ownership
   // of stdin.
   if (go_interactive) {
     m_debugger.SetInputFileHandle(stdin, true);