[lldb/IOHandler] Change the way we manage IO handler

[JDevlieghere]

Summary

The way the IO handlers are currently managed by the debugger is wrong. The implementation lacks proper synchronization between RunIOHandler and ExecuteIOHandlers. The latter is meant to be run by the "main thread", while the former is meant to be run synchronously, potentially from a different thread. Imagine a scenario where RunIOHandler is called from a different thread than ExecuteIOHandlers. Both functions manipulate the debugger's IOHandlerStack. Although the push and pop operations are synchronized, the logic to activate, deactivate and run IO handlers is not.

While investigating https://llvm.org/PR44352, I noticed some weird behavior in the Editline implementation. One of its members (m_editor_status) was modified from another thread. This happened because the main thread, while running ExecuteIOHandlers ended up execution the IOHandlerEditline created by the breakpoint callback thread. Even worse, due to the lack of synchronization within the IO handler implementation, both threads ended up executing the same IO handler.

Given the way the IO handlers work, I don't see the need to have execute them synchronously. When an IO handler is pushed, it will interrupt the current handler unless specified otherwise. One exception where being able to run a handler synchronously is the sourcing of the .lldbinit file in the home and current working directory. This takes place *before* ExecuteIOHandlers is started from RunCommandInterpreter, which means that in the new scheme these two IO handlers end up at the bottom of the stack. To work around this specific problem, I've added an option to run the IO handler synchronously if needed (i.e. ExecuteIOHandlers is not running yet). When that's the case, any other threads are prevented (blocked) from starting to execute the IO handlers. I don't think this workaround is necessary for any other handlers.

I've been starting at this for quite a bit and tried a few different approaches, but it's totally possible that I missed some. I'm more than open to suggestions for more elegant solutions!

[JDevlieghere]

I didn't get a chance to write a test case yet, but you can reproduce the problem with the following setup:

$ echo "script foo = 1" > test.lldb
$ lldb ./a.out
(lldb) b main
(lldb) breakpoint command add -s python
    frame.GetThread().GetProcess().GetTarget().GetDebugger().HandleCommand("command source -s true ./test.lldb")
    DONE
(lldb) run

[JDevlieghere]

Rebase on top of NFC change

[labath]

Though I have messed with IOHandlers in the past, I have successfully suppressed most of the memories of it. I think I have a rough understanding of what the bug is, but I don't understand the solution yet.

With this patch, what does guarantee that the IOHandler for the "command source -s true ./test.lldb" thingy completes before the breakpoint callback is finished (I assume that the intention is for this to be executed synchronously)?

I don't know if this matters, but another detail to be aware of is that the IOHandler stack will be different if driving lldb through python (without calling SBDebugger::RunCommandInterpreter). In this case there won't be a stdio editline handler sitting at the bottom of the stack.

lldb/source/Core/Debugger.cpp

1061

This looks very clever, but it can still be racy if someone calls ExecuteIOHandlers concurrently to the owns_lock check...

A better way to achieve this (if I understand correctly what you are trying to achieve) would be to have a ExecuteIOHandlersIfNeeded function which does something like

std::unique_lock lock(m_synchronous_reader_mutex, std::try_lock);
if (lock)
  ReallyExecuteIOHandlers(); // No locking in here

[clayborg]

So I did a bunch of original IOHandler. And there are many complex cases for sure. One thing to be aware of is that if you won't use editline() and we call fgets() in the default implementation, there is no way to cancel this IIRC. So it might be worth trying this without editline support to make sure things don't deadlock. If the test suite is happy, then this looks worth trying, though with all the complexities I don't think we can guarantee this doesn't cause issues in some unexpected way. The main things that worry me are:

• REPL issues since the REPL and (lldb) prompt switch between themselves in a tricky way where they swap IOHandlers
• running from python script directly when no IOHandlers are pushed because no command interpreter is being run and all the fallout from the cases (HandleCommand that results in a python breakpoint callback etc)
• the process IO handler that does STDIO for a process
• when no editline, we use fgets() that can't be canceled

[jingham]

In D72748#1822443, @clayborg wrote:

So I did a bunch of original IOHandler. And there are many complex cases for sure. One thing to be aware of is that if you won't use editline() and we call fgets() in the default implementation, there is no way to cancel this IIRC. So it might be worth trying this without editline support to make sure things don't deadlock. If the test suite is happy, then this looks worth trying, though with all the complexities I don't think we can guarantee this doesn't cause issues in some unexpected way. The main things that worry me are:

