[Lldb-commits] [lldb] [lldb][windows] release the ConPTY instead of copying its reference (PR #181811)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Charles Zablit (charles-zablit)

<details>
<summary>Changes</summary>

When running multiple targets in lldb, the `ProcessLaunchInfo` instance gets re-used. This is a problem for the ConPTY instance which is owned by `launch_info` because, when creating a second target, we close and reopen the `m_pty` of the first target.

This patch replaces the `GetPTYSP` method with `ReleasePTY`, which moves the `std::shared_ptr` out of `launch_info` instead of creating a new `std::shared_ptr`. The `m_pty` instance is moved into the target's process. This way, each target's `Process` instance owns its PTY instance and is responsible for closing it.

---
Full diff: https://github.com/llvm/llvm-project/pull/181811.diff


7 Files Affected:

- (modified) lldb/include/lldb/Host/ProcessLaunchInfo.h (+1-1) 
- (modified) lldb/include/lldb/Target/Process.h (+1-6) 
- (modified) lldb/source/Host/common/ProcessLaunchInfo.cpp (+3) 
- (modified) lldb/source/Host/windows/PseudoConsole.cpp (+2-2) 
- (modified) lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp (+1-2) 
- (modified) lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp (+6-5) 
- (modified) lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h (+1-2) 


``````````diff
diff --git a/lldb/include/lldb/Host/ProcessLaunchInfo.h b/lldb/include/lldb/Host/ProcessLaunchInfo.h
index 023d150503da3..a07f35ac472e9 100644
--- a/lldb/include/lldb/Host/ProcessLaunchInfo.h
+++ b/lldb/include/lldb/Host/ProcessLaunchInfo.h
@@ -130,7 +130,7 @@ class ProcessLaunchInfo : public ProcessInfo {
 
   PTY &GetPTY() const { return *m_pty; }
 
-  std::shared_ptr<PTY> GetPTYSP() const { return m_pty; }
+  std::shared_ptr<PTY> ReleasePTY() { return std::move(m_pty); }
 
   /// Returns whether if lldb should read information from the PTY. This is
   /// always true on non Windows.
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index 7a15adebc63ee..c4eb8cf3694b1 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -2561,15 +2561,10 @@ void PruneThreadPlans();
   /// When data is successfully read from the ConPTY, it is stored in
   /// m_stdout_data. There is no differentiation between stdout and stderr.
   ///
-  /// \param[in] pty
-  ///     The ConPTY to use for process STDIO communication. It's
-  ///     assumed to be valid.
-  ///
   /// \see lldb_private::Process::STDIOReadThreadBytesReceived()
   /// \see lldb_private::IOHandlerProcessSTDIOWindows
   /// \see lldb_private::PseudoConsole
-  virtual void
-  SetPseudoConsoleHandle(const std::shared_ptr<PseudoConsole> &pty) {};
+  virtual void SetPseudoConsoleHandle() {};
 #endif
 
   /// Associates a file descriptor with the process' STDIO handling
diff --git a/lldb/source/Host/common/ProcessLaunchInfo.cpp b/lldb/source/Host/common/ProcessLaunchInfo.cpp
index e82cc11187fe5..3c354c9164a50 100644
--- a/lldb/source/Host/common/ProcessLaunchInfo.cpp
+++ b/lldb/source/Host/common/ProcessLaunchInfo.cpp
@@ -200,6 +200,9 @@ void ProcessLaunchInfo::SetDetachOnError(bool enable) {
 llvm::Error ProcessLaunchInfo::SetUpPtyRedirection() {
   Log *log = GetLog(LLDBLog::Process);
 
+  if (m_pty == nullptr)
+    m_pty = std::make_shared<PTY>();
+
   bool stdin_free = GetFileActionForFD(STDIN_FILENO) == nullptr;
   bool stdout_free = GetFileActionForFD(STDOUT_FILENO) == nullptr;
   bool stderr_free = GetFileActionForFD(STDERR_FILENO) == nullptr;
diff --git a/lldb/source/Host/windows/PseudoConsole.cpp b/lldb/source/Host/windows/PseudoConsole.cpp
index 413c8a3328445..2228c315665f4 100644
--- a/lldb/source/Host/windows/PseudoConsole.cpp
+++ b/lldb/source/Host/windows/PseudoConsole.cpp
@@ -72,8 +72,8 @@ llvm::Error PseudoConsole::OpenPseudoConsole() {
     return llvm::make_error<llvm::StringError>("ConPTY is not available",
                                                llvm::errc::io_error);
 
-  // close any previously opened handles
-  Close();
+  assert(m_conpty_handle == INVALID_HANDLE_VALUE &&
+         "ConPTY has already been opened");
 
   HRESULT hr;
   HANDLE hInputRead = INVALID_HANDLE_VALUE;
diff --git a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
index 0e74cab23c99e..1bc9d3fb9978d 100644
--- a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -523,8 +523,7 @@ ProcessSP PlatformWindows::DebugProcess(ProcessLaunchInfo &launch_info,
   error = process_sp->Launch(launch_info);
 #ifdef _WIN32
   if (error.Success()) {
-    if (launch_info.ShouldUsePTY())
-      process_sp->SetPseudoConsoleHandle(launch_info.GetPTYSP());
+    process_sp->SetPseudoConsoleHandle();
   } else {
     Log *log = GetLog(LLDBLog::Platform);
     LLDB_LOGF(log, "Platform::%s LaunchProcess() failed: %s", __FUNCTION__,
diff --git a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
index 373729c952071..ce969e2ff8454 100644
--- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
@@ -214,7 +214,7 @@ Status ProcessWindows::DoLaunch(Module *exe_module,
   error = LaunchProcess(launch_info, delegate);
   if (error.Success())
     SetID(launch_info.GetProcessID());
-  m_pty = launch_info.GetPTYSP();
+  m_pty = launch_info.ReleasePTY();
   return error;
 }
 
@@ -1130,10 +1130,11 @@ class IOHandlerProcessSTDIOWindows : public IOHandler {
   bool m_is_running = false;
 };
 
-void ProcessWindows::SetPseudoConsoleHandle(
-    const std::shared_ptr<PseudoConsole> &pty) {
+void ProcessWindows::SetPseudoConsoleHandle() {
+  if (m_pty == nullptr)
+    return;
   m_stdio_communication.SetConnection(
-      std::make_unique<ConnectionGenericFile>(pty->GetSTDOUTHandle(), false));
+      std::make_unique<ConnectionGenericFile>(m_pty->GetSTDOUTHandle(), false));
   if (m_stdio_communication.IsConnected()) {
     m_stdio_communication.SetReadThreadBytesReceivedCallback(
         STDIOReadThreadBytesReceived, this);
@@ -1144,7 +1145,7 @@ void ProcessWindows::SetPseudoConsoleHandle(
       std::lock_guard<std::mutex> guard(m_process_input_reader_mutex);
       if (!m_process_input_reader)
         m_process_input_reader = std::make_shared<IOHandlerProcessSTDIOWindows>(
-            this, pty->GetSTDINHandle());
+            this, m_pty->GetSTDINHandle());
     }
   }
 }
diff --git a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h
index becab6964a4aa..d3e3f9a5ed0ce 100644
--- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h
+++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h
@@ -98,8 +98,7 @@ class ProcessWindows : public Process, public ProcessDebugger {
   Status DisableWatchpoint(lldb::WatchpointSP wp_sp,
                            bool notify = true) override;
 
-  void
-  SetPseudoConsoleHandle(const std::shared_ptr<PseudoConsole> &pty) override;
+  void SetPseudoConsoleHandle() override;
 
 protected:
   ProcessWindows(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);

``````````

</details>


https://github.com/llvm/llvm-project/pull/181811