https://github.com/charles-zablit updated https://github.com/llvm/llvm-project/pull/182302
>From 00adde7afb629ebfd602264c34cc0c0a5047b552 Mon Sep 17 00:00:00 2001 From: Charles Zablit <[email protected]> Date: Thu, 19 Feb 2026 15:30:28 +0000 Subject: [PATCH 1/2] [lldb][windows] fix a race condition when closing the ConPTY --- lldb/include/lldb/Host/ProcessLaunchInfo.h | 2 ++ .../Host/windows/ConnectionGenericFileWindows.h | 7 ++++++- lldb/include/lldb/Host/windows/PseudoConsole.h | 4 ++++ lldb/source/Host/common/ProcessLaunchInfo.cpp | 11 +++++++++-- .../Host/windows/ConnectionGenericFileWindows.cpp | 13 +++++++++++-- lldb/source/Host/windows/ProcessLauncherWindows.cpp | 2 +- lldb/source/Host/windows/PseudoConsole.cpp | 1 + .../Process/Windows/Common/ProcessWindows.cpp | 11 ++++++----- 8 files changed, 40 insertions(+), 11 deletions(-) diff --git a/lldb/include/lldb/Host/ProcessLaunchInfo.h b/lldb/include/lldb/Host/ProcessLaunchInfo.h index e13eecc9463ea..7801662b244ad 100644 --- a/lldb/include/lldb/Host/ProcessLaunchInfo.h +++ b/lldb/include/lldb/Host/ProcessLaunchInfo.h @@ -136,6 +136,8 @@ class ProcessLaunchInfo : public ProcessInfo { /// always true on non Windows. bool ShouldUsePTY() const { #ifdef _WIN32 + if (!m_pty) + return false; return GetPTY().GetPseudoTerminalHandle() != ((HANDLE)(long long)-1) && GetNumFileActions() == 0; #else diff --git a/lldb/include/lldb/Host/windows/ConnectionGenericFileWindows.h b/lldb/include/lldb/Host/windows/ConnectionGenericFileWindows.h index d8f06a7162b22..6d7be7db6d3ae 100644 --- a/lldb/include/lldb/Host/windows/ConnectionGenericFileWindows.h +++ b/lldb/include/lldb/Host/windows/ConnectionGenericFileWindows.h @@ -9,9 +9,11 @@ #ifndef liblldb_Host_windows_ConnectionGenericFileWindows_h_ #define liblldb_Host_windows_ConnectionGenericFileWindows_h_ +#include "lldb/Host/windows/PseudoConsole.h" #include "lldb/Host/windows/windows.h" #include "lldb/Utility/Connection.h" #include "lldb/lldb-types.h" +#include <mutex> namespace lldb_private { @@ -23,6 +25,8 @@ class ConnectionGenericFile : public lldb_private::Connection { ConnectionGenericFile(lldb::file_t file, bool owns_file); + ConnectionGenericFile(PseudoConsole &pty); + ~ConnectionGenericFile() override; bool IsConnected() const override; @@ -43,9 +47,10 @@ class ConnectionGenericFile : public lldb_private::Connection { protected: OVERLAPPED m_overlapped; + std::mutex *m_mutex = nullptr; HANDLE m_file; HANDLE m_event_handles[2]; - bool m_owns_file; + bool m_owns_file = false; LARGE_INTEGER m_file_position; enum { kBytesAvailableEvent, kInterruptEvent }; diff --git a/lldb/include/lldb/Host/windows/PseudoConsole.h b/lldb/include/lldb/Host/windows/PseudoConsole.h index 996faf434585d..f5317d0e97b19 100644 --- a/lldb/include/lldb/Host/windows/PseudoConsole.h +++ b/lldb/include/lldb/Host/windows/PseudoConsole.h @@ -10,6 +10,7 @@ #define LIBLLDB_HOST_WINDOWS_PSEUDOCONSOLE_H_ #include "llvm/Support/Error.h" +#include <mutex> #include <string> #define PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE 0x20016 @@ -34,6 +35,8 @@ class PseudoConsole { /// Close the ConPTY, its read/write handles and invalidate them. void Close(); + std::mutex *GetMutex() { return &m_mutex; }; + /// The ConPTY HPCON handle accessor. /// /// This object retains ownership of the HPCON when this accessor is used. @@ -75,6 +78,7 @@ class PseudoConsole { HANDLE m_conpty_handle = ((HANDLE)(long long)-1); HANDLE m_conpty_output = ((HANDLE)(long long)-1); HANDLE m_conpty_input = ((HANDLE)(long long)-1); + std::mutex m_mutex = {}; }; } // namespace lldb_private diff --git a/lldb/source/Host/common/ProcessLaunchInfo.cpp b/lldb/source/Host/common/ProcessLaunchInfo.cpp index ea036d45e7c80..2f67a417996ac 100644 --- a/lldb/source/Host/common/ProcessLaunchInfo.cpp +++ b/lldb/source/Host/common/ProcessLaunchInfo.cpp @@ -33,7 +33,11 @@ using namespace lldb_private; ProcessLaunchInfo::ProcessLaunchInfo() : ProcessInfo(), m_working_dir(), m_plugin_name(), m_flags(0), - m_file_actions(), m_pty(new PTY), m_monitor_callback(nullptr) {} + m_file_actions(), m_monitor_callback(nullptr) { +#ifndef _WIN32 + m_pty = std::make_shared<PTY>(); +#endif +} ProcessLaunchInfo::ProcessLaunchInfo(const FileSpec &stdin_file_spec, const FileSpec &stdout_file_spec, @@ -41,7 +45,10 @@ ProcessLaunchInfo::ProcessLaunchInfo(const FileSpec &stdin_file_spec, const FileSpec &working_directory, uint32_t launch_flags) : ProcessInfo(), m_working_dir(), m_plugin_name(), m_flags(launch_flags), - m_file_actions(), m_pty(new PTY) { + m_file_actions() { +#ifndef _WIN32 + m_pty = std::make_shared<PTY>(); +#endif if (stdin_file_spec) { FileAction file_action; const bool read = true; diff --git a/lldb/source/Host/windows/ConnectionGenericFileWindows.cpp b/lldb/source/Host/windows/ConnectionGenericFileWindows.cpp index 6a6153d6e34a0..c4b7a63b60ac8 100644 --- a/lldb/source/Host/windows/ConnectionGenericFileWindows.cpp +++ b/lldb/source/Host/windows/ConnectionGenericFileWindows.cpp @@ -50,8 +50,7 @@ class ReturnInfo { }; } -ConnectionGenericFile::ConnectionGenericFile() - : m_file(INVALID_HANDLE_VALUE), m_owns_file(false) { +ConnectionGenericFile::ConnectionGenericFile() : m_file(INVALID_HANDLE_VALUE) { ::ZeroMemory(&m_overlapped, sizeof(m_overlapped)); ::ZeroMemory(&m_file_position, sizeof(m_file_position)); InitializeEventHandles(); @@ -64,6 +63,13 @@ ConnectionGenericFile::ConnectionGenericFile(lldb::file_t file, bool owns_file) InitializeEventHandles(); } +ConnectionGenericFile::ConnectionGenericFile(PseudoConsole &pty) + : m_file(pty.GetSTDOUTHandle()), m_mutex(pty.GetMutex()) { + ::ZeroMemory(&m_overlapped, sizeof(m_overlapped)); + ::ZeroMemory(&m_file_position, sizeof(m_file_position)); + InitializeEventHandles(); +} + ConnectionGenericFile::~ConnectionGenericFile() { if (m_owns_file && IsConnected()) ::CloseHandle(m_file); @@ -164,6 +170,9 @@ size_t ConnectionGenericFile::Read(void *dst, size_t dst_len, const Timeout<std::micro> &timeout, lldb::ConnectionStatus &status, Status *error_ptr) { + std::unique_lock<std::mutex> guard; + if (m_mutex) + guard = std::unique_lock<std::mutex>(*m_mutex); ReturnInfo return_info; BOOL result = 0; DWORD bytes_read = 0; diff --git a/lldb/source/Host/windows/ProcessLauncherWindows.cpp b/lldb/source/Host/windows/ProcessLauncherWindows.cpp index cfd84731f0eb6..f22e4739ca7b6 100644 --- a/lldb/source/Host/windows/ProcessLauncherWindows.cpp +++ b/lldb/source/Host/windows/ProcessLauncherWindows.cpp @@ -124,7 +124,6 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info, startupinfoex.StartupInfo.cb = sizeof(STARTUPINFOEXW); startupinfoex.StartupInfo.dwFlags |= STARTF_USESTDHANDLES; - HPCON hPC = launch_info.GetPTY().GetPseudoTerminalHandle(); bool use_pty = launch_info.ShouldUsePTY(); HANDLE stdin_handle = GetStdioHandle(launch_info, STDIN_FILENO); @@ -148,6 +147,7 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info, std::vector<HANDLE> inherited_handles; if (use_pty) { + HPCON hPC = launch_info.GetPTY().GetPseudoTerminalHandle(); if (auto err = attributelist.SetupPseudoConsole(hPC)) { error = Status::FromError(std::move(err)); return HostProcess(); diff --git a/lldb/source/Host/windows/PseudoConsole.cpp b/lldb/source/Host/windows/PseudoConsole.cpp index 2228c315665f4..30fb683999c1d 100644 --- a/lldb/source/Host/windows/PseudoConsole.cpp +++ b/lldb/source/Host/windows/PseudoConsole.cpp @@ -129,6 +129,7 @@ llvm::Error PseudoConsole::OpenPseudoConsole() { } void PseudoConsole::Close() { + std::unique_lock<std::mutex> guard(m_mutex); if (m_conpty_handle != INVALID_HANDLE_VALUE) kernel32.ClosePseudoConsole(m_conpty_handle); if (m_conpty_input != INVALID_HANDLE_VALUE) diff --git a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp index 8c1919eca7dda..47973f7f72f43 100644 --- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp +++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp @@ -286,7 +286,11 @@ Status ProcessWindows::DoResume(RunDirection direction) { Status ProcessWindows::DoDestroy() { StateType private_state = GetPrivateState(); - return DestroyProcess(private_state); + auto status = DestroyProcess(private_state); + // Now that the m_stdio_communication is closed, we can close the ConPTY. + if (m_pty) + m_pty->Close(); + return status; } Status ProcessWindows::DoHalt(bool &caused_stop) { @@ -651,9 +655,6 @@ void ProcessWindows::OnExitProcess(uint32_t exit_code) { Log *log = GetLog(WindowsLog::Process); LLDB_LOG(log, "Process {0} exited with code {1}", GetID(), exit_code); - if (m_pty) - m_pty->Close(); - TargetSP target = CalculateTarget(); if (target) { ModuleSP executable_module = target->GetExecutableModule(); @@ -1134,7 +1135,7 @@ void ProcessWindows::SetPseudoConsoleHandle() { if (m_pty == nullptr) return; m_stdio_communication.SetConnection( - std::make_unique<ConnectionGenericFile>(m_pty->GetSTDOUTHandle(), false)); + std::make_unique<ConnectionGenericFile>(m_pty)); if (m_stdio_communication.IsConnected()) { m_stdio_communication.SetReadThreadBytesReceivedCallback( STDIOReadThreadBytesReceived, this); >From 0417811bbb7319531b074928f0b841d4006d7b65 Mon Sep 17 00:00:00 2001 From: Charles Zablit <[email protected]> Date: Thu, 19 Feb 2026 22:45:51 +0000 Subject: [PATCH 2/2] --wip-- [skip ci] --- .../windows/ConnectionGenericFileWindows.h | 3 ++ .../include/lldb/Host/windows/PseudoConsole.h | 4 +++ lldb/source/Core/ThreadedCommunication.cpp | 9 +++++- .../windows/ConnectionGenericFileWindows.cpp | 31 ++++++++++++++++--- lldb/source/Host/windows/PseudoConsole.cpp | 20 +++++++----- .../Process/Windows/Common/ProcessWindows.cpp | 14 +++++++-- 6 files changed, 67 insertions(+), 14 deletions(-) diff --git a/lldb/include/lldb/Host/windows/ConnectionGenericFileWindows.h b/lldb/include/lldb/Host/windows/ConnectionGenericFileWindows.h index 6d7be7db6d3ae..5626aa15131cb 100644 --- a/lldb/include/lldb/Host/windows/ConnectionGenericFileWindows.h +++ b/lldb/include/lldb/Host/windows/ConnectionGenericFileWindows.h @@ -48,6 +48,9 @@ class ConnectionGenericFile : public lldb_private::Connection { protected: OVERLAPPED m_overlapped; std::mutex *m_mutex = nullptr; + std::condition_variable *m_cv = nullptr; + std::atomic<bool> *m_pty_stopping = nullptr; + bool m_read_pending = false; HANDLE m_file; HANDLE m_event_handles[2]; bool m_owns_file = false; diff --git a/lldb/include/lldb/Host/windows/PseudoConsole.h b/lldb/include/lldb/Host/windows/PseudoConsole.h index f5317d0e97b19..c21ed17b748d9 100644 --- a/lldb/include/lldb/Host/windows/PseudoConsole.h +++ b/lldb/include/lldb/Host/windows/PseudoConsole.h @@ -74,11 +74,15 @@ class PseudoConsole { /// then drain all output before launching the actual debuggee. llvm::Error DrainInitSequences(); + std::condition_variable *GetCV() { return &m_cv; }; + std::atomic<bool> *GetStopping() { return &m_stopping;}; protected: HANDLE m_conpty_handle = ((HANDLE)(long long)-1); HANDLE m_conpty_output = ((HANDLE)(long long)-1); HANDLE m_conpty_input = ((HANDLE)(long long)-1); std::mutex m_mutex = {}; + std::condition_variable m_cv; + std::atomic<bool> m_stopping = false; }; } // namespace lldb_private diff --git a/lldb/source/Core/ThreadedCommunication.cpp b/lldb/source/Core/ThreadedCommunication.cpp index 09f78f13f34d3..575accc8b9aad 100644 --- a/lldb/source/Core/ThreadedCommunication.cpp +++ b/lldb/source/Core/ThreadedCommunication.cpp @@ -196,6 +196,7 @@ bool ThreadedCommunication::StartReadThread(Status *error_ptr) { bool ThreadedCommunication::StopReadThread(Status *error_ptr) { std::lock_guard<std::mutex> lock(m_read_thread_mutex); + llvm::errs() << "stopping\n"; if (!m_read_thread.IsJoinable()) return true; @@ -204,6 +205,7 @@ bool ThreadedCommunication::StopReadThread(Status *error_ptr) { "{0} ThreadedCommunication::StopReadThread ()", this); m_read_thread_enabled = false; + // m_connection_sp->InterruptRead(); BroadcastEvent(eBroadcastBitReadThreadShouldExit, nullptr); @@ -279,8 +281,12 @@ lldb::thread_result_t ThreadedCommunication::ReadThread() { while (!done && m_read_thread_enabled) { size_t bytes_read = ReadFromConnection( buf, sizeof(buf), std::chrono::seconds(5), status, &error); - if (bytes_read > 0 || status == eConnectionStatusEndOfFile) + if (bytes_read > 0 || status == eConnectionStatusEndOfFile) { + // llvm::errs() << "got: '"; + // llvm::errs().write(reinterpret_cast<const char*>(buf), bytes_read); + // llvm::errs() << "'\n"; AppendBytesToCache(buf, bytes_read, true, status); + } switch (status) { case eConnectionStatusSuccess: @@ -363,6 +369,7 @@ void ThreadedCommunication::SetReadThreadBytesReceivedCallback( void ThreadedCommunication::SynchronizeWithReadThread() { // Only one thread can do the synchronization dance at a time. std::lock_guard<std::mutex> guard(m_synchronize_mutex); + llvm::errs() << "syncing\n"; // First start listening for the synchronization event. ListenerSP listener_sp(Listener::MakeListener( diff --git a/lldb/source/Host/windows/ConnectionGenericFileWindows.cpp b/lldb/source/Host/windows/ConnectionGenericFileWindows.cpp index c4b7a63b60ac8..6daad11a84aa1 100644 --- a/lldb/source/Host/windows/ConnectionGenericFileWindows.cpp +++ b/lldb/source/Host/windows/ConnectionGenericFileWindows.cpp @@ -64,7 +64,7 @@ ConnectionGenericFile::ConnectionGenericFile(lldb::file_t file, bool owns_file) } ConnectionGenericFile::ConnectionGenericFile(PseudoConsole &pty) - : m_file(pty.GetSTDOUTHandle()), m_mutex(pty.GetMutex()) { + : m_file(pty.GetSTDOUTHandle()), m_mutex(pty.GetMutex()), m_cv(pty.GetCV()), m_pty_stopping(pty.GetStopping()) { ::ZeroMemory(&m_overlapped, sizeof(m_overlapped)); ::ZeroMemory(&m_file_position, sizeof(m_file_position)); InitializeEventHandles(); @@ -171,23 +171,38 @@ size_t ConnectionGenericFile::Read(void *dst, size_t dst_len, lldb::ConnectionStatus &status, Status *error_ptr) { std::unique_lock<std::mutex> guard; - if (m_mutex) + if (m_mutex) { guard = std::unique_lock<std::mutex>(*m_mutex); + if (m_pty_stopping->load()) { + llvm::errs() << "waiting for closing\n"; + m_cv->wait(guard, [this]{ return !m_pty_stopping->load(); }); + } + } ReturnInfo return_info; BOOL result = 0; DWORD bytes_read = 0; if (error_ptr) error_ptr->Clear(); + std::string s; if (!IsConnected()) { return_info.Set(0, eConnectionStatusNoConnection, ERROR_INVALID_HANDLE); goto finish; } - m_overlapped.hEvent = m_event_handles[kBytesAvailableEvent]; + if(m_read_pending) { + if (::GetOverlappedResult(m_file, &m_overlapped, &bytes_read, FALSE)) { + m_read_pending = false; + return bytes_read; + } + } + memset(dst, 0, 1024); + m_overlapped.hEvent = m_event_handles[kBytesAvailableEvent]; + llvm::errs() << "READING\n"; result = ::ReadFile(m_file, dst, dst_len, NULL, &m_overlapped); + m_read_pending = true; if (result || ::GetLastError() == ERROR_IO_PENDING) { if (!result) { // The expected return path. The operation is pending. Wait for the @@ -203,6 +218,7 @@ size_t ConnectionGenericFile::Read(void *dst, size_t dst_len, // to non-signalled. switch (wait_result) { case WAIT_OBJECT_0 + kBytesAvailableEvent: + // llvm::errs() << "kBytesAvailableEvent\n"; break; case WAIT_OBJECT_0 + kInterruptEvent: return_info.Set(0, eConnectionStatusInterrupted, 0); @@ -243,6 +259,14 @@ size_t ConnectionGenericFile::Read(void *dst, size_t dst_len, goto finish; finish: + if(return_info.GetStatus() != eConnectionStatusInterrupted) { + m_read_pending = false; + // s = std::string(reinterpret_cast<const char*>(dst)); + // return_info.Set(s.length(), eConnectionStatusInterrupted, 0); + } else { + } + llvm::errs() << "Got " << return_info.GetBytes() << " " << bytes_read << ' ' << m_overlapped.InternalHigh <<" bytes status: " << return_info.GetStatus() << '\n'; + // llvm::errs().write(reinterpret_cast<const char*>(dst), 1024); status = return_info.GetStatus(); if (error_ptr) *error_ptr = return_info.GetError().Clone(); @@ -259,7 +283,6 @@ size_t ConnectionGenericFile::Read(void *dst, size_t dst_len, "dst_len = %zu) => %zu, error = %s", static_cast<void *>(this), m_file, dst, dst_len, return_info.GetBytes(), return_info.GetError().AsCString()); - return return_info.GetBytes(); } diff --git a/lldb/source/Host/windows/PseudoConsole.cpp b/lldb/source/Host/windows/PseudoConsole.cpp index 30fb683999c1d..796b7ea7b9213 100644 --- a/lldb/source/Host/windows/PseudoConsole.cpp +++ b/lldb/source/Host/windows/PseudoConsole.cpp @@ -129,17 +129,23 @@ llvm::Error PseudoConsole::OpenPseudoConsole() { } void PseudoConsole::Close() { + llvm::errs() << "trying to close\n"; + m_stopping = true; std::unique_lock<std::mutex> guard(m_mutex); + llvm::errs() << "closing\n"; if (m_conpty_handle != INVALID_HANDLE_VALUE) kernel32.ClosePseudoConsole(m_conpty_handle); - if (m_conpty_input != INVALID_HANDLE_VALUE) - CloseHandle(m_conpty_input); - if (m_conpty_output != INVALID_HANDLE_VALUE) - CloseHandle(m_conpty_output); - m_conpty_handle = INVALID_HANDLE_VALUE; - m_conpty_input = INVALID_HANDLE_VALUE; - m_conpty_output = INVALID_HANDLE_VALUE; + m_stopping = false; + m_cv.notify_all(); + // if (m_conpty_input != INVALID_HANDLE_VALUE) + // CloseHandle(m_conpty_input); + // if (m_conpty_output != INVALID_HANDLE_VALUE) + // CloseHandle(m_conpty_output); + + // m_conpty_handle = INVALID_HANDLE_VALUE; + // m_conpty_input = INVALID_HANDLE_VALUE; + // m_conpty_output = INVALID_HANDLE_VALUE; } llvm::Error PseudoConsole::DrainInitSequences() { diff --git a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp index 47973f7f72f43..3951a35baf75f 100644 --- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp +++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp @@ -288,8 +288,10 @@ Status ProcessWindows::DoDestroy() { StateType private_state = GetPrivateState(); auto status = DestroyProcess(private_state); // Now that the m_stdio_communication is closed, we can close the ConPTY. - if (m_pty) + if (m_pty) { + llvm::errs() << "closing the pty\n"; m_pty->Close(); + } return status; } @@ -655,6 +657,14 @@ void ProcessWindows::OnExitProcess(uint32_t exit_code) { Log *log = GetLog(WindowsLog::Process); LLDB_LOG(log, "Process {0} exited with code {1}", GetID(), exit_code); + if (m_pty) { + m_pty->GetStopping()->store(true); + m_stdio_communication.SynchronizeWithReadThread(); + // m_stdio_communication.StopReadThread(); + // m_stdio_communication.Disconnect(); + m_pty->Close(); + } + TargetSP target = CalculateTarget(); if (target) { ModuleSP executable_module = target->GetExecutableModule(); @@ -1135,7 +1145,7 @@ void ProcessWindows::SetPseudoConsoleHandle() { if (m_pty == nullptr) return; m_stdio_communication.SetConnection( - std::make_unique<ConnectionGenericFile>(m_pty)); + std::make_unique<ConnectionGenericFile>(*m_pty)); if (m_stdio_communication.IsConnected()) { m_stdio_communication.SetReadThreadBytesReceivedCallback( STDIOReadThreadBytesReceived, this); _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
