Don't stop execution in batch mode when process stops with SIGINT or SIGSTOP

[tatyana-krasnukha]

Summary

Usually, SIGINT and SIGSTOP don't imply a crash, e.g. SIGSTOP is sent on process launch and attach on some platforms.

[labath]

What behavior does this change exactly? This is not immediately obvious to me, and probably anyone else not intimately familiar with the stop reason machinery (= everyone except Jim, probably). Could you add a test case for this (which I am hoping will also shed some light on what is this patch doing)?

[tatyana-krasnukha]

The flag m_abnormal_stop_was_expected was added in the commit to avoid stop on crash because attach always stops
with a SIGSTOP on OSX. I faced the same issue with SIGTRAP and SIGSTOP sent by a gdb-remote stub on some commands. This patch filters out such harmless signals instead of setting the flag everywhere in commands.

[JDevlieghere]

+1 on a test case. Other than that this seems like a good improvement to the way we filter stop reasons.

source/Interpreter/CommandInterpreter.cpp

2164 ↗

(On Diff #220913)

I'm curious why you swapped the two operands. Is there a benefit in eStopReasonException == reason compared to reason == eStopReasonException?

[tatyana-krasnukha]

Added a test

[tatyana-krasnukha]

source/Interpreter/CommandInterpreter.cpp

2164 ↗

(On Diff #220913)

This technique prevents unintended assignment instead of comparison since left-hand value is constant.

[JDevlieghere]

packages/Python/lldbsuite/test/driver/batch_mode/TestBatchMode.py

83 ↗

(On Diff #221058)

Update the comment?

source/Interpreter/CommandInterpreter.cpp

2164 ↗

(On Diff #220913)

Ah, because the StopReason is not const. Fair enough, thanks for the explanation. I don't think I've seen that in other parts of LLDB but I'm not opposed to it.

[jingham]

This is all about the decision in "batch" mode for what constitutes a stop by the program that should cause batch mode to return the control to the user.

I don't think anybody would want to stop for SIGSTOP and SIGINT if the program under the debugger can recover.

However, builtin_trap on most UNIX systems results in a SIGTRAP. It's not at all uncommon to use builtin_trap to signal a fatal condition in a library (for instance Swift uses this on array overflow and for a number of other error conditions.) It has the benefit over "abort" that it doesn't push a bunch of stack frames onto the stack before dying, which makes crash analysis easier. This use means we certainly want batch mode to stop for SIGTRAP.

If the SIGTRAP is for a breakpoint, lldb will convert the incoming signal event into a breakpoint-hit event, which doesn't cause the batch mode to stop.

So maybe we don't actually need to have "should suppress" be true for SIGTRAP. We should be able to distinguish between traps that were really in the code of the binary and traps lldb inserted, and make the decision based on that. That "process handle" behavior was just copied from gdb, which kept the breakpoint SIGTRAP's raw. And after all, non-breakpoint SIGTRAP's are generally fatal so whether you pass them or not is not of much real concern.

I can't tell how real SIGTRAP signaled breakpoints work through lldb because on MacOS because don't come in as SIGTRAP's (they come in as Mach exceptions). It would be worth seeing how this works for Linux hosts.

But in any case, a SIGTRAP that doesn't have an underlying breakpoint should definitely cause batch mode to stop. Under what circumstances did you need to suppress it?

source/Interpreter/CommandInterpreter.cpp

2164 ↗

(On Diff #220913)

We don't use that convention anywhere else in lldb. Introducing it piecemeal just makes the code harder to read, and I personally find this convention makes code generally harder to parse in my head.

[labath]

This is an interesting problem. As Jim suspects, breakpoint int3s come out as stop reason = breakpoint in lldb, and non-breakpoint int3 as SIGTRAP. So, in theory, it should be possible to change the SIGTRAP behavior so that it does not impact SIGTRAPs from lldb-generated breakpoints, though it will require some fiddling with the QPassSignals packet -- the way it's implemented now is that it would bypass the stop reason determination and forcefully reinject all signals (even the "debugger generated" ones into the process).

That said, I'm not sure this is actually a good idea. :) One of the uses of application-generated SIGTRAPs I've seen (and used) was to implement a poor man's "fast conditional breakpoint" -- you insert something like if (do_i_want_to_stop) int3() into the code, and let the application run under a debugger. The intention here is to be able to continue the program from this "breakpoint" and this will only work if the debugger does not reinject the SIGTRAP.

Now, this is not a super common pattern, so it might be fine to just ask to user to change the SIGTRAP disposition if he wants to do this. However, given that this is the default in gdb too, I am not sure if it is such a good idea to change it. As this problem is specifically about the behavior of process attach, maybe the problem should be solved at a higher level? Could we make "process attach" never set the "crashed" flag? Attaching to a process just at the moment it is about the crash is very unlikely, and I suspect it is actually impossible (on linux at least).

Alternatively, (and this is something that would be useful in in (lit) tests too), we could add a mechanism to selectively suppress the stop-on-error behavior. Something like nohup process attach perhaps?

[jingham]

First off, note that this becomes complicated because this patch tied the decision of whether a given stop reason should warrant returning control to the user in lldb's "batch mode", and whether the signal should be passed back to the target. That isn't a necessary binding, and indeed it might be surprising to people to find out that changing the "target handle -p" option changes the behavior of batch mode. So I'm not 100% sure I think these two notions should be tied together.

[jingham]

Pavel's example shows why these two choices might be better separated. If you are using __builtin_trap to implement a stop when in the debugger of some sort, it seems likely you would want to suppress the signal, but it also seems likely that when you are running in batch mode, you would want the batch mode run to return control to the user at this point. batch mode is really so you can run some scenario in a loop, looking for something odd to happen. These debug breaks are probably a signal of that "something odd"...

[tatyana-krasnukha]

To Pavel: this problem is not only about the behavior of process attach, but for process launch too (and possibly something else). That's why I'm trying to move this logic out of specific commands.

Now I see that there are too many different usages of SIGTRAP and most of them are architecture-dependent (like __builtin_trap which can send SIGTRAP for one processor and SIGILL for another). So, a process cannot know how to distinguish the signal sender's intention. The problem is common to gdb.

[tatyana-krasnukha]

Removed using UnixSignals::ShouldSuppress, just check if a signal is either SIGINT or SIGSTOP.

Fixed header of the test.

[jingham]

LGTM. Thanks for sticking with this.

lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h

if (ch == 'd')
       return ConstString("quit\n");
     return ConstString();
   }
 
   bool IOHandlerInterrupt(IOHandler &io_handler) override;
 
   void GetProcessOutput();
 
+  bool DidProcessStopAbnormally() const;
+
   void SetSynchronous(bool value);
 
   lldb::CommandObjectSP GetCommandSP(llvm::StringRef cmd,
                                      bool include_aliases = true,
                                      bool exact = true,
                                      StringList *matches = nullptr,
                                      StringList *descriptions = nullptr) const;
 

lldb/trunk/include/lldb/Interpreter/CommandReturnObject.h

public:
   bool GetDidChangeProcessState();
 
   void SetDidChangeProcessState(bool b);
 
   bool GetInteractive() const;
 
   void SetInteractive(bool b);
 
-  bool GetAbnormalStopWasExpected() const {
-    return m_abnormal_stop_was_expected;
-  }
-
-  void SetAbnormalStopWasExpected(bool signal_was_expected) {
-    m_abnormal_stop_was_expected = signal_was_expected;
-  }
-
 private:
   enum { eStreamStringIndex = 0, eImmediateStreamIndex = 1 };
 
   StreamTee m_out_stream;
   StreamTee m_err_stream;
 
   lldb::ReturnStatus m_status;
   bool m_did_change_process_state;
   bool m_interactive; // If true, then the input handle from the debugger will
                       // be hooked up
-  bool m_abnormal_stop_was_expected; // This is to support
-                                     // eHandleCommandFlagStopOnCrash vrs.
-                                     // attach.
-  // The attach command often ends up with the process stopped due to a signal.
-  // Normally that would mean stop on crash should halt batch execution, but we
-  // obviously don't want that for attach.  Using this flag, the attach command
-  // (and anything else for which this is relevant) can say that the signal is
-  // expected, and batch command execution can continue.
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_CommandReturnObject_h_

lldb/trunk/packages/Python/lldbsuite/test/driver/batch_mode/TestBatchMode.py

def test_batch_mode_run_exit(self):
         # The App should have not have crashed:
         child.expect_exact("Got there on time and it did not crash.")
 
         # Then lldb should exit.
         child.expect_exact("exited")
         import pexpect
         child.expect(pexpect.EOF)
 
+    @expectedFlakeyFreeBSD("llvm.org/pr25172 fails rarely on the buildbot")
+    def test_batch_mode_launch_stop_at_entry(self):
+        """Test that the lldb driver's batch mode works correctly for process launch."""
+        self.build()
+
+        exe = self.getBuildArtifact("a.out")
+
+        # Launch with the option '--stop-at-entry' stops with a signal (usually SIGSTOP)
+        # that should be suppressed since it doesn't imply a crash and
+        # this is not a reason to exit batch mode.
+        extra_args = ['-b',
+            '-o', 'process launch --stop-at-entry',
+            '-o', 'continue',
+        ]
+        self.launch(executable=exe, extra_args=extra_args)
+        child = self.child
+
+        # Check that the process has been launched:
+        child.expect("Process ([0-9]+) launched:")
+        # We should have continued:
+        child.expect_exact("continue")
+        # The App should have not have crashed:
+        child.expect_exact("Got there on time and it did not crash.")
+        
+        # Then lldb should exit.
+        child.expect_exact("exited")
+        import pexpect
+        child.expect(pexpect.EOF)
+
     def closeVictim(self):
         if self.victim is not None:
             self.victim.close()
             self.victim = None
 
     @skipIfRemote  # test not remote-ready llvm.org/pr24813
     @expectedFlakeyFreeBSD("llvm.org/pr25172 fails rarely on the buildbot")
     @expectedFailureNetBSD

lldb/trunk/source/Commands/CommandObjectProcess.cpp

bool DoExecute(Args &command, CommandReturnObject &result) override {
     StreamString stream;
     const auto error = target->Attach(m_options.attach_info, &stream);
     if (error.Success()) {
       ProcessSP process_sp(target->GetProcessSP());
       if (process_sp) {
         result.AppendMessage(stream.GetString());
         result.SetStatus(eReturnStatusSuccessFinishNoResult);
         result.SetDidChangeProcessState(true);
-        result.SetAbnormalStopWasExpected(true);
       } else {
         result.AppendError(
             "no error returned from Target::Attach, and target has no process");
         result.SetStatus(eReturnStatusFailed);
       }
     } else {
       result.AppendErrorWithFormat("attach failed: %s\n", error.AsCString());
       result.SetStatus(eReturnStatusFailed);

lldb/trunk/source/Interpreter/CommandInterpreter.cpp

 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Interpreter/OptionValueProperties.h"
 #include "lldb/Interpreter/Options.h"
 #include "lldb/Interpreter/Property.h"
 #include "lldb/Utility/Args.h"
 
 #include "lldb/Target/Process.h"
+#include "lldb/Target/StopInfo.h"
 #include "lldb/Target/TargetList.h"
 #include "lldb/Target/Thread.h"
+#include "lldb/Target/UnixSignals.h"
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/PrettyStackTrace.h"
 
 using namespace lldb;
 using namespace lldb_private;
if (target)
       platform_sp = target->GetPlatform();
   }
 
   if (!platform_sp)
     platform_sp = m_debugger.GetPlatformList().GetSelectedPlatform();
   return platform_sp;
 }
 
+bool CommandInterpreter::DidProcessStopAbnormally() const {
+  TargetSP target_sp = m_debugger.GetTargetList().GetSelectedTarget();
+  if (!target_sp)
+    return false;
+
+  ProcessSP process_sp(target_sp->GetProcessSP());
+  if (!process_sp)
+    return false;
+
+  if (eStateStopped != process_sp->GetState())
+    return false;
+
+  for (const auto &thread_sp : process_sp->GetThreadList().Threads()) {
+    StopInfoSP stop_info = thread_sp->GetStopInfo();
+    if (!stop_info)
+      return false;
+
+    const StopReason reason = stop_info->GetStopReason();
+    if (reason == eStopReasonException || reason == eStopReasonInstrumentation)
+      return true;
+
+    if (reason == eStopReasonSignal) {
+      const auto stop_signal = static_cast<int32_t>(stop_info->GetValue());
+      UnixSignalsSP signals_sp = process_sp->GetUnixSignals();
+      if (!signals_sp || !signals_sp->SignalIsValid(stop_signal))
+        // The signal is unknown, treat it as abnormal.
+        return true;
+
+      const auto sigint_num = signals_sp->GetSignalNumberFromName("SIGINT");
+      const auto sigstop_num = signals_sp->GetSignalNumberFromName("SIGSTOP");
+      if ((stop_signal != sigint_num) && (stop_signal != sigstop_num))
+        // The signal very likely implies a crash.
+        return true;
+    }
+  }
+
+  return false;
+}
+
 void CommandInterpreter::HandleCommands(const StringList &commands,
                                         ExecutionContext *override_context,
                                         CommandInterpreterRunOptions &options,
                                         CommandReturnObject &result) {
   size_t num_lines = commands.GetSize();
 
   // If we are going to continue past a "continue" then we need to run the
   // commands synchronously. Make sure you reset this value anywhere you return
if ((tmp_result.GetStatus() == eReturnStatusSuccessContinuingNoResult) ||
         result.SetStatus(tmp_result.GetStatus());
         m_debugger.SetAsyncExecution(old_async_execution);
 
         return;
       }
     }
 
     // Also check for "stop on crash here:
-    bool should_stop = false;
-    if (tmp_result.GetDidChangeProcessState() && options.GetStopOnCrash()) {
-      TargetSP target_sp(m_debugger.GetTargetList().GetSelectedTarget());
-      if (target_sp) {
-        ProcessSP process_sp(target_sp->GetProcessSP());
-        if (process_sp) {
-          for (ThreadSP thread_sp : process_sp->GetThreadList().Threads()) {
-            StopReason reason = thread_sp->GetStopReason();
-            if (reason == eStopReasonSignal || reason == eStopReasonException ||
-                reason == eStopReasonInstrumentation) {
-              should_stop = true;
-              break;
-            }
-          }
-        }
-      }
-      if (should_stop) {
-        if (idx != num_lines - 1)
-          result.AppendErrorWithFormat(
-              "Aborting reading of commands after command #%" PRIu64
-              ": '%s' stopped with a signal or exception.\n",
-              (uint64_t)idx + 1, cmd);
-        else
-          result.AppendMessageWithFormat(
-              "Command #%" PRIu64 " '%s' stopped with a signal or exception.\n",
-              (uint64_t)idx + 1, cmd);
+    if (tmp_result.GetDidChangeProcessState() && options.GetStopOnCrash() &&
+        DidProcessStopAbnormally()) {
 
-        result.SetStatus(tmp_result.GetStatus());
-        m_debugger.SetAsyncExecution(old_async_execution);
 
-        return;
-      }
-    }
-  }
 
   result.SetStatus(eReturnStatusSuccessFinishResult);
   m_debugger.SetAsyncExecution(old_async_execution);
 
   return;
 }
 
 // Make flags that we can pass into the IOHandler so our delegates can do the
void CommandInterpreter::IOHandlerInputComplete(IOHandler &io_handler,
   case eReturnStatusQuit:
     m_quit_requested = true;
     io_handler.SetIsDone(true);
     break;
   }
 
   // Finally, if we're going to stop on crash, check that here:
   if (!m_quit_requested && result.GetDidChangeProcessState() &&
-      io_handler.GetFlags().Test(eHandleCommandFlagStopOnCrash)) {
-    bool should_stop = false;
-    TargetSP target_sp(m_debugger.GetTargetList().GetSelectedTarget());
-    if (target_sp) {
-      ProcessSP process_sp(target_sp->GetProcessSP());
-      if (process_sp) {
-        for (ThreadSP thread_sp : process_sp->GetThreadList().Threads()) {
-          StopReason reason = thread_sp->GetStopReason();
-          if ((reason == eStopReasonSignal || reason == eStopReasonException ||
-               reason == eStopReasonInstrumentation) &&
-              !result.GetAbnormalStopWasExpected()) {
-            should_stop = true;
-            break;
-          }
-        }
-      }
-    }
-    if (should_stop) {
-      io_handler.SetIsDone(true);
-      m_stopped_for_crash = true;
-    }
-  }
-}
+      io_handler.GetFlags().Test(eHandleCommandFlagStopOnCrash) &&
+      DidProcessStopAbnormally()) {
 
 bool CommandInterpreter::IOHandlerInterrupt(IOHandler &io_handler) {
   ExecutionContext exe_ctx(GetExecutionContext());
   Process *process = exe_ctx.GetProcessPtr();
 
   if (InterruptCommand())
     return true;
 

lldb/trunk/source/Interpreter/CommandReturnObject.cpp

if (s.empty()) {
     add_newline = last_char != '\n' && last_char != '\r';
   }
   if (add_newline)
     strm.EOL();
 }
 
 CommandReturnObject::CommandReturnObject()
     : m_out_stream(), m_err_stream(), m_status(eReturnStatusStarted),
-      m_did_change_process_state(false), m_interactive(true),
-      m_abnormal_stop_was_expected(false) {}
+      m_did_change_process_state(false), m_interactive(true) {}
 
 CommandReturnObject::~CommandReturnObject() {}
 
 void CommandReturnObject::AppendErrorWithFormat(const char *format, ...) {
   if (!format)
     return;
   va_list args;
   va_start(args, format);