ProcessLaunchInfo: Remove Target reference

Summary:
The target was being used in FinalizeFileActions to provide default
values for stdin/out/err. Also, most of the logic of this function was
very specific to how the lldb's Target class wants to launch processes,
so I, move it to Target::FinalizeFileActions, inverting the dependency.
The only piece of logic that was useful elsewhere (lldb-server) was the
part which sets up a pty and relevant file actions. I've kept this part
as ProcessLaunchInfo::SetUpPtyRedirection.

This makes ProcessLaunchInfo independent of any high-level lldb constructs.

Reviewers: zturner, jingham, teemperor

Subscribers: lldb-commits

Differential Revision: https://reviews.llvm.org/D56196

5 files changed, 119 insertions, 129 deletions

diff --git a/lldb/include/lldb/Target/ProcessLaunchInfo.h b/lldb/include/lldb/Target/ProcessLaunchInfo.h
index fbd319abc4f..ef1d63035b1 100644
--- a/lldb/include/lldb/Target/ProcessLaunchInfo.h
+++ b/lldb/include/lldb/Target/ProcessLaunchInfo.h
@@ -52,7 +52,10 @@ public:
 
   bool AppendSuppressFileAction(int fd, bool read, bool write);
 
-  void FinalizeFileActions(Target *target, bool default_to_use_pty);
+  // Redirect stdin/stdout/stderr to a pty, if no action for the respective file
+  // descriptor is specified. (So if stdin and stdout already have file actions,
+  // but stderr doesn't, then only stderr will be redirected to a pty.)
+  llvm::Error SetUpPtyRedirection();
 
   size_t GetNumFileActions() const { return m_file_actions.size(); }
 
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index 801ffb31c0d..90df1f4929b 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1354,6 +1354,8 @@ private:
 
   void AddBreakpoint(lldb::BreakpointSP breakpoint_sp, bool internal);
 
