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 dc752c3cedec79a6a5879c05a62949f31204c6cd Mon Sep 17 00:00:00 2001 From: Charles Zablit <[email protected]> Date: Thu, 19 Feb 2026 19:52:32 +0000 Subject: [PATCH 2/2] --wip-- [skip ci] --- lldb/source/Core/ThreadedCommunication.cpp | 9 ++++++- .../windows/ConnectionGenericFileWindows.cpp | 27 ++++++++++++++++--- lldb/source/Host/windows/PseudoConsole.cpp | 16 ++++++----- .../Process/Windows/Common/ProcessWindows.cpp | 13 +++++++-- 4 files changed, 51 insertions(+), 14 deletions(-) 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..7ef133f97bc7f 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(nullptr) { ::ZeroMemory(&m_overlapped, sizeof(m_overlapped)); ::ZeroMemory(&m_file_position, sizeof(m_file_position)); InitializeEventHandles(); @@ -179,14 +179,16 @@ size_t ConnectionGenericFile::Read(void *dst, size_t dst_len, if (error_ptr) error_ptr->Clear(); + std::string s; if (!IsConnected()) { return_info.Set(0, eConnectionStatusNoConnection, ERROR_INVALID_HANDLE); goto finish; } + 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); if (result || ::GetLastError() == ERROR_IO_PENDING) { if (!result) { @@ -203,9 +205,21 @@ 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); + // llvm::errs() << "kInterruptEvent\n"; + // if (WaitForSingleObject(m_event_handles[kBytesAvailableEvent], 1000) == WAIT_OBJECT_0) { + // llvm::errs() << "no it's available!\n"; + // // Data is ready, fall through to GetOverlappedResult + // break; + // } + // llvm::errs() << "overlapped: " << m_overlapped.InternalHigh << '\n'; + // if (bytes_read == 0 && m_overlapped.InternalHigh > 0) { + // return_info.Set(m_overlapped.InternalHigh, eConnectionStatusInterrupted, 0); + // } else { + return_info.Set(0, eConnectionStatusInterrupted, 0); + // } goto finish; case WAIT_TIMEOUT: return_info.Set(0, eConnectionStatusTimedOut, 0); @@ -243,6 +257,12 @@ size_t ConnectionGenericFile::Read(void *dst, size_t dst_len, goto finish; finish: + if(return_info.GetStatus() == eConnectionStatusInterrupted) { + s = std::string(reinterpret_cast<const char*>(dst)); + return_info.Set(s.length(), eConnectionStatusInterrupted, 0); + } + 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 +279,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..6710404f1f11f 100644 --- a/lldb/source/Host/windows/PseudoConsole.cpp +++ b/lldb/source/Host/windows/PseudoConsole.cpp @@ -130,16 +130,18 @@ llvm::Error PseudoConsole::OpenPseudoConsole() { void PseudoConsole::Close() { 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; + // 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..920cd13b8c039 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,13 @@ 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_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 +1144,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
