JDevlieghere created this revision.
JDevlieghere added a reviewer: LLDB.
JDevlieghere added a project: LLDB.
JDevlieghere requested review of this revision.
The comment associate with `m_arguments ` in `ProcessInfo.h` states that it
holds "all program arguments except argv[0]". Unfortunately, this assumption
often does not hold, and even worse, half of the code depends on it being the
case, while the other half depends on the opposite. Miraculously we seem to
have managed to keep the two from interacting with each other, but it's only a
matter of time before this blows up. Indeed, I already ran into this issue in
D85235 <https://reviews.llvm.org/D85235>.
If I understand correctly, we want to exclude `argv[0]` from the argument list
because some (host) platforms allow you to launch an executable and specify a
different argv[0]. As far as I can tell, no launching code is actually honoring
that distinction. That said, we have a bunch of infrastructure is in place to
make this possible, so it would be a pity to ignore it instead of fixing it. On
the other hand, not differentiating between the two, and possibly including the
executable in the argument list would simply the code a lot. Currently there's
a bunch of code spread out to do the splitting and concatenating, each
implementation with its own assumptions and potential for bugs.
I suspect this has grown to become an issue because the API makes it easy to do
the wrong thing. Most of the APIs to launch the binary expect all expect
`char**` array, which is something that the `Args` class conveniently provides.
Indeed, all the launch code takes `ProcessInfo::GetArguments()` (which is
supposed to exclude `argv[0]`), converts it to a char** and passes it to the
posix_spawn et al.
I decided I'd get some input from the community before spending any more time
on this. This patch is the furthest I've gotten and passes the test suite on
macOS, though I'm sure there's a bunch of edge cases that this doesn't handle
correctly (yet).
Repository:
rLLDB LLDB
https://reviews.llvm.org/D85275
Files:
lldb/include/lldb/Utility/ProcessInfo.h
lldb/source/API/SBLaunchInfo.cpp
lldb/source/API/SBProcess.cpp
lldb/source/API/SBTarget.cpp
lldb/source/Commands/CommandObjectProcess.cpp
lldb/source/Host/common/MonitoringProcessLauncher.cpp
lldb/source/Host/common/ProcessLaunchInfo.cpp
lldb/source/Host/macosx/objcxx/Host.mm
lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
lldb/source/Host/windows/ProcessLauncherWindows.cpp
lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
lldb/source/Target/Target.cpp
lldb/source/Utility/ProcessInfo.cpp
Index: lldb/source/Utility/ProcessInfo.cpp
===================================================================
--- lldb/source/Utility/ProcessInfo.cpp
+++ lldb/source/Utility/ProcessInfo.cpp
@@ -59,21 +59,6 @@
s.Format("Environment:\n{0}", m_environment);
}
-void ProcessInfo::SetExecutableFile(const FileSpec &exe_file,
- bool add_exe_file_as_first_arg) {
- if (exe_file) {
- m_executable = exe_file;
- if (add_exe_file_as_first_arg) {
- llvm::SmallString<128> filename;
- exe_file.GetPath(filename);
- if (!filename.empty())
- m_arguments.InsertArgumentAtIndex(0, filename);
- }
- } else {
- m_executable.Clear();
- }
-}
-
llvm::StringRef ProcessInfo::GetArg0() const { return m_arg0; }
void ProcessInfo::SetArg0(llvm::StringRef arg) { m_arg0 = std::string(arg); }
@@ -81,35 +66,51 @@
void ProcessInfo::SetArguments(char const **argv,
bool first_arg_is_executable) {
m_arguments.SetArguments(argv);
-
- // Is the first argument the executable?
if (first_arg_is_executable) {
- const char *first_arg = m_arguments.GetArgumentAtIndex(0);
- if (first_arg) {
- // Yes the first argument is an executable, set it as the executable in
- // the launch options. Don't resolve the file path as the path could be a
- // remote platform path
+ if (const char *first_arg = m_arguments.GetArgumentAtIndex(0)) {
+ // Don't resolve the file path as the path could be a remote platform
+ // path.
m_executable.SetFile(first_arg, FileSpec::Style::native);
+ m_arguments.DeleteArgumentAtIndex(0);
}
}
}
void ProcessInfo::SetArguments(const Args &args, bool first_arg_is_executable) {
- // Copy all arguments
m_arguments = args;
-
- // Is the first argument the executable?
if (first_arg_is_executable) {
- const char *first_arg = m_arguments.GetArgumentAtIndex(0);
- if (first_arg) {
- // Yes the first argument is an executable, set it as the executable in
- // the launch options. Don't resolve the file path as the path could be a
- // remote platform path
+ if (const char *first_arg = m_arguments.GetArgumentAtIndex(0)) {
+ // Don't resolve the file path as the path could be a remote platform
+ // path.
m_executable.SetFile(first_arg, FileSpec::Style::native);
+ m_arguments.DeleteArgumentAtIndex(0);
}
}
}
+Args ProcessInfo::GetAllArguments() const {
+ Args args = m_arguments;
+
+ // FIXME: Make this an assert once all legacy code has been fixed.
+ if (args.GetArgumentCount() != 0) {
+ llvm::StringRef arg0 = args.GetArgumentAtIndex(0);
+ if (arg0 == m_arg0)
+ return args;
+ if (arg0 == m_executable.GetPath())
+ return args;
+ }
+
+ if (!m_arg0.empty()) {
+ args.InsertArgumentAtIndex(0, m_arg0);
+ }
+
+ else if (m_executable) {
+ args.InsertArgumentAtIndex(0, m_executable.GetPath());
+ }
+
+ return args;
+}
+
void ProcessInstanceInfo::Dump(Stream &s, UserIDResolver &resolver) const {
if (m_pid != LLDB_INVALID_PROCESS_ID)
s.Printf(" pid = %" PRIu64 "\n", m_pid);
Index: lldb/source/Target/Target.cpp
===================================================================
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2714,7 +2714,7 @@
if (is_main_executable) {
platform_sp->SetFilePermissions(remote_file, 0700);
if (launch_info)
- launch_info->SetExecutableFile(remote_file, false);
+ launch_info->SetExecutableFile(remote_file);
}
} else
break;
Index: lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
===================================================================
--- lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -542,9 +542,7 @@
info.SetArchitecture(GetArchitecture());
lldb::ModuleSP module_sp = GetTarget().GetExecutableModule();
if (module_sp) {
- const bool add_exe_file_as_first_arg = false;
- info.SetExecutableFile(GetTarget().GetExecutableModule()->GetFileSpec(),
- add_exe_file_as_first_arg);
+ info.SetExecutableFile(GetTarget().GetExecutableModule()->GetFileSpec());
}
return true;
}
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -3425,7 +3425,7 @@
&bufsize, NULL, 0) == 0 && bufsize > 0) {
if (processInfo.kp_proc.p_flag & P_TRANSLATED) {
FileSpec rosetta_debugserver("/Library/Apple/usr/libexec/oah/debugserver");
- debugserver_launch_info.SetExecutableFile(rosetta_debugserver, false);
+ debugserver_launch_info.SetExecutableFile(rosetta_debugserver);
}
}
#endif
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -777,34 +777,21 @@
int GDBRemoteCommunicationClient::SendArgumentsPacket(
const ProcessLaunchInfo &launch_info) {
+ Args launch_args = launch_info.GetArguments();
+
// Since we don't get the send argv0 separate from the executable path, we
// need to make sure to use the actual executable path found in the
// launch_info...
- std::vector<const char *> argv;
FileSpec exe_file = launch_info.GetExecutableFile();
- std::string exe_path;
- const char *arg = nullptr;
- const Args &launch_args = launch_info.GetArguments();
- if (exe_file)
- exe_path = exe_file.GetPath(false);
- else {
- arg = launch_args.GetArgumentAtIndex(0);
- if (arg)
- exe_path = arg;
- }
- if (!exe_path.empty()) {
- argv.push_back(exe_path.c_str());
- for (uint32_t i = 1; (arg = launch_args.GetArgumentAtIndex(i)) != nullptr;
- ++i) {
- if (arg)
- argv.push_back(arg);
- }
+ if (exe_file) {
+ launch_args.InsertArgumentAtIndex(0, exe_file.GetPath(false));
}
- if (!argv.empty()) {
+
+ if (!launch_args.empty()) {
StreamString packet;
packet.PutChar('A');
- for (size_t i = 0, n = argv.size(); i < n; ++i) {
- arg = argv[i];
+ for (size_t i = 0, n = launch_args.GetArgumentCount(); i < n; ++i) {
+ const char *arg = launch_args.GetArgumentAtIndex(i);
const int arg_len = strlen(arg);
if (i > 0)
packet.PutChar(',');
Index: lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
===================================================================
--- lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
+++ lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
@@ -914,9 +914,7 @@
info.SetArchitecture(GetArchitecture());
lldb::ModuleSP module_sp = GetTarget().GetExecutableModule();
if (module_sp) {
- const bool add_exe_file_as_first_arg = false;
- info.SetExecutableFile(GetTarget().GetExecutableModule()->GetFileSpec(),
- add_exe_file_as_first_arg);
+ info.SetExecutableFile(GetTarget().GetExecutableModule()->GetFileSpec());
}
return true;
}
Index: lldb/source/Host/windows/ProcessLauncherWindows.cpp
===================================================================
--- lldb/source/Host/windows/ProcessLauncherWindows.cpp
+++ lldb/source/Host/windows/ProcessLauncherWindows.cpp
@@ -104,7 +104,7 @@
executable = launch_info.GetExecutableFile().GetPath();
std::wstring wcommandLine;
- GetFlattenedWindowsCommandString(launch_info.GetArguments(), wcommandLine);
+ GetFlattenedWindowsCommandString(launch_info.GetAllArguments(), wcommandLine);
std::wstring wexecutable, wworkingDirectory;
llvm::ConvertUTF8toWide(executable, wexecutable);
Index: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
===================================================================
--- lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -115,7 +115,7 @@
}
}
- const char **argv = info.GetArguments().GetConstArgumentVector();
+ const char **argv = info.GetAllArguments().GetConstArgumentVector();
// Change working directory
if (info.GetWorkingDirectory() &&
Index: lldb/source/Host/macosx/objcxx/Host.mm
===================================================================
--- lldb/source/Host/macosx/objcxx/Host.mm
+++ lldb/source/Host/macosx/objcxx/Host.mm
@@ -243,7 +243,7 @@
command.PutCString(" -- ");
- const char **argv = launch_info.GetArguments().GetConstArgumentVector();
+ const char **argv = launch_info.GetAllArguments().GetConstArgumentVector();
if (argv) {
for (size_t i = 0; argv[i] != NULL; ++i) {
if (i == 0)
@@ -1107,20 +1107,15 @@
}
}
- const char *tmp_argv[2];
- char *const *argv = const_cast<char *const *>(
- launch_info.GetArguments().GetConstArgumentVector());
- Environment::Envp envp = launch_info.GetEnvironment().getEnvp();
- if (argv == NULL) {
- // posix_spawn gets very unhappy if it doesn't have at least the program
- // name in argv[0]. One of the side affects I have noticed is the
- // environment
- // variables don't make it into the child process if "argv == NULL"!!!
- tmp_argv[0] = exe_path;
- tmp_argv[1] = NULL;
- argv = const_cast<char *const *>(tmp_argv);
- }
+ Args args = launch_info.GetAllArguments();
+ // posix_spawn gets very unhappy if it doesn't have at least the program name
+ // in argv[0]. One of the side effects when this happens is that environment
+ // variables don't make it into the child process.
+ if (args.GetArgumentCount() == 0)
+ args.AppendArgument(exe_path);
+ char *const *argv = const_cast<char *const *>(args.GetConstArgumentVector());
+ Environment::Envp envp = launch_info.GetEnvironment().getEnvp();
FileSpec working_dir{launch_info.GetWorkingDirectory()};
if (working_dir) {
// Set the working directory on this thread only
Index: lldb/source/Host/common/ProcessLaunchInfo.cpp
===================================================================
--- lldb/source/Host/common/ProcessLaunchInfo.cpp
+++ lldb/source/Host/common/ProcessLaunchInfo.cpp
@@ -255,7 +255,6 @@
return false;
Args shell_arguments;
std::string safe_arg;
- shell_arguments.AppendArgument(shell_executable);
const llvm::Triple &triple = GetArchitecture().GetTriple();
if (triple.getOS() == llvm::Triple::Win32 &&
!triple.isWindowsCygwinEnvironment())
Index: lldb/source/Host/common/MonitoringProcessLauncher.cpp
===================================================================
--- lldb/source/Host/common/MonitoringProcessLauncher.cpp
+++ lldb/source/Host/common/MonitoringProcessLauncher.cpp
@@ -43,7 +43,7 @@
return HostProcess();
}
- resolved_info.SetExecutableFile(exe_spec, false);
+ resolved_info.SetExecutableFile(exe_spec);
assert(!resolved_info.GetFlags().Test(eLaunchFlagLaunchInTTY));
HostProcess process =
Index: lldb/source/Commands/CommandObjectProcess.cpp
===================================================================
--- lldb/source/Commands/CommandObjectProcess.cpp
+++ lldb/source/Commands/CommandObjectProcess.cpp
@@ -195,15 +195,11 @@
m_options.launch_info.GetEnvironment().insert(target_env.begin(),
target_env.end());
- if (!target_settings_argv0.empty()) {
- m_options.launch_info.GetArguments().AppendArgument(
- target_settings_argv0);
- m_options.launch_info.SetExecutableFile(
- exe_module_sp->GetPlatformFileSpec(), false);
- } else {
- m_options.launch_info.SetExecutableFile(
- exe_module_sp->GetPlatformFileSpec(), true);
- }
+ if (!target_settings_argv0.empty())
+ m_options.launch_info.SetArg0(target_settings_argv0);
+
+ m_options.launch_info.SetExecutableFile(
+ exe_module_sp->GetPlatformFileSpec());
if (launch_args.GetArgumentCount() == 0) {
m_options.launch_info.GetArguments().AppendArguments(
Index: lldb/source/API/SBTarget.cpp
===================================================================
--- lldb/source/API/SBTarget.cpp
+++ lldb/source/API/SBTarget.cpp
@@ -371,7 +371,7 @@
Module *exe_module = target_sp->GetExecutableModulePointer();
if (exe_module)
- launch_info.SetExecutableFile(exe_module->GetPlatformFileSpec(), true);
+ launch_info.SetExecutableFile(exe_module->GetPlatformFileSpec());
if (argv) {
launch_info.GetArguments().AppendArguments(argv);
} else {
@@ -429,9 +429,8 @@
lldb_private::ProcessLaunchInfo launch_info = sb_launch_info.ref();
if (!launch_info.GetExecutableFile()) {
- Module *exe_module = target_sp->GetExecutableModulePointer();
- if (exe_module)
- launch_info.SetExecutableFile(exe_module->GetPlatformFileSpec(), true);
+ if (Module *exe_module = target_sp->GetExecutableModulePointer())
+ launch_info.SetExecutableFile(exe_module->GetPlatformFileSpec());
}
const ArchSpec &arch_spec = target_sp->GetArchitecture();
Index: lldb/source/API/SBProcess.cpp
===================================================================
--- lldb/source/API/SBProcess.cpp
+++ lldb/source/API/SBProcess.cpp
@@ -150,7 +150,7 @@
FileSpec(working_directory), launch_flags);
Module *exe_module = process_sp->GetTarget().GetExecutableModulePointer();
if (exe_module)
- launch_info.SetExecutableFile(exe_module->GetPlatformFileSpec(), true);
+ launch_info.SetExecutableFile(exe_module->GetPlatformFileSpec());
if (argv)
launch_info.GetArguments().AppendArguments(argv);
if (envp)
Index: lldb/source/API/SBLaunchInfo.cpp
===================================================================
--- lldb/source/API/SBLaunchInfo.cpp
+++ lldb/source/API/SBLaunchInfo.cpp
@@ -121,7 +121,7 @@
LLDB_RECORD_METHOD(void, SBLaunchInfo, SetExecutableFile,
(lldb::SBFileSpec, bool), exe_file, add_as_first_arg);
- m_opaque_sp->SetExecutableFile(exe_file.ref(), add_as_first_arg);
+ m_opaque_sp->SetExecutableFile(exe_file.ref());
}
SBListener SBLaunchInfo::GetListener() {
Index: lldb/include/lldb/Utility/ProcessInfo.h
===================================================================
--- lldb/include/lldb/Utility/ProcessInfo.h
+++ lldb/include/lldb/Utility/ProcessInfo.h
@@ -42,8 +42,7 @@
FileSpec &GetExecutableFile() { return m_executable; }
- void SetExecutableFile(const FileSpec &exe_file,
- bool add_exe_file_as_first_arg);
+ void SetExecutableFile(const FileSpec &exe_file) { m_executable = exe_file; }
const FileSpec &GetExecutableFile() const { return m_executable; }
@@ -77,6 +76,8 @@
const Args &GetArguments() const { return m_arguments; }
+ Args GetAllArguments() const;
+
llvm::StringRef GetArg0() const;
void SetArg0(llvm::StringRef arg);
@@ -90,11 +91,14 @@
protected:
template <class T> friend struct llvm::yaml::MappingTraits;
+ /// Resolved platform executable.
FileSpec m_executable;
- std::string m_arg0; // argv[0] if supported. If empty, then use m_executable.
- // Not all process plug-ins support specifying an argv[0] that differs from
- // the resolved platform executable (which is in m_executable)
- Args m_arguments; // All program arguments except argv[0]
+ /// argv[0] if supported. If empty, then use m_executable. Not all process
+ /// plug-ins support specifying an argv[0] that differs from the resolved
+ /// platform executable (m_executable).
+ std::string m_arg0;
+ /// All program arguments except argv[0].
+ Args m_arguments;
Environment m_environment;
uint32_t m_uid;
uint32_t m_gid;
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