+  void FinalizeFileActions(ProcessLaunchInfo &info);
+
   DISALLOW_COPY_AND_ASSIGN(Target);
 };
 
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
index d15f54dcc2a..cdb63e72f6b 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -218,8 +218,10 @@ Status GDBRemoteCommunicationServerLLGS::LaunchProcess() {
   m_process_launch_info.SetLaunchInSeparateProcessGroup(true);
   m_process_launch_info.GetFlags().Set(eLaunchFlagDebug);
 
-  const bool default_to_use_pty = true;
-  m_process_launch_info.FinalizeFileActions(nullptr, default_to_use_pty);
+  if (should_forward_stdio) {
+    if (llvm::Error Err = m_process_launch_info.SetUpPtyRedirection())
+      return Status(std::move(Err));
+  }
 
   {
     std::lock_guard<std::recursive_mutex> guard(m_debugged_process_mutex);
diff --git a/lldb/source/Target/ProcessLaunchInfo.cpp b/lldb/source/Target/ProcessLaunchInfo.cpp
index a7a5a5c39dd..ac1eba04f0c 100644
--- a/lldb/source/Target/ProcessLaunchInfo.cpp
+++ b/lldb/source/Target/ProcessLaunchInfo.cpp
@@ -14,7 +14,6 @@
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Target/FileAction.h"
 #include "lldb/Target/ProcessLaunchInfo.h"
-#include "lldb/Target/Target.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
 
@@ -206,123 +205,38 @@ void ProcessLaunchInfo::SetDetachOnError(bool enable) {
     m_flags.Clear(lldb::eLaunchFlagDetachOnError);
 }
 
-void ProcessLaunchInfo::FinalizeFileActions(Target *target,
-                                            bool default_to_use_pty) {
-  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
-
-  // If nothing for stdin or stdout or stderr was specified, then check the
-  // process for any default settings that were set with "settings set"
-  if (GetFileActionForFD(STDIN_FILENO) == nullptr ||
-      GetFileActionForFD(STDOUT_FILENO) == nullptr ||
-      GetFileActionForFD(STDERR_FILENO) == nullptr) {
-    if (log)
-      log->Printf("ProcessLaunchInfo::%s at least one of stdin/stdout/stderr "
-                  "was not set, evaluating default handling",
-                  __FUNCTION__);
-
-    if (m_flags.Test(eLaunchFlagLaunchInTTY)) {
-      // Do nothing, if we are launching in a remote terminal no file actions
-      // should be done at all.
-      return;
-    }
-
-    if (m_flags.Test(eLaunchFlagDisableSTDIO)) {
-      if (log)
-        log->Printf("ProcessLaunchInfo::%s eLaunchFlagDisableSTDIO set, adding "
-                    "suppression action for stdin, stdout and stderr",
-                    __FUNCTION__);
-      AppendSuppressFileAction(STDIN_FILENO, true, false);
-      AppendSuppressFileAction(STDOUT_FILENO, false, true);
-      AppendSuppressFileAction(STDERR_FILENO, false, true);
-    } else {
-      // Check for any values that might have gotten set with any of: (lldb)
-      // settings set target.input-path (lldb) settings set target.output-path
-      // (lldb) settings set target.error-path
-      FileSpec in_file_spec;
-      FileSpec out_file_spec;
-      FileSpec err_file_spec;
-      if (target) {
-        // Only override with the target settings if we don't already have an
-        // action for in, out or error
-        if (GetFileActionForFD(STDIN_FILENO) == nullptr)
-          in_file_spec = target->GetStandardInputPath();
-        if (GetFileActionForFD(STDOUT_FILENO) == nullptr)
-          out_file_spec = target->GetStandardOutputPath();
-        if (GetFileActionForFD(STDERR_FILENO) == nullptr)
-          err_file_spec = target->GetStandardErrorPath();
-      }
-
-      if (log)
-        log->Printf("ProcessLaunchInfo::%s target stdin='%s', target "
-                    "stdout='%s', stderr='%s'",
-                    __FUNCTION__,
-                    in_file_spec ? in_file_spec.GetCString() : "<null>",
-                    out_file_spec ? out_file_spec.GetCString() : "<null>",
-                    err_file_spec ? err_file_spec.GetCString() : "<null>");
-
-      if (in_file_spec) {
-        AppendOpenFileAction(STDIN_FILENO, in_file_spec, true, false);
-        if (log)
-          log->Printf(
-              "ProcessLaunchInfo::%s appended stdin open file action for %s",
-              __FUNCTION__, in_file_spec.GetCString());
-      }
-
-      if (out_file_spec) {
-        AppendOpenFileAction(STDOUT_FILENO, out_file_spec, false, true);
-        if (log)
-          log->Printf(
-              "ProcessLaunchInfo::%s appended stdout open file action for %s",
-              __FUNCTION__, out_file_spec.GetCString());
-      }
-
-      if (err_file_spec) {
-        AppendOpenFileAction(STDERR_FILENO, err_file_spec, false, true);
-        if (log)
-          log->Printf(
-              "ProcessLaunchInfo::%s appended stderr open file action for %s",
-              __FUNCTION__, err_file_spec.GetCString());
-      }
+llvm::Error ProcessLaunchInfo::SetUpPtyRedirection() {
+  Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS);
+  LLDB_LOG(log, "Generating a pty to use for stdin/out/err");
 
-      if (default_to_use_pty &&
-          (!in_file_spec || !out_file_spec || !err_file_spec)) {
-        if (log)
-          log->Printf("ProcessLaunchInfo::%s default_to_use_pty is set, and at "
-                      "least one stdin/stderr/stdout is unset, so generating a "
-                      "pty to use for it",
-                      __FUNCTION__);
-
-        int open_flags = O_RDWR | O_NOCTTY;
+  int open_flags = O_RDWR | O_NOCTTY;
 #if !defined(_WIN32)
-        // We really shouldn't be specifying platform specific flags that are
-        // intended for a system call in generic code.  But this will have to
-        // do for now.
-        open_flags |= O_CLOEXEC;
+  // We really shouldn't be specifying platform specific flags that are
+  // intended for a system call in generic code.  But this will have to
+  // do for now.
+  open_flags |= O_CLOEXEC;
 #endif
-        if (m_pty->OpenFirstAvailableMaster(open_flags, nullptr, 0)) {
-          const FileSpec slave_file_spec(m_pty->GetSlaveName(nullptr, 0));
-
-          // Only use the slave tty if we don't have anything specified for
-          // input and don't have an action for stdin
-          if (!in_file_spec && GetFileActionForFD(STDIN_FILENO) == nullptr) {
-            AppendOpenFileAction(STDIN_FILENO, slave_file_spec, true, false);
-          }
-
-          // Only use the slave tty if we don't have anything specified for
-          // output and don't have an action for stdout
-          if (!out_file_spec && GetFileActionForFD(STDOUT_FILENO) == nullptr) {
-            AppendOpenFileAction(STDOUT_FILENO, slave_file_spec, false, true);
-          }
-
-          // Only use the slave tty if we don't have anything specified for
-          // error and don't have an action for stderr
-          if (!err_file_spec && GetFileActionForFD(STDERR_FILENO) == nullptr) {
-            AppendOpenFileAction(STDERR_FILENO, slave_file_spec, false, true);
-          }
-        }
-      }
-    }
+  if (!m_pty->OpenFirstAvailableMaster(open_flags, nullptr, 0)) {
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "PTY::OpenFirstAvailableMaster failed");
   }
+  const FileSpec slave_file_spec(m_pty->GetSlaveName(nullptr, 0));
+
+  // Only use the slave tty if we don't have anything specified for
+  // input and don't have an action for stdin
+  if (GetFileActionForFD(STDIN_FILENO) == nullptr)
+    AppendOpenFileAction(STDIN_FILENO, slave_file_spec, true, false);
+
+  // Only use the slave tty if we don't have anything specified for
+  // output and don't have an action for stdout
+  if (GetFileActionForFD(STDOUT_FILENO) == nullptr)
+    AppendOpenFileAction(STDOUT_FILENO, slave_file_spec, false, true);
+
+  // Only use the slave tty if we don't have anything specified for
+  // error and don't have an action for stderr
+  if (GetFileActionForFD(STDERR_FILENO) == nullptr)
+    AppendOpenFileAction(STDERR_FILENO, slave_file_spec, false, true);
+  return llvm::Error::success();
 }
 
 bool ProcessLaunchInfo::ConvertArgumentsForLaunchingInShell(
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 84890e0b5cf..3ba5fc0a728 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -2834,18 +2834,7 @@ Status Target::Launch(ProcessLaunchInfo &launch_info, Stream *stream) {
 
   PlatformSP platform_sp(GetPlatform());
 
-  // Finalize the file actions, and if none were given, default to opening up a
-  // pseudo terminal
-  const bool default_to_use_pty = platform_sp ? platform_sp->IsHost() : false;
-  if (log)
-    log->Printf("Target::%s have platform=%s, platform_sp->IsHost()=%s, "
-                "default_to_use_pty=%s",
-                __FUNCTION__, platform_sp ? "true" : "false",
-                platform_sp ? (platform_sp->IsHost() ? "true" : "false")
-                            : "n/a",
-                default_to_use_pty ? "true" : "false");
-
-  launch_info.FinalizeFileActions(this, default_to_use_pty);
+  FinalizeFileActions(launch_info);
 
   if (state == eStateConnected) {
     if (launch_info.GetFlags().Test(eLaunchFlagLaunchInTTY)) {
@@ -3061,6 +3050,86 @@ Status Target::Attach(ProcessAttachInfo &attach_info, Stream *stream) {
   return error;
 }
 
+void Target::FinalizeFileActions(ProcessLaunchInfo &info) {
+  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
+
+  // Finalize the file actions, and if none were given, default to opening up a
+  // pseudo terminal
+  PlatformSP platform_sp = GetPlatform();
+  const bool default_to_use_pty =
+      m_platform_sp ? m_platform_sp->IsHost() : false;
+  LLDB_LOG(
+      log,
+      "have platform={0}, platform_sp->IsHost()={1}, default_to_use_pty={2}",
+      bool(platform_sp),
+      platform_sp ? (platform_sp->IsHost() ? "true" : "false") : "n/a",
+      default_to_use_pty);
+
+  // If nothing for stdin or stdout or stderr was specified, then check the
+  // process for any default settings that were set with "settings set"
+  if (info.GetFileActionForFD(STDIN_FILENO) == nullptr ||
+      info.GetFileActionForFD(STDOUT_FILENO) == nullptr ||
+      info.GetFileActionForFD(STDERR_FILENO) == nullptr) {
+    LLDB_LOG(log, "at least one of stdin/stdout/stderr was not set, evaluating "
+                  "default handling");
+
+    if (info.GetFlags().Test(eLaunchFlagLaunchInTTY)) {
+      // Do nothing, if we are launching in a remote terminal no file actions
+      // should be done at all.
+      return;
+    }
+
+    if (info.GetFlags().Test(eLaunchFlagDisableSTDIO)) {
+      LLDB_LOG(log, "eLaunchFlagDisableSTDIO set, adding suppression action "
+                    "for stdin, stdout and stderr");
+      info.AppendSuppressFileAction(STDIN_FILENO, true, false);
+      info.AppendSuppressFileAction(STDOUT_FILENO, false, true);
+      info.AppendSuppressFileAction(STDERR_FILENO, false, true);
+    } else {
+      // Check for any values that might have gotten set with any of: (lldb)
+      // settings set target.input-path (lldb) settings set target.output-path
+      // (lldb) settings set target.error-path
+      FileSpec in_file_spec;
+      FileSpec out_file_spec;
+      FileSpec err_file_spec;
+      // Only override with the target settings if we don't already have an
+      // action for in, out or error
+      if (info.GetFileActionForFD(STDIN_FILENO) == nullptr)
+        in_file_spec = GetStandardInputPath();
+      if (info.GetFileActionForFD(STDOUT_FILENO) == nullptr)
+        out_file_spec = GetStandardOutputPath();
+      if (info.GetFileActionForFD(STDERR_FILENO) == nullptr)
+        err_file_spec = GetStandardErrorPath();
+
+      LLDB_LOG(log, "target stdin='{0}', target stdout='{1}', stderr='{1}'",
+               in_file_spec, out_file_spec, err_file_spec);
+
+      if (in_file_spec) {
+        info.AppendOpenFileAction(STDIN_FILENO, in_file_spec, true, false);
+        LLDB_LOG(log, "appended stdin open file action for {0}", in_file_spec);
+      }
+
+      if (out_file_spec) {
+        info.AppendOpenFileAction(STDOUT_FILENO, out_file_spec, false, true);
+        LLDB_LOG(log, "appended stdout open file action for {0}",
+                 out_file_spec);
+      }
+
+      if (err_file_spec) {
+        info.AppendOpenFileAction(STDERR_FILENO, err_file_spec, false, true);
+        LLDB_LOG(log, "appended stderr open file action for {0}",
+                 err_file_spec);
+      }
+
+      if (default_to_use_pty &&
+          (!in_file_spec || !out_file_spec || !err_file_spec)) {
+        llvm::Error Err = info.SetUpPtyRedirection();
+        LLDB_LOG_ERROR(log, std::move(Err), "SetUpPtyRedirection failed: {0}");
+      }
+    }
+  }
+}
+
 //--------------------------------------------------------------
 // Target::StopHook
 //--------------------------------------------------------------