Mark the execution of commands in stop hooks as non-interactive

[jingham]

Summary

stop-hooks were not intended to be interactive, that would be really confusing when there are many in flight. So this patch marks the results as non-interactive.

Beyond correctness, then main reason for doing this patch is that when you go to run a command based stop hook that includes a Python command, if the command is marked as interactive, Python would try to act on stdin (fflush it) when switching to the Python Session, and if stdin was backed by an lldb NativeFile, the the fflush would deadlock against the I/O handler thread that was sitting in read on the same file handle.

That is a more general problem that this patch does not address. The only place it has shown up IRL is in this scenario, and since doing the right thing for the stop-hook execution works around it, the more general fix can wait on somebody else's free time. But I did add a test that will deadlock without this patch, and succeeds with it.

lldb/packages/Python/lldbsuite/test/lldbtest.py

def generateSource(self, source):
         if self.hasDarwinFramework():
             include_stmt = "'#include <%s>' % os.path.join('LLDB', header)"
         else:
             include_stmt = "'#include <%s>' % os.path.join('" + public_api_dir + "', header)"
         list = [eval(include_stmt) for header in public_headers if (
             header.startswith("SB") and header.endswith(".h"))]
         includes = '\n'.join(list)
         new_content = content.replace('%include_SB_APIs%', includes)
+        new_content = new_content.replace('%SOURCE_DIR%', self.getSourceDir())
         src = os.path.join(self.getBuildDir(), source)
         with open(src, 'w') as f:
             f.write(new_content)
 
         self.addTearDownHook(lambda: os.remove(src))
 
     def setUp(self):
         # Works with the test driver to conditionally skip tests via

lldb/source/Interpreter/CommandInterpreter.cpp

