ProcessLaunchInfo: Remove Target reference

[labath]

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.

[Hui]

lldb/trunk/source/Target/ProcessLaunchInfo.cpp

220 ↗

(On Diff #180639)

Look like the windows implementation of OpenFirstAvailableMaster is missing.

There was no error before these changes.

$ ./lldb-server.exe g *:2000 --log-channels="lldb all:gdb-remote all" "d:\\m.exe"
1548354703.504552364 lldb-server launch
1548354703.504905224 argv[0] = 'g'
1548354703.505040407 argv[1] = '--log-channels=lldb all:gdb-remote all'
1548354703.505141735 argv[2] = '*:2000'
1548354703.505235672 argv[3] = 'd:\m.exe'
1548354703.505473614 000000DBF458F040 Broadcaster::Broadcaster("gdb-remote.server")
1548354703.505620718 000000DBF458F040 Communication::Communication (name = gdb-remote.server)
1548354703.507417917 000000DBF458F658 Broadcaster::Broadcaster("process.stdio")
1548354703.507594585 000000DBF458F658 Communication::Communication (name = process.stdio)
1548354703.510816097 processlaunchinfo.cpp:SetUpPtyRedirection Generating a pty to use for stdin/out/err
error: failed to launch 'd:\m.exe': PTY::OpenFirstAvailableMaster failed

[labath]

lldb/trunk/source/Target/ProcessLaunchInfo.cpp

220 ↗

(On Diff #180639)

Yes, there was no error before, though that doesn't mean that the code was correct. The reason you're seeing the error now is because I changed GDBRemoteCommunicationServerLLGS::LaunchProcess to check the error code, whereas previously it was ignoring it.

Overall, I think you'll have to figure out stdio forwarding sooner or later, but if it's enough for you right now to replicate the !lldb_server path and open a new terminal for the debugged app, then the easiest way to achieve that would be to move the pty setup code into the NativeProcess class. So, NativeProcessLinux/NetBSD would call SetUpPtyRedirection to do the usual stdout forwarding, and the windows process would do whatever it takes to create a new terminal window (and return -1 for GetTerminalFileDescriptor).

include/lldb/Target/ProcessLaunchInfo.h

public:
 
   bool AppendDuplicateFileAction(int fd, int dup_fd);
 
   bool AppendOpenFileAction(int fd, const FileSpec &file_spec, bool read,
                             bool write);
 
   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(); }
 
   const FileAction *GetFileActionAtIndex(size_t idx) const;
 
   const FileAction *GetFileActionForFD(int fd) const;
 
   Flags &GetFlags() { return m_flags; }

include/lldb/Target/Target.h

private:
   // Helper function.
   bool ProcessIsValid();
 
   // Copy breakpoints, stop hooks and so forth from the dummy target:
   void PrimeFromDummyTarget(Target *dummy_target);
 
   void AddBreakpoint(lldb::BreakpointSP breakpoint_sp, bool internal);
 
+  void FinalizeFileActions(ProcessLaunchInfo &info);
+
   DISALLOW_COPY_AND_ASSIGN(Target);
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_Target_h_

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

Status GDBRemoteCommunicationServerLLGS::LaunchProcess() {
 
   const bool should_forward_stdio =
       m_process_launch_info.GetFileActionForFD(STDIN_FILENO) == nullptr ||
       m_process_launch_info.GetFileActionForFD(STDOUT_FILENO) == nullptr ||
       m_process_launch_info.GetFileActionForFD(STDERR_FILENO) == nullptr;
   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);
     assert(!m_debugged_process_up && "lldb-server creating debugged "
                                      "process but one already exists");
     auto process_or =
         m_process_factory.Launch(m_process_launch_info, *this, m_mainloop);
     if (!process_or)

source/Target/ProcessLaunchInfo.cpp

 //===-- ProcessLaunchInfo.cpp -----------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include <climits>
 
 #include "lldb/Host/Config.h"
 #include "lldb/Host/FileSystem.h"
 #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"
 
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/FileSystem.h"
 
 #if !defined(_WIN32)
 #include <limits.h>

 
 void ProcessLaunchInfo::SetDetachOnError(bool enable) {
   if (enable)
     m_flags.Set(lldb::eLaunchFlagDetachOnError);
   else
     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());
-      }
-
-      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__);
+llvm::Error ProcessLaunchInfo::SetUpPtyRedirection() {
+  Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS);
+  LLDB_LOG(log, "Generating a pty to use for stdin/out/err");
 
-        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;
 #endif
-        if (m_pty->OpenFirstAvailableMaster(open_flags, nullptr, 0)) {
-          const FileSpec slave_file_spec(m_pty->GetSlaveName(nullptr, 0));
+  if (!m_pty->OpenFirstAvailableMaster(open_flags, nullptr, 0)) {
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "PTY::OpenFirstAvailableMaster failed");
+  }
 
-          // 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);
-          }
+  if (GetFileActionForFD(STDIN_FILENO) == nullptr)
 
-          // 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);
-          }
+  if (GetFileActionForFD(STDOUT_FILENO) == nullptr)
 
-          // 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 (GetFileActionForFD(STDERR_FILENO) == nullptr)
+  return llvm::Error::success();
 }
 
 bool ProcessLaunchInfo::ConvertArgumentsForLaunchingInShell(
     Status &error, bool localhost, bool will_debug,
     bool first_arg_is_full_shell_command, int32_t num_resumes) {
   error.Clear();
 
   if (GetFlags().Test(eLaunchFlagLaunchInShell)) {

source/Target/Target.cpp

Status Target::Launch(ProcessLaunchInfo &launch_info, Stream *stream) {
   // have started to run, then you could have hit a breakpoint, whose command
   // might switch the value, and then you'll pick up that incorrect value.
   Debugger &debugger = GetDebugger();
   const bool synchronous_execution =
       debugger.GetCommandInterpreter().GetSynchronous();
 
   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)) {
       error.SetErrorString(
           "can't launch in tty when launching through a remote connection");
       return error;
     }
   }
if (async) {
               "process did not stop (no such process or permission problem?)");
         process_sp->Destroy(false);
       }
     }
   }
   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
 //--------------------------------------------------------------
 Target::StopHook::StopHook(lldb::TargetSP target_sp, lldb::user_id_t uid)
     : UserID(uid), m_target_sp(target_sp), m_commands(), m_specifier_sp(),
       m_thread_spec_ap(), m_active(true) {}
 
 Target::StopHook::StopHook(const StopHook &rhs)