• REPL issues since the REPL and (lldb) prompt switch between themselves in a tricky way where they swap IOHandlers
• running from python script directly when no IOHandlers are pushed because no command interpreter is being run and all the fallout from the cases (HandleCommand that results in a python breakpoint callback etc)
• the process IO handler that does STDIO for a process
• when no editline, we use fgets() that can't be canceled

The fgets part is problematic in other ways. For instance, Python based commands in stop hooks work in command line lldb, but in Xcode which doesn't use editline, when we go to fflush the I/O channel on switching to the Python interpreter, fflush deadlocks against the lock held by fgets. I wonder if we should just stop using fgets and use select everywhere we can, as it doesn't have this problem.

[JDevlieghere]

I'm working on a different approach that should address al the concerns raised so far.

[JDevlieghere]

Synchronize between RunIOHandler and ExecuteIOHandlers using a mutex. When running an IO handler synchronously, block ExecuteIOHandlers until we're finished.

[labath]

I didn't actually try it but I am pretty sure this will deadlock with nested lldb command files (running command source from a file that is itself being sourced). Changing the mutex to a recursive_mutex would fix that, but I don't believe it would make this fully correct -- it would just make it harder to demonstrate that it's wrong. OTOH, that may be the best thing we can do in the current state of affairs.

The thing I don't understand now is why do we even need this stack in the first place. It seems like this could be handled by just running a new iohandler "main loop" instead of pushing something. Take the "expr" command for example. In the single-line mode it evaluates the expression synchronously, but in a multi-line expression, it returns immediately after pushing it's own IOHandler (which then gathers the expression and calls back into the command to run it). I don't see why we couldn't achieve the same thing by "running" the iohandler directly, instead of pushing it to some stack and waiting for it to be executed at the top level. The same thing could be said for the "script" command and various other things which "hijack" the main (lldb) iohandler.

Greg, am I missing something?

lldb/include/lldb/Core/Debugger.h

413

typo

[JDevlieghere]

Use recursive mutex

[labath]

Not super ideal, but not too bad either. Not clicking accept yet because of the missing test case...

[JDevlieghere]

Add PExpect test

[labath]

Let's give this a shot. I'm don't think this is fully right, but I also don't know how to improve that, exactly...

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/TestBreakpointCallbackCommandSource.py

16

Btw, all pexpect tests get this automatically from the base class.

41–43

There's a ptrace version of self.expect to automate these things for you. It probably won't work for the multiline "breakpoint command add" command, but the rest could be something like self.expect("script print(foo)", substrs=["95126"])

[JDevlieghere]

In D72748#1823945, @labath wrote:

I didn't actually try it but I am pretty sure this will deadlock with nested lldb command files (running command source from a file that is itself being sourced). Changing the mutex to a recursive_mutex would fix that, but I don't believe it would make this fully correct -- it would just make it harder to demonstrate that it's wrong. OTOH, that may be the best thing we can do in the current state of affairs.

The thing I don't understand now is why do we even need this stack in the first place. It seems like this could be handled by just running a new iohandler "main loop" instead of pushing something. Take the "expr" command for example. In the single-line mode it evaluates the expression synchronously, but in a multi-line expression, it returns immediately after pushing it's own IOHandler (which then gathers the expression and calls back into the command to run it). I don't see why we couldn't achieve the same thing by "running" the iohandler directly, instead of pushing it to some stack and waiting for it to be executed at the top level. The same thing could be said for the "script" command and various other things which "hijack" the main (lldb) iohandler.

Isn't the problem that you can't be sure your IO handler pushes another one on top of the stack? I considered an alternative implementation, where the synchronous IO handlers has its own stack and everything that's pushed while it is executing ends up on that stack. It adds a lot of complexity and you still need to synchronize with the "main loop"

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/TestBreakpointCallbackCommandSource.py

41–43

Cool, I didn't know, I'll refactor that before landing

[labath]

In D72748#1829956, @JDevlieghere wrote:

In D72748#1823945, @labath wrote:

I didn't actually try it but I am pretty sure this will deadlock with nested lldb command files (running command source from a file that is itself being sourced). Changing the mutex to a recursive_mutex would fix that, but I don't believe it would make this fully correct -- it would just make it harder to demonstrate that it's wrong. OTOH, that may be the best thing we can do in the current state of affairs.

The thing I don't understand now is why do we even need this stack in the first place. It seems like this could be handled by just running a new iohandler "main loop" instead of pushing something. Take the "expr" command for example. In the single-line mode it evaluates the expression synchronously, but in a multi-line expression, it returns immediately after pushing it's own IOHandler (which then gathers the expression and calls back into the command to run it). I don't see why we couldn't achieve the same thing by "running" the iohandler directly, instead of pushing it to some stack and waiting for it to be executed at the top level. The same thing could be said for the "script" command and various other things which "hijack" the main (lldb) iohandler.