for (size_t idx = 0; idx < num_lines && !WasInterrupted(); idx++) {
 
     if (options.GetEchoCommands()) {
       // TODO: Add Stream support.
       result.AppendMessageWithFormat("%s %s\n",
                                      m_debugger.GetPrompt().str().c_str(), cmd);
     }
 
     CommandReturnObject tmp_result(m_debugger.GetUseColor());
+    tmp_result.SetInteractive(result.GetInteractive());
+
     // If override_context is not NULL, pass no_context_switching = true for
     // HandleCommand() since we updated our context already.
 
     // We might call into a regex or alias command, in which case the
     // add_to_history will get lost.  This m_command_source_depth dingus is the
     // way we turn off adding to the history in that case, so set it up here.
     if (!options.GetAddToHistory())
       m_command_source_depth++;

lldb/source/Target/Target.cpp

Target::StopHookCommandLine::HandleStop(ExecutionContext &exc_ctx,
   assert(exc_ctx.GetTargetPtr() && "Can't call PerformAction on a context "
                                    "with no target");
 
   if (!m_commands.GetSize())
     return StopHookResult::KeepStopped;
 
   CommandReturnObject result(false);
   result.SetImmediateOutputStream(output_sp);
+  result.SetInteractive(false);
   Debugger &debugger = exc_ctx.GetTargetPtr()->GetDebugger();
   CommandInterpreterRunOptions options;
   options.SetStopOnContinue(true);
   options.SetStopOnError(true);
   options.SetEchoCommands(false);
   options.SetPrintResults(true);
   options.SetPrintErrors(true);
   options.SetAddToHistory(false);

lldb/test/API/api/multithreaded/TestMultithreaded.py

class SBBreakpointCallbackCase(TestBase):
     def setUp(self):
         TestBase.setUp(self)
         self.generateSource('driver.cpp')
         self.generateSource('listener_test.cpp')
         self.generateSource('test_breakpoint_callback.cpp')
         self.generateSource('test_listener_event_description.cpp')
         self.generateSource('test_listener_event_process_state.cpp')
         self.generateSource('test_listener_resume.cpp')
+        self.generateSource('test_stop-hook.cpp')
 
     mydir = TestBase.compute_mydir(__file__)
 
     @skipIfRemote
     @skipIfNoSBHeaders
     # clang-cl does not support throw or catch (llvm.org/pr24538)
     @skipIfWindows
+    def test_python_stop_hook(self):
+        """Test that you can run a python command in a stop-hook when stdin is File based. """
+        self.build_and_test('driver.cpp test_stop-hook.cpp',
+                            'test_python_stop_hook')
+
+    @skipIfRemote
+    @skipIfNoSBHeaders
+    # clang-cl does not support throw or catch (llvm.org/pr24538)
+    @skipIfWindows
     def test_breakpoint_callback(self):
         """Test the that SBBreakpoint callback is invoked when a breakpoint is hit. """
         self.build_and_test('driver.cpp test_breakpoint_callback.cpp',
                             'test_breakpoint_callback')
 
     @skipIfRemote
     @skipIfNoSBHeaders
     # clang-cl does not support throw or catch (llvm.org/pr24538)

lldb/test/API/api/multithreaded/some_cmd.py

+""" Test command for checking the Python commands can run in a stop-hook """
+import lldb
+
+did_run = False
+
+class SomeCommand:
+    def __init__(self, debugger, unused):
+        self.dbg = debugger
+    def __call__(self, debugger, command, exe_ctx, result):
+        global did_run
+        did_run = True
+        result.PutCString("some output\n")
+
+    def get_short_help(self):
+        return "Test command - sets a variable."
+
+class OtherCommand:
+    def __init__(self, debugger, unused):
+        self.dbg = debugger
+    def __call__(self, debugger, command, exe_ctx, result):
+        global did_run
+        if did_run:
+            result.SetStatus(lldb.eReturnStatusSuccessFinishNoResult)
+        else:
+            result.SetStatus(lldb.eReturnStatusFailed)
+
+    def get_short_help(self):
+        return "Test command - sets a variable."
+
+def __lldb_init_module(debugger, unused):
+    print("Adding command some-cmd and report-cmd")
+    debugger.HandleCommand("command script add -c some_cmd.SomeCommand some-cmd")
+    debugger.HandleCommand("command script add -c some_cmd.OtherCommand report-cmd")

lldb/test/API/api/multithreaded/test_stop-hook.cpp.template

+// LLDB C++ API Test: Verify that when the Debugger stdin
+// is set to a FILE *, lldb can still successfully run a
+// python command in a stop hook.
+
+#include <errno.h>
+#include <mutex>
+#include <stdio.h>
+#include <string>
+#include <vector>
+
+%include_SB_APIs%
+
+#include "common.h"
+
+using namespace lldb;
+
+void test(SBDebugger &dbg, std::vector<std::string> args) {
+  // The problem we had was that when the thread that was
+  // waiting on input went into the call to 'read' it had
+  // the file handle lock.  Then when the python interpreter
+  // Initialized itself to run the python command, it tried
+  // to flush the file channel, and that deadlocked.
+  // This only happens when Async is true, since otherwise
+  // the process event is handled on the I/O read thread,
+  // which sidestepped the problem.
+  dbg.SetAsync(true);
+
+  SBTarget target = dbg.CreateTarget(args.at(0).c_str());
+  if (!target.IsValid())
+    throw Exception("invalid target");
+
+  SBBreakpoint breakpoint = target.BreakpointCreateByName("next");
+  if (!breakpoint.IsValid())
+    throw Exception("invalid breakpoint");
+
+  SBCommandInterpreter interp = dbg.GetCommandInterpreter();
+  SBCommandReturnObject result;
+
+  // Bring in the python command. We actually add two commands,
+  // one that runs in the stop hook and sets a variable when it
+  // runs, and one that reports out the variable so we can ensure
+  // that we did indeed run the stop hook.
+  const char *source_dir = "%SOURCE_DIR%";
+  SBFileSpec script_spec(source_dir);
+  script_spec.AppendPathComponent("some_cmd.py");
+  char path[PATH_MAX];
+  script_spec.GetPath(path, PATH_MAX);
+
+  std::string import_command("command script import ");
+  import_command.append(path);
+  interp.HandleCommand(import_command.c_str(), result);
+  if (!result.Succeeded())
+    throw Exception("Couldn't import %SOURCE_DIR%/some_cmd.py");
+
+  SBProcess process = target.LaunchSimple(nullptr, nullptr, nullptr);
+  if (!process.IsValid())
+    throw Exception("Couldn't launch process.");
+  if (process.GetState() != lldb::eStateStopped)
+    throw Exception("Process was not stopped");
+
+  process.SetSelectedThreadByIndexID(0);
+
+  // Now add the stop hook:
+  interp.HandleCommand("target stop-hook add -o some-cmd", result);
+  if (!result.Succeeded())
+    throw Exception("Couldn't add a stop hook.");
+
+  // Now switch the I/O over to a pipe, which will be handled by the
+  // NativeFile class:
+  int to_lldb_des[2];
+  int pipe_result = pipe(to_lldb_des);
+  FILE *fh_lldb_in = fdopen(to_lldb_des[0], "r");
+  FILE *fh_to_lldb = fdopen(to_lldb_des[1], "w");
+
+  // We need to reset the handle before destroying the debugger
+  // or the same deadlock will stall exiting:
+  class Cleanup {
+  public:
+    Cleanup(SBDebugger dbg, int filedes[2]) : m_dbg(dbg) {
+      m_file = m_dbg.GetInputFileHandle();
+      m_filedes[0] = filedes[0];
+      m_filedes[1] = filedes[1];
+    }
+    ~Cleanup() {
+      m_dbg.SetInputFileHandle(m_file, false);
+      close(m_filedes[0]);
+      close(m_filedes[1]);
+    }
+
+  private:
+    FILE *m_file;
+    SBDebugger m_dbg;
+    int m_filedes[2];
+  };
+  Cleanup cleanup(dbg, to_lldb_des);
+
+  dbg.SetInputFileHandle(fh_lldb_in, false);
+
+  // Now run the command interpreter.  You have to pass true to
+  // start thread so we will run the I/O in a separate thread.
+  dbg.RunCommandInterpreter(false, true);
+
+  // Now issue a stepi, and fetch the running and stopped events:
+  fprintf(fh_to_lldb, "thread step-inst\n");
+
+  SBEvent proc_event;
+  StateType state;
+  bool got_event;
+
+  got_event = dbg.GetListener().WaitForEventForBroadcaster(
+      100, process.GetBroadcaster(), proc_event);
+  if (!got_event)
+    throw Exception("Didn't get running event");
+  state = SBProcess::GetStateFromEvent(proc_event);
+  if (state != eStateRunning)
+    throw Exception("Event wasn't a running event.");
+
+  got_event = dbg.GetListener().WaitForEventForBroadcaster(
+      100, process.GetBroadcaster(), proc_event);
+  if (!got_event)
+    throw Exception("Didn't get a stopped event");
+  state = SBProcess::GetStateFromEvent(proc_event);
+  if (state != eStateStopped)
+    throw Exception("Event wasn't a stop event.");
+
+  // At this point the stop hook should have run.  Check that:
+  interp.HandleCommand("report-cmd", result);
+  if (!result.Succeeded())
+    throw Exception("Didn't actually call stop hook.");
+}