llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Jason Molenda (jasonmolenda) <details> <summary>Changes</summary> Host::RunShellCommand takes a std::string &command_output argument and a bool hide_stderr=false defaulted argument. If the shell command returns stderr and stdout text, it is intermixed in the same command_output, unless hide_stderr=true. In SymbolLocatorDebugSymbols::DownloadObjectAndSymbolFile we call an external program to find a binary and dSYM by uuid, and the external program returns a plist (xml) output. In some cases, it printed a (harmless) warning message to stderr, and then a complete plist output to stdout. We attempt to parse the combination of these two streams, and the parse fails - we don't get the output. This patch removes hide_stderr and instead adds a std::string &error_output argument, and callers can choose to use/print/return error_output, or ignore it. This keeps command_output the clean stdout from the shell command. rdar://168621579 --- Patch is 35.50 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/184548.diff 19 Files Affected: - (modified) lldb/include/lldb/Host/Host.h (+80-37) - (modified) lldb/include/lldb/Target/Platform.h (+4) - (modified) lldb/include/lldb/Target/RemoteAwarePlatform.h (+2-1) - (modified) lldb/source/API/SBPlatform.cpp (+2-1) - (modified) lldb/source/Commands/CommandObjectPlatform.cpp (+6-3) - (modified) lldb/source/Host/common/Host.cpp (+58-14) - (modified) lldb/source/Host/macosx/objcxx/Host.mm (+2-2) - (modified) lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm (+6-2) - (modified) lldb/source/Host/windows/Host.cpp (+4-3) - (modified) lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp (+7-6) - (modified) lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp (+5-5) - (modified) lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp (+4-2) - (modified) lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h (+2) - (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp (+2) - (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h (+2) - (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp (+3-1) - (modified) lldb/source/Plugins/SymbolLocator/DebugSymbols/SymbolLocatorDebugSymbols.cpp (+4-1) - (modified) lldb/source/Target/Platform.cpp (+7-2) - (modified) lldb/source/Target/RemoteAwarePlatform.cpp (+8-7) ``````````diff diff --git a/lldb/include/lldb/Host/Host.h b/lldb/include/lldb/Host/Host.h index 4e19d15b06743..a9ab31d180591 100644 --- a/lldb/include/lldb/Host/Host.h +++ b/lldb/include/lldb/Host/Host.h @@ -195,65 +195,108 @@ class Host { static Status ShellExpandArguments(ProcessLaunchInfo &launch_info); /// Run a shell command. - /// \arg command shouldn't be empty - /// \arg working_dir Pass empty FileSpec to use the current working directory - /// \arg status_ptr Pass NULL if you don't want the process exit status - /// \arg signo_ptr Pass NULL if you don't want the signal that caused the - /// process to exit - /// \arg command_output Pass NULL if you don't want the command output - /// \arg hide_stderr if this is false, redirect stderr to stdout + /// \param[in] command + /// Command to execute, should not be empty. + /// \param[in] working_dir + /// Pass empty FileSpec to use the current working directory + /// \param[out] status_ptr + /// Pass nullptr if you don't want the process exit status + /// \param[out] signo_ptr + /// Pass nullptr if you don't want the signal that caused the + /// process to exit + /// \param[out] command_output + /// Pass nullptr if you don't want the command output + /// \param[out] error_output + /// Pass nullptr if you don't want the command error output + /// \param[in] timeout + /// Timeout duration to enforce + /// \param[in] run_in_shell + /// Run in a subshell, with glob expansion of args static Status RunShellCommand(llvm::StringRef command, const FileSpec &working_dir, int *status_ptr, int *signo_ptr, std::string *command_output, + std::string *error_output, const Timeout<std::micro> &timeout, - bool run_in_shell = true, - bool hide_stderr = false); + bool run_in_shell = true); /// Run a shell command. - /// \arg shell Pass an empty string if you want to use the default shell - /// interpreter \arg command \arg working_dir Pass empty FileSpec to use the - /// current working directory \arg status_ptr Pass NULL if you don't want - /// the process exit status \arg signo_ptr Pass NULL if you don't want the - /// signal that caused - /// the process to exit - /// \arg command_output Pass NULL if you don't want the command output - /// \arg hide_stderr If this is \b false, redirect stderr to stdout + /// \param[in] shell + /// Pass an empty string to use the default shell + /// \param[in] command + /// Command to execute, should not be empty. + /// \param[in] working_dir + /// Pass empty FileSpec to use the current working directory + /// \param[out] status_ptr + /// Pass nullptr if you don't want the process exit status + /// \param[out] signo_ptr + /// Pass nullptr if you don't want the signal that caused the + /// process to exit + /// \param[out] command_output + /// Pass nullptr if you don't want the command output + /// \param[out] error_output + /// Pass nullptr if you don't want the command error output + /// \param[in] timeout + /// Timeout duration to enforce + /// \param[in] run_in_shell + /// Run in a subshell, with glob expansion of args static Status RunShellCommand(llvm::StringRef shell, llvm::StringRef command, const FileSpec &working_dir, int *status_ptr, int *signo_ptr, std::string *command_output, + std::string *error_output, const Timeout<std::micro> &timeout, - bool run_in_shell = true, - bool hide_stderr = false); + bool run_in_shell = true); /// Run a shell command. - /// \arg working_dir Pass empty FileSpec to use the current working directory - /// \arg status_ptr Pass NULL if you don't want the process exit status - /// \arg signo_ptr Pass NULL if you don't want the signal that caused the - /// process to exit - /// \arg command_output Pass NULL if you don't want the command output - /// \arg hide_stderr if this is false, redirect stderr to stdout + /// \param[in] args + /// Command to execute + /// \param[in] working_dir + /// Pass empty FileSpec to use the current working directory + /// \param[out] status_ptr + /// Pass nullptr if you don't want the process exit status + /// \param[out] signo_ptr + /// Pass nullptr if you don't want the signal that caused the + /// process to exit + /// \param[out] command_output + /// Pass nullptr if you don't want the command output + /// \param[out] error_output + /// Pass nullptr if you don't want the command error output + /// \param[in] timeout + /// Timeout duration to enforce + /// \param[in] run_in_shell + /// Run in a subshell, with glob expansion of args static Status RunShellCommand(const Args &args, const FileSpec &working_dir, int *status_ptr, int *signo_ptr, std::string *command_output, + std::string *error_output, const Timeout<std::micro> &timeout, - bool run_in_shell = true, - bool hide_stderr = false); + bool run_in_shell = true); /// Run a shell command. - /// \arg shell Pass an empty string if you want to use the default - /// shell interpreter \arg command \arg working_dir Pass empty FileSpec to use - /// the current working directory \arg status_ptr Pass NULL if you don't - /// want the process exit status \arg signo_ptr Pass NULL if you don't - /// want the signal that caused the - /// process to exit - /// \arg command_output Pass NULL if you don't want the command output - /// \arg hide_stderr If this is \b false, redirect stderr to stdout + /// \param[in] shell + /// Pass an empty string to use the default shell + /// \param[in] args + /// Command to execute + /// \param[in] working_dir + /// Pass empty FileSpec to use the current working directory + /// \param[out] status_ptr + /// Pass nullptr if you don't want the process exit status + /// \param[out] signo_ptr + /// Pass nullptr if you don't want the signal that caused the + /// process to exit + /// \param[out] command_output + /// Pass nullptr if you don't want the command output + /// \param[out] error_output + /// Pass nullptr if you don't want the command error output + /// \param[in] timeout + /// Timeout duration to enforce + /// \param[in] run_in_shell + /// Run in a subshell, with glob expansion of args static Status RunShellCommand(llvm::StringRef shell, const Args &args, const FileSpec &working_dir, int *status_ptr, int *signo_ptr, std::string *command_output, + std::string *error_output, const Timeout<std::micro> &timeout, - bool run_in_shell = true, - bool hide_stderr = false); + bool run_in_shell = true); static llvm::Error OpenFileInExternalEditor(llvm::StringRef editor, const FileSpec &file_spec, diff --git a/lldb/include/lldb/Target/Platform.h b/lldb/include/lldb/Target/Platform.h index 637d4c37b90bc..ee79c946ad91d 100644 --- a/lldb/include/lldb/Target/Platform.h +++ b/lldb/include/lldb/Target/Platform.h @@ -677,6 +677,8 @@ class Platform : public PluginInterface { // the process to exit std::string *command_output, // Pass nullptr if you don't want the command output + std::string + *error_output, // Pass nullptr if you don't want the command output const Timeout<std::micro> &timeout); virtual lldb_private::Status RunShellCommand( @@ -688,6 +690,8 @@ class Platform : public PluginInterface { // the process to exit std::string *command_output, // Pass nullptr if you don't want the command output + std::string + *error_output, // Pass nullptr if you don't want the command output const Timeout<std::micro> &timeout); virtual void SetLocalCacheDirectory(const char *local); diff --git a/lldb/include/lldb/Target/RemoteAwarePlatform.h b/lldb/include/lldb/Target/RemoteAwarePlatform.h index de13b18f30d85..c1d8d9994654d 100644 --- a/lldb/include/lldb/Target/RemoteAwarePlatform.h +++ b/lldb/include/lldb/Target/RemoteAwarePlatform.h @@ -70,12 +70,13 @@ class RemoteAwarePlatform : public Platform { Status RunShellCommand(llvm::StringRef command, const FileSpec &working_dir, int *status_ptr, int *signo_ptr, - std::string *command_output, + std::string *command_output, std::string *error_output, const Timeout<std::micro> &timeout) override; Status RunShellCommand(llvm::StringRef interpreter, llvm::StringRef command, const FileSpec &working_dir, int *status_ptr, int *signo_ptr, std::string *command_output, + std::string *error, const Timeout<std::micro> &timeout) override; const char *GetHostname() override; diff --git a/lldb/source/API/SBPlatform.cpp b/lldb/source/API/SBPlatform.cpp index 9a0b47ce80336..5bab9d9e112fd 100644 --- a/lldb/source/API/SBPlatform.cpp +++ b/lldb/source/API/SBPlatform.cpp @@ -559,12 +559,13 @@ SBError SBPlatform::Run(SBPlatformShellCommand &shell_command) { if (!platform_working_dir.empty()) shell_command.SetWorkingDirectory(platform_working_dir.c_str()); } + std::string stderr_output; return platform_sp->RunShellCommand( shell_command.m_opaque_ptr->m_shell, command, FileSpec(shell_command.GetWorkingDirectory()), &shell_command.m_opaque_ptr->m_status, &shell_command.m_opaque_ptr->m_signo, - &shell_command.m_opaque_ptr->m_output, + &shell_command.m_opaque_ptr->m_output, &stderr_output, shell_command.m_opaque_ptr->m_timeout); }); } diff --git a/lldb/source/Commands/CommandObjectPlatform.cpp b/lldb/source/Commands/CommandObjectPlatform.cpp index 4865c48c82b7f..d7c87f36f6e2e 100644 --- a/lldb/source/Commands/CommandObjectPlatform.cpp +++ b/lldb/source/Commands/CommandObjectPlatform.cpp @@ -1706,13 +1706,16 @@ class CommandObjectPlatformShell : public CommandObjectRaw { if (platform_sp) { FileSpec working_dir{}; std::string output; + std::string error_output; int status = -1; int signo = -1; - error = (platform_sp->RunShellCommand(m_options.m_shell_interpreter, cmd, - working_dir, &status, &signo, - &output, m_options.m_timeout)); + error = (platform_sp->RunShellCommand( + m_options.m_shell_interpreter, cmd, working_dir, &status, &signo, + &output, &error_output, m_options.m_timeout)); if (!output.empty()) result.GetOutputStream().PutCString(output); + if (!error_output.empty()) + result.GetOutputStream().PutCString(error_output); if (status > 0) { if (signo > 0) { const char *signo_cstr = Host::GetSignalAsCString(signo); diff --git a/lldb/source/Host/common/Host.cpp b/lldb/source/Host/common/Host.cpp index 510f9c7696d12..8c0de586eaa7d 100644 --- a/lldb/source/Host/common/Host.cpp +++ b/lldb/source/Host/common/Host.cpp @@ -389,39 +389,43 @@ MonitorShellCommand(std::shared_ptr<ShellInfo> shell_info, lldb::pid_t pid, Status Host::RunShellCommand(llvm::StringRef command, const FileSpec &working_dir, int *status_ptr, int *signo_ptr, std::string *command_output_ptr, + std::string *command_error_ptr, const Timeout<std::micro> &timeout, - bool run_in_shell, bool hide_stderr) { + bool run_in_shell) { return RunShellCommand(llvm::StringRef(), Args(command), working_dir, - status_ptr, signo_ptr, command_output_ptr, timeout, - run_in_shell, hide_stderr); + status_ptr, signo_ptr, command_output_ptr, + command_error_ptr, timeout, run_in_shell); } Status Host::RunShellCommand(llvm::StringRef shell_path, llvm::StringRef command, const FileSpec &working_dir, int *status_ptr, int *signo_ptr, std::string *command_output_ptr, + std::string *command_error_ptr, const Timeout<std::micro> &timeout, - bool run_in_shell, bool hide_stderr) { + bool run_in_shell) { return RunShellCommand(shell_path, Args(command), working_dir, status_ptr, - signo_ptr, command_output_ptr, timeout, run_in_shell, - hide_stderr); + signo_ptr, command_output_ptr, command_error_ptr, + timeout); } Status Host::RunShellCommand(const Args &args, const FileSpec &working_dir, int *status_ptr, int *signo_ptr, std::string *command_output_ptr, + std::string *command_error_ptr, const Timeout<std::micro> &timeout, - bool run_in_shell, bool hide_stderr) { + bool run_in_shell) { return RunShellCommand(llvm::StringRef(), args, working_dir, status_ptr, - signo_ptr, command_output_ptr, timeout, run_in_shell, - hide_stderr); + signo_ptr, command_output_ptr, command_error_ptr, + timeout); } Status Host::RunShellCommand(llvm::StringRef shell_path, const Args &args, const FileSpec &working_dir, int *status_ptr, int *signo_ptr, std::string *command_output_ptr, + std::string *command_error_ptr, const Timeout<std::micro> &timeout, - bool run_in_shell, bool hide_stderr) { + bool run_in_shell) { Status error; ProcessLaunchInfo launch_info; launch_info.SetArchitecture(HostInfo::GetArchitecture()); @@ -448,9 +452,10 @@ Status Host::RunShellCommand(llvm::StringRef shell_path, const Args &args, if (working_dir) launch_info.SetWorkingDirectory(working_dir); llvm::SmallString<64> output_file_path; + llvm::SmallString<64> error_file_path; if (command_output_ptr) { - // Create a temporary file to get the stdout/stderr and redirect the output + // Create a temporary file to get the stdout and redirect the output // of the command into this file. We will later read this file if all goes // well and fill the data into "command_output_ptr" if (FileSpec tmpdir_file_spec = HostInfo::GetProcessTempDir()) { @@ -463,7 +468,22 @@ Status Host::RunShellCommand(llvm::StringRef shell_path, const Args &args, } } + if (command_error_ptr) { + // Create a temporary file to get the stderr and redirect the output + // of the command into this file. We will later read this file if all goes + // well and fill the data into "command_output_ptr" + if (FileSpec tmpdir_file_spec = HostInfo::GetProcessTempDir()) { + tmpdir_file_spec.AppendPathComponent("lldb-shell-error.%%%%%%"); + llvm::sys::fs::createUniqueFile(tmpdir_file_spec.GetPath(), + error_file_path); + } else { + llvm::sys::fs::createTemporaryFile("lldb-shell-error.%%%%%%", "", + error_file_path); + } + } + FileSpec output_file_spec(output_file_path.str()); + FileSpec error_file_spec(error_file_path.str()); // Set up file descriptors. launch_info.AppendSuppressFileAction(STDIN_FILENO, true, false); if (output_file_spec) @@ -472,8 +492,9 @@ Status Host::RunShellCommand(llvm::StringRef shell_path, const Args &args, else launch_info.AppendSuppressFileAction(STDOUT_FILENO, false, true); - if (output_file_spec && !hide_stderr) - launch_info.AppendDuplicateFileAction(STDOUT_FILENO, STDERR_FILENO); + if (error_file_spec) + launch_info.AppendOpenFileAction(STDERR_FILENO, error_file_spec, false, + true); else launch_info.AppendSuppressFileAction(STDERR_FILENO, false, true); @@ -524,10 +545,33 @@ Status Host::RunShellCommand(llvm::StringRef shell_path, const Args &args, } } } + if (command_error_ptr) { + command_error_ptr->clear(); + uint64_t file_size = + FileSystem::Instance().GetByteSize(error_file_spec); + if (file_size > 0) { + if (file_size > command_error_ptr->max_size()) { + error = Status::FromErrorStringWithFormat( + "shell command error output is too large to fit into a " + "std::string"); + } else { + WritableDataBufferSP Buffer = + FileSystem::Instance().CreateWritableDataBuffer( + error_file_spec); + if (error.Success()) + command_error_ptr->assign( + reinterpret_cast<char *>(Buffer->GetBytes()), + Buffer->GetByteSize()); + } + } + } } } - llvm::sys::fs::remove(output_file_spec.GetPath()); + if (output_file_spec) + llvm::sys::fs::remove(output_file_spec.GetPath()); + if (error_file_spec) + llvm::sys::fs::remove(error_file_spec.GetPath()); return error; } diff --git a/lldb/source/Host/macosx/objcxx/Host.mm b/lldb/source/Host/macosx/objcxx/Host.mm index f52b78f257ca8..4d6ad08e1ad74 100644 --- a/lldb/source/Host/macosx/objcxx/Host.mm +++ b/lldb/source/Host/macosx/objcxx/Host.mm @@ -1516,6 +1516,7 @@ static bool ShouldLaunchUsingXPC(ProcessLaunchInfo &launch_info) { int status; std::string output; + std::string error_output; FileSpec cwd(launch_info.GetWorkingDirectory()); if (!FileSystem::Instance().Exists(cwd)) { char *wd = getcwd(nullptr, 0); @@ -1530,10 +1531,9 @@ static bool ShouldLaunchUsingXPC(ProcessLaunchInfo &launch_info) { } } bool run_in_shell = true; - bool hide_stderr = true; Status e = RunShellCommand(expand_command, cwd, &status, nullptr, &output, - std::chrono::seconds(10), run_in_shell, hide_stderr); + &error_output, std::chrono::seconds(10), run_in_shell); if (e.Fail()) return e; diff --git a/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm b/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm index 63cef827a91c3..7f5acadffef94 100644 --- a/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm +++ b/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm @@ -452,12 +452,14 @@ static bool ResolveAndVerifyCandidateSupportDir(FileSpec &path) { int status = 0; int signo = 0; std::string output_str; + std::string error_str; // The first time after Xcode was updated or freshly installed, // xcrun can take surprisingly long to build up its database. auto timeout = std::chrono::seconds(60); bool run_in_shell = false; ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/184548 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