Isn't the problem that you can't be sure your IO handler pushes another one on top of the stack? I considered an alternative implementation, where the synchronous IO handlers has its own stack and everything that's pushed while it is executing ends up on that stack. It adds a lot of complexity and you still need to synchronize with the "main loop"

Well.. in the way I as imagining things, there would be no stacks (at least no explicit stacks), no pushing, and everything would execute in the "synchronous" mode. So e.g., when we start up the main "(lldb)" loop, we just take that iohandler, and run it until it says it's done. If the user types "script", then we start another "main loop" with the python iohandler, but the main loop can be completely oblivious to that -- as far as it is concerned, its iohandler is still executing CommandObjectScript::DoExecute. When the user exits the python prompt the control returns to the (lldb) iohandler just like it would after any other "simple" command. So, essentially, there's still some stacking involved, but it's not managed explicitly -- it just comes out from the way the code is organized. Similarly, the "breakpoint command add" could run an iohandler to collect the breakpoint commands, "process continue" could run a loop to forward the inferior stdio, etc.

(The last bit is tricky because of ^C, and it means that we will still need to have some global notion of the "active" or "top" iohandler, which is the one that receives ^Cs, but still, I think that it should be possible to run everything "synchronously" and I hope that would get us rid of a lot of complexity.)

[clayborg]

In D72748#1830638, @labath wrote:

In D72748#1829956, @JDevlieghere wrote:

In D72748#1823945, @labath wrote:

I didn't actually try it but I am pretty sure this will deadlock with nested lldb command files (running command source from a file that is itself being sourced). Changing the mutex to a recursive_mutex would fix that, but I don't believe it would make this fully correct -- it would just make it harder to demonstrate that it's wrong. OTOH, that may be the best thing we can do in the current state of affairs.

The thing I don't understand now is why do we even need this stack in the first place. It seems like this could be handled by just running a new iohandler "main loop" instead of pushing something. Take the "expr" command for example. In the single-line mode it evaluates the expression synchronously, but in a multi-line expression, it returns immediately after pushing it's own IOHandler (which then gathers the expression and calls back into the command to run it). I don't see why we couldn't achieve the same thing by "running" the iohandler directly, instead of pushing it to some stack and waiting for it to be executed at the top level. The same thing could be said for the "script" command and various other things which "hijack" the main (lldb) iohandler.

Isn't the problem that you can't be sure your IO handler pushes another one on top of the stack? I considered an alternative implementation, where the synchronous IO handlers has its own stack and everything that's pushed while it is executing ends up on that stack. It adds a lot of complexity and you still need to synchronize with the "main loop"

Well.. in the way I as imagining things, there would be no stacks (at least no explicit stacks), no pushing, and everything would execute in the "synchronous" mode. So e.g., when we start up the main "(lldb)" loop, we just take that iohandler, and run it until it says it's done. If the user types "script", then we start another "main loop" with the python iohandler, but the main loop can be completely oblivious to that -- as far as it is concerned, its iohandler is still executing CommandObjectScript::DoExecute. When the user exits the python prompt the control returns to the (lldb) iohandler just like it would after any other "simple" command. So, essentially, there's still some stacking involved, but it's not managed explicitly -- it just comes out from the way the code is organized. Similarly, the "breakpoint command add" could run an iohandler to collect the breakpoint commands, "process continue" could run a loop to forward the inferior stdio, etc.

(The last bit is tricky because of ^C, and it means that we will still need to have some global notion of the "active" or "top" iohandler, which is the one that receives ^Cs, but still, I think that it should be possible to run everything "synchronously" and I hope that would get us rid of a lot of complexity.)

The pushing and popping is done because of how the command interpreter works. Think about typing:

(lldb) script

We are in the IOHandler for the command interpreter for LLDB commands and running the code for the "script" command in CommandObjectScript::DoExecute(...), and now we need to switch to the python interpreter.

Before doing this we might need to remove the "(lldb)" prompt from the screen if had already been redisplayed. The idea was while in CommandObjectScript::DoExecute(...) we can push a new IOHandler for python and let the current function exit. There might be some locks preventing multiple commands from executing at the same time, so best to avoid this if this is the case. Then when we return from CommandObjectScript::DoExecute(...) we can let the IOHandler stack do what it needs to do and switch to the python interpreter.

This flow also allows the "gui" command to do any curses setup and teardown at the right times as the IOHandler is pushed and popped.

