llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) <details> <summary>Changes</summary> A Python command's print() writes to sys.stdout, which the interpreter session pointed at the debugger's raw terminal descriptor via PyFile_FromFd. Those writes (directly to the fd) bypass the debugger's output locking, potentially racing with the statusline and causing inconsistent/truncated output. Back the session's terminal stdout and stderr with a pipe instead. A read thread drains the pipe and writes to the terminal through Debugger::PrintAsync which correctly locks. Python still gets a real line-buffered text file over the pipe, so output stays live and file objects passed to SB APIs (SetImmediateOutputFile, subprocess, fileno) keep working. On session teardown the wrappers are flushed before the pipe is closed so a retained sys.stdout does not strand a trailing line or write into a closed descriptor. rdar://181841574 --- Full diff: https://github.com/llvm/llvm-project/pull/208609.diff 4 Files Affected: - (modified) lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp (+166) - (modified) lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h (+18-4) - (modified) lldb/test/API/functionalities/statusline/TestStatusline.py (+46) - (added) lldb/test/API/functionalities/statusline/statusline_flood.py (+17) ``````````diff diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp index 3b822f2585c5e..b8057653d5717 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp +++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp @@ -25,6 +25,7 @@ #include "lldb/Core/ThreadedCommunication.h" #include "lldb/DataFormatters/TypeSummary.h" #include "lldb/Host/Config.h" +#include "lldb/Host/ConnectionFileDescriptor.h" #include "lldb/Host/FileSystem.h" #include "lldb/Host/HostInfo.h" #include "lldb/Host/Pipe.h" @@ -46,6 +47,10 @@ #include "llvm/Support/FileSystem.h" #include "llvm/Support/FormatAdapters.h" +#if defined(_WIN32) +#include "lldb/Host/windows/ConnectionGenericFileWindows.h" +#endif + #include <cstdio> #include <cstdlib> #include <memory> @@ -444,6 +449,88 @@ ScriptInterpreterPythonImpl::ScriptInterpreterPythonImpl(Debugger &debugger) RunSimpleString(run_string.GetData()); } +/// A Python sys.stdout/stderr file backed by a pipe whose read end is drained +/// by a reader thread that writes to the debugger's terminal under the output +/// lock (Debugger::PrintAsync). Handing Python the raw terminal descriptor +/// instead lets a script's print() race the statusline, which redraws on the +/// event thread under that lock; its cursor save/restore then rewinds over and +/// eats the script's output. +class ScriptInterpreterPythonImpl::SessionIORedirect { +public: + static std::unique_ptr<SessionIORedirect> Create(lldb::user_id_t debugger_id, + bool is_stdout) { + Pipe pipe; + if (pipe.CreateNew().Fail()) + return nullptr; + + std::unique_ptr<SessionIORedirect> redirect( + new SessionIORedirect(debugger_id, is_stdout)); + +#if defined(_WIN32) + lldb::file_t read_handle = pipe.GetReadNativeHandle(); + pipe.ReleaseReadFileDescriptor(); + std::unique_ptr<Connection> conn = + std::make_unique<ConnectionGenericFile>(read_handle, true); +#else + std::unique_ptr<Connection> conn = + std::make_unique<ConnectionFileDescriptor>( + pipe.ReleaseReadFileDescriptor(), /*owns_fd=*/true); +#endif + if (!conn->IsConnected()) + return nullptr; + + redirect->m_communication.SetConnection(std::move(conn)); + redirect->m_communication.SetReadThreadBytesReceivedCallback( + ReadThreadBytesReceived, redirect.get()); + if (!redirect->m_communication.StartReadThread()) + return nullptr; + redirect->m_connected = true; + + // The write end is owned here; Python only borrows its descriptor. + redirect->m_write_file_sp = std::make_shared<NativeFile>( + pipe.ReleaseWriteFileDescriptor(), File::eOpenOptionWriteOnly, + NativeFile::Owned); + return redirect; + } + + ~SessionIORedirect() { + if (!m_connected) + return; + // Close the write end so the reader sees EOF and exits, then join it. + if (m_write_file_sp) + m_write_file_sp->Close(); + m_communication.JoinReadThread(); + m_communication.Disconnect(); + } + + int GetWriteDescriptor() const { + return m_write_file_sp ? m_write_file_sp->GetDescriptor() + : File::kInvalidDescriptor; + } + +private: + SessionIORedirect(lldb::user_id_t debugger_id, bool is_stdout) + : m_debugger_id(debugger_id), m_is_stdout(is_stdout), + m_communication("lldb.ScriptInterpreterPython.io-redirect") {} + + static void ReadThreadBytesReceived(void *baton, const void *src, + size_t src_len) { + if (!src || !src_len) + return; + auto *self = static_cast<SessionIORedirect *>(baton); + if (lldb::DebuggerSP debugger_sp = + Debugger::FindDebuggerWithID(self->m_debugger_id)) + debugger_sp->PrintAsync(static_cast<const char *>(src), src_len, + self->m_is_stdout); + } + + lldb::user_id_t m_debugger_id; + bool m_is_stdout; + lldb::FileSP m_write_file_sp; + ThreadedCommunication m_communication; + bool m_connected = false; +}; + ScriptInterpreterPythonImpl::~ScriptInterpreterPythonImpl() { // the session dictionary may hold objects with complex state which means // that they may need to be torn down with some level of smarts and that, in @@ -567,6 +654,26 @@ void ScriptInterpreterPythonImpl::LeaveSession() { if (PyThreadState_GetDict()) { PythonDictionary &sys_module_dict = GetSysModuleDictionary(); if (sys_module_dict.IsValid()) { + // Flush the pipe-backed wrappers while they are still sys.stdout/stderr. + // Line buffering already flushes on each newline, but a trailing + // unterminated line would otherwise be stranded (and later flushed into + // a closed descriptor) once we close the pipe write end below. + auto flush_redirect = [&](const char *py_name, + std::unique_ptr<SessionIORedirect> &redirect) { + if (!redirect) + return; + PythonObject file = + sys_module_dict.GetItemForKey(PythonString(py_name)); + if (!file.IsValid()) + return; + if (llvm::Expected<PythonObject> result = file.CallMethod("flush")) + (void)result; + else + llvm::consumeError(result.takeError()); + }; + flush_redirect("stdout", m_stdout_redirect); + flush_redirect("stderr", m_stderr_redirect); + if (m_saved_stdin.IsValid()) { sys_module_dict.SetItemForKey(PythonString("stdin"), m_saved_stdin); m_saved_stdin.Reset(); @@ -582,9 +689,62 @@ void ScriptInterpreterPythonImpl::LeaveSession() { } } + // Tear down the pipe redirects (closes each write end and joins its reader). + // The wrappers were flushed above, so nothing buffered is lost. + m_stdout_redirect.reset(); + m_stderr_redirect.reset(); + m_session_is_active = false; } +bool ScriptInterpreterPythonImpl::RedirectTerminalHandleThroughLock( + const char *py_name, PythonObject &save_file, const char *mode, + File &file) { + const bool is_stdout = ::strcmp(py_name, "stdout") == 0; + if (!is_stdout && ::strcmp(py_name, "stderr") != 0) + return false; + + // Only the debugger's own terminal races the statusline. A redirect to a + // pipe or user file (a different descriptor) is wrapped normally. + lldb::FileSP debugger_file = + is_stdout ? m_debugger.GetOutputFileSP() : m_debugger.GetErrorFileSP(); + int fd = file.GetDescriptor(); + if (!debugger_file || fd == File::kInvalidDescriptor || + fd != debugger_file->GetDescriptor()) + return false; + + std::unique_ptr<SessionIORedirect> &redirect = + is_stdout ? m_stdout_redirect : m_stderr_redirect; + redirect = SessionIORedirect::Create(m_debugger.GetID(), is_stdout); + if (!redirect) + return false; + + // Line-buffer the wrapper so each print() reaches the reader (and the + // terminal) promptly: the pipe descriptor is not a tty, so the default + // buffering would hold output back until the buffer filled. + PyObject *pipe_file = PyFile_FromFd( + redirect->GetWriteDescriptor(), nullptr, mode, /*buffering=*/1, + /*encoding=*/nullptr, /*errors=*/"ignore", /*newline=*/nullptr, + /*closefd=*/0); + if (!pipe_file) { + // Fall back to the raw descriptor. That reopens the statusline race, so + // leave a breadcrumb rather than failing silently. + LLDB_LOG(GetLog(LLDBLog::Script), + "failed to wrap sys.{0} on a synchronized pipe; falling back to " + "the unsynchronized terminal descriptor", + py_name); + PyErr_Clear(); + redirect.reset(); + return false; + } + + PythonObject new_file(PyRefType::Owned, pipe_file); + PythonDictionary &sys_module_dict = GetSysModuleDictionary(); + save_file = sys_module_dict.GetItemForKey(PythonString(py_name)); + sys_module_dict.SetItemForKey(PythonString(py_name), new_file); + return true; +} + bool ScriptInterpreterPythonImpl::SetStdHandle(FileSP file_sp, const char *py_name, PythonObject &save_file, @@ -595,6 +755,12 @@ bool ScriptInterpreterPythonImpl::SetStdHandle(FileSP file_sp, } File &file = *file_sp; + // When stdout/stderr point at the debugger's own terminal, route Python's + // output through a pipe drained under the output lock so a script's print() + // cannot race the statusline. Any other target keeps the normal wrapping. + if (RedirectTerminalHandleThroughLock(py_name, save_file, mode, file)) + return true; + // Flush the file before giving it to python to avoid interleaved output. file.Flush(); diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h index 863cf27785824..29437dad3d7e2 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h @@ -200,7 +200,7 @@ class ScriptInterpreterPythonImpl : public ScriptInterpreterPython { bool GetLongHelpForCommandObject(StructuredData::GenericSP cmd_obj_sp, std::string &dest) override; - + StructuredData::ObjectSP GetOptionsForCommandObject(StructuredData::GenericSP cmd_obj_sp) override; @@ -209,7 +209,7 @@ class ScriptInterpreterPythonImpl : public ScriptInterpreterPython { bool SetOptionValueForCommandObject(StructuredData::GenericSP cmd_obj_sp, ExecutionContext *exe_ctx, - llvm::StringRef long_option, + llvm::StringRef long_option, llvm::StringRef value) override; void OptionParsingStartedForCommandObject( @@ -273,8 +273,7 @@ class ScriptInterpreterPythonImpl : public ScriptInterpreterPython { Status SetBreakpointCommandCallback(BreakpointOptions &bp_options, const char *command_body_text, StructuredData::ObjectSP extra_args_sp, - bool uses_extra_args, - bool is_callback); + bool uses_extra_args, bool is_callback); /// Set a one-liner as the callback for the watchpoint. void SetWatchpointCommandCallback(WatchpointOptions *wp_options, @@ -407,9 +406,24 @@ class ScriptInterpreterPythonImpl : public ScriptInterpreterPython { bool SetStdHandle(lldb::FileSP file, const char *py_name, python::PythonObject &save_file, const char *mode); + /// A Python sys.stdout/stderr file backed by a pipe that a reader thread + /// drains to the debugger's terminal under the output lock, so a script's + /// print() cannot race the statusline. Defined in the implementation file. + class SessionIORedirect; + + /// If \p file is the debugger's own terminal, point sys.\p py_name at a + /// pipe-backed file whose writes are serialized through the output lock (see + /// SessionIORedirect) and return true. Return false to let SetStdHandle wrap + /// \p file normally. + bool RedirectTerminalHandleThroughLock(const char *py_name, + python::PythonObject &save_file, + const char *mode, File &file); + python::PythonObject m_saved_stdin; python::PythonObject m_saved_stdout; python::PythonObject m_saved_stderr; + std::unique_ptr<SessionIORedirect> m_stdout_redirect; + std::unique_ptr<SessionIORedirect> m_stderr_redirect; python::PythonModule m_main_module; python::PythonDictionary m_session_dict; python::PythonDictionary m_sys_module_dict; diff --git a/lldb/test/API/functionalities/statusline/TestStatusline.py b/lldb/test/API/functionalities/statusline/TestStatusline.py index 8891a561419cf..a77e18e9bc6e9 100644 --- a/lldb/test/API/functionalities/statusline/TestStatusline.py +++ b/lldb/test/API/functionalities/statusline/TestStatusline.py @@ -121,6 +121,52 @@ def test_no_target(self): self.expect("set set show-statusline true", ["no target"]) + def test_scripted_command_output_not_eaten(self): + """A scripted command's print() output must be serialized with the + statusline redraw. The statusline brackets its redraw with a cursor + save (ESC 7) and restore (ESC 8); if command output lands in between, + the restore rewinds the cursor back over it and the terminal + overwrites it, so the output is non-deterministically eaten. Assert + that no command output appears inside a save/restore pair.""" + self.launch(use_colors=False) + self.resize() + self.expect("settings set show-statusline true", ["no target"]) + + flood = os.path.join( + os.path.dirname(os.path.realpath(__file__)), "statusline_flood.py" + ) + self.expect('command script import "{}"'.format(flood)) + + tee = CaptureTee() + self.child.logfile_read = tee + self.child.sendline("statusline_flood") + self.child.expect("MARKER_0049") + self.child.expect("(lldb)") + self.child.logfile_read = None + + data = tee.data + # The test is only meaningful if the statusline actually redrew and the + # command actually produced output. + self.assertIn(b"\x1b7", data) + self.assertIn(b"MARKER_0049", data) + + # No command output may appear between a cursor save (ESC 7) and its + # matching restore (ESC 8); that would mean the two writers interleaved. + pos = 0 + while True: + save = data.find(b"\x1b7", pos) + if save == -1: + break + restore = data.find(b"\x1b8", save) + if restore == -1: + break + self.assertNotIn( + b"MARKER_", + data[save + 2 : restore], + "scripted command output was spliced into a statusline redraw", + ) + pos = restore + 2 + @skipIfEditlineSupportMissing def test_resize(self): """Test that move the cursor when resizing.""" diff --git a/lldb/test/API/functionalities/statusline/statusline_flood.py b/lldb/test/API/functionalities/statusline/statusline_flood.py new file mode 100644 index 0000000000000..2ce5af925453f --- /dev/null +++ b/lldb/test/API/functionalities/statusline/statusline_flood.py @@ -0,0 +1,17 @@ +"""Helper for TestStatusline.test_scripted_command_output_not_eaten. + +Registers a command that floods output while emitting progress events, so the +statusline redraws (on the event thread) concurrently with the command output. +""" + +import lldb + + [email protected]("statusline_flood") +def statusline_flood(debugger, command, result, internal_dict): + count = 50 + progress = lldb.SBProgress("flood", "working", count, debugger) + for i in range(count): + print("MARKER_{:04d}".format(i), flush=True) + progress.Increment(1, "step {}".format(i)) + progress.Finalize() `````````` </details> https://github.com/llvm/llvm-project/pull/208609 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
