From 40af87383f2e51bfc698dcb6fcba2af23bc19756 Mon Sep 17 00:00:00 2001 From: Pavel Labath Date: Tue, 8 Jan 2019 11:55:19 +0000 Subject: 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 --- lldb/include/lldb/Target/ProcessLaunchInfo.h | 5 +- lldb/include/lldb/Target/Target.h | 2 + .../GDBRemoteCommunicationServerLLGS.cpp | 6 +- lldb/source/Target/ProcessLaunchInfo.cpp | 142 ++++----------------- lldb/source/Target/Target.cpp | 93 ++++++++++++-- 5 files changed, 119 insertions(+), 129 deletions(-) (limited to 'lldb') 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 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() : "", - out_file_spec ? out_file_spec.GetCString() : "", - err_file_spec ? err_file_spec.GetCString() : ""); - - 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 //-------------------------------------------------------------- -- cgit v1.2.3