When the previous IOHandler becomes active again, it can resume where it left off, like curses can clear the screen and re-display anything it needs to. So the stack does serve a purpose, but I am sure this can be done in another way and happy to try a new strategy.

One complexity you won't see here is how the REPL and LLDB prompt can switch back and forth. From the Swift REPL, you can drop into the lldb prompt and back, and for that we have some special handling that allows the two to switch without creating a whole new IOHandler each time they switch.

Also think about a file that is sourced with "command source" and where it it contains:

target stop-hook add
bt
fr variable
DONE
image list

The current IOHandler will be working with STDIN from the debugger, and then we execute "command source /tmp/foo" which will create a new IOHandlerEditline with the file handle for "/tmp/foo" as its input and STDOUT from the debugger as its output and pushed onto the stack. The IOHandler will grab one line, "target stop-hook add" and then push another for the target stop hook to get its commands. This will also use the top IOHandler's file for "/tmp/foo" as input and STDOUT for output, grab its stuff and exit. If the user forgot to add the "DONE" in the list, and the IOHandler just returned and resumed with the old STDIN as input we would be in an intermediate state still waiting for this input, but with the IOHandler stack it would finish up because we would reach EOF on the "/tmp/foo" input.

As each IOHandler is being pushed, it can adopt a different source (STDIN of the debugger object by default, but might be a file when we are using "command source"). So the pushing and popping allow a new IOHandler to be queued up during the CommandObject::DoExecute() function (like a new IOHandler that gets its input from the file from "command source") and as soon as this IOHandler object on the stack is out of input, it can pop itself off the stack. So the stack does provide a nice clean way to do theses kinds of things and the stack allows us to go the the current IOHandler and tell it it is about to not be the top IOHandler and allows it to do setup/teardown etc.

I think there are some complexities for python code where this doesn't work right for things like:

script
for i in range(0..1):
  print(i)
quit()
image list

But it would be great if it did work with any new solution.

So I just wanted to give some examples of the complex things the stack does for us right now so any modifications can keep this in mind.

lldb/include/lldb/Core/Debugger.h

SourceManager::SourceFileCache m_source_file_cache; // All the source managers
                                                       // source file cache.
   std::unique_ptr<CommandInterpreter> m_command_interpreter_up;
 
   std::recursive_mutex m_script_interpreter_mutex;
   std::array<lldb::ScriptInterpreterSP, lldb::eScriptLanguageUnknown>
       m_script_interpreters;
 
   IOHandlerStack m_io_handler_stack;
+  std::recursive_mutex m_io_handler_synchronous_mutex;
+
   llvm::StringMap<std::weak_ptr<llvm::raw_ostream>> m_log_streams;
   std::shared_ptr<llvm::raw_ostream> m_log_callback_stream_sp;
   ConstString m_instance_name;
   static LoadPluginCallbackType g_load_plugin_callback;
   typedef std::vector<llvm::sys::DynamicLibrary> LoadedPluginsList;
   LoadedPluginsList m_loaded_plugins;
   HostThread m_event_handler_thread;
   HostThread m_io_handler_thread;

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/Makefile

+C_SOURCES := main.c
+
+include Makefile.rules

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/TestBreakpointCallbackCommandSource.py

+"""
+Test completion in our IOHandlers.
+"""
+
+import os
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.lldbpexpect import PExpectTest
+
+
+class BreakpointCallbackCommandSource(PExpectTest):
+
+    mydir = TestBase.compute_mydir(__file__)
+    file_to_source = os.path.join(os.path.abspath(os.path.dirname(__file__)), 'source.lldb')
+
+    # PExpect uses many timeouts internally and doesn't play well
+    # under ASAN on a loaded machine..
+    @skipIfAsan
+    @skipIfEditlineSupportMissing
+    def test_breakpoint_callback_command_source(self):
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+
+        self.launch(exe)
+        self.expect("b main", substrs=["Breakpoint 1"])
+        self.child.send("breakpoint command add -s python\n")
+        self.child.send(
+            "frame.GetThread().GetProcess().GetTarget().GetDebugger().HandleCommand('command source -s true {}')\n"
+            .format(self.file_to_source))
+        self.child.send("DONE\n")
+        self.expect_prompt()
+        self.expect("run", substrs=["Process", "stopped"])
+        self.expect("script print(foo)", substrs=["95126"])

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/main.c

+int main (int argc, char const *argv[])
+{
+  return 0;
+}

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/source.lldb

+script foo = 95126

lldb/source/Core/Debugger.cpp

void Debugger::ClearIOHandlers() {
   while (m_io_handler_stack.GetSize() > 1) {
     IOHandlerSP reader_sp(m_io_handler_stack.Top());
     if (reader_sp)
       PopIOHandler(reader_sp);
   }
 }
 
 void Debugger::RunIOHandlers() {
+  IOHandlerSP reader_sp = m_io_handler_stack.Top();
   while (true) {
-    IOHandlerSP reader_sp(m_io_handler_stack.Top());
     if (!reader_sp)
       break;
 
     reader_sp->Run();
+    {
+      std::lock_guard<std::recursive_mutex> guard(
+          m_io_handler_synchronous_mutex);
 
       // Remove all input readers that are done from the top of the stack
       while (true) {
         IOHandlerSP top_reader_sp = m_io_handler_stack.Top();
         if (top_reader_sp && top_reader_sp->GetIsDone())
           PopIOHandler(top_reader_sp);
         else
           break;
       }
+      reader_sp = m_io_handler_stack.Top();
+    }
   }
   ClearIOHandlers();
 }
 
+void Debugger::RunIOHandlerSync(const IOHandlerSP &reader_sp) {
+  std::lock_guard<std::recursive_mutex> guard(m_io_handler_synchronous_mutex);
+
+  PushIOHandler(reader_sp);
+  IOHandlerSP top_reader_sp = reader_sp;
+
+  while (top_reader_sp) {
+    if (!top_reader_sp)
+      break;
+
+    top_reader_sp->Run();
+
+    // Don't unwind past the starting point.
+    if (top_reader_sp.get() == reader_sp.get()) {
+      if (PopIOHandler(reader_sp))
+        break;
+    }
+
+    // If we pushed new IO handlers, pop them if they're done or restart the
+    // loop to run them if they're not.
+    while (true) {
+      top_reader_sp = m_io_handler_stack.Top();
+      if (top_reader_sp && top_reader_sp->GetIsDone()) {
+        PopIOHandler(top_reader_sp);
+        // Don't unwind past the starting point.
+        if (top_reader_sp.get() == reader_sp.get())
+          return;
+      } else {
+        break;
+      }
+    }
+  }
+}
+
 bool Debugger::IsTopIOHandler(const lldb::IOHandlerSP &reader_sp) {
   return m_io_handler_stack.IsTop(reader_sp);
 }
 
 bool Debugger::CheckTopIOHandlerTypes(IOHandler::Type top_type,
                                       IOHandler::Type second_top_type) {
   return m_io_handler_stack.CheckTopIOHandlerTypes(top_type, second_top_type);
 }
bool Debugger::RemoveIOHandler(const IOHandlerSP &reader_sp) {
   return PopIOHandler(reader_sp);
 }
 
 void Debugger::RunIOHandlerAsync(const IOHandlerSP &reader_sp,
                                  bool cancel_top_handler) {
   PushIOHandler(reader_sp, cancel_top_handler);
 }
 
-void Debugger::RunIOHandlerSync(const IOHandlerSP &reader_sp) {
-  PushIOHandler(reader_sp);
-
-  IOHandlerSP top_reader_sp = reader_sp;
-  while (top_reader_sp) {
-    top_reader_sp->Run();
-
-    if (top_reader_sp.get() == reader_sp.get()) {
-      if (PopIOHandler(reader_sp))
-        break;
-    }
-
-    while (true) {
-      top_reader_sp = m_io_handler_stack.Top();
-      if (top_reader_sp && top_reader_sp->GetIsDone())
-        PopIOHandler(top_reader_sp);
-      else
-        break;
-    }
-  }
-}
-
 void Debugger::AdoptTopIOHandlerFilesIfInvalid(FileSP &in, StreamFileSP &out,
                                                StreamFileSP &err) {
   // Before an IOHandler runs, it must have in/out/err streams. This function
   // is called when one ore more of the streams are nullptr. We use the top
   // input reader's in/out/err streams, or fall back to the debugger file
   // handles, or we fall back onto stdin/stdout/stderr as a last resort.
 
   std::lock_guard<std::recursive_mutex> guard(m_io_handler_stack.GetMutex());
void Debugger::PushIOHandler(const IOHandlerSP &reader_sp,
   if (top_reader_sp) {
     top_reader_sp->Deactivate();
     if (cancel_top_handler)
       top_reader_sp->Cancel();
   }
 }
 
 bool Debugger::PopIOHandler(const IOHandlerSP &pop_reader_sp) {
   if (!pop_reader_sp)
     return false;
 
   std::lock_guard<std::recursive_mutex> guard(m_io_handler_stack.GetMutex());
 
   // The reader on the stop of the stack is done, so let the next read on the
   // stack refresh its prompt and if there is one...
   if (m_io_handler_stack.IsEmpty())
     return false;