[RFC/WIP] Fix the assumption that m_arguments in ProcessInfo does not include arg0

[JDevlieghere]

Summary

The comment associated 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.

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 a 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** array and passes it to the posix_spawn et al.

I decided I'd get some input from the community before spending much 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).

[labath]

I agree this should be cleaned up.

I don't have any strong opinions on how this should work. IIUC, this patch goes in direction of making m_args always not contain argv[0]. That is probably fine -- if nothing else, it makes the class match the lldb settings (which have argv[0] as a separate entity). But maybe matching the settings is not the most important thing, and I can see how making argv[0] a part of the args array could simplify other things...

In any case, we should add some unit tests for the ProcessInfo class which demonstrates how these apis are supposed to work.

lldb/include/lldb/Utility/ProcessInfo.h

public:
   void Clear();
 
   const char *GetName() const;
 
   llvm::StringRef GetNameAsStringRef() const;
 
   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; }
 
   uint32_t GetUserID() const { return m_uid; }
 
   uint32_t GetGroupID() const { return m_gid; }
 
   bool UserIDIsValid() const { return m_uid != UINT32_MAX; }
public:
   bool ProcessIDIsValid() const { return m_pid != LLDB_INVALID_PROCESS_ID; }
 
   void Dump(Stream &s, Platform *platform) const;
 
   Args &GetArguments() { return m_arguments; }
 
   const Args &GetArguments() const { return m_arguments; }
 
+  Args GetAllArguments() const;
+
   llvm::StringRef GetArg0() const;
 
   void SetArg0(llvm::StringRef arg);
 
   void SetArguments(const Args &args, bool first_arg_is_executable);
 
   void SetArguments(char const **argv, bool first_arg_is_executable);
 
   Environment &GetEnvironment() { return m_environment; }
   const Environment &GetEnvironment() const { return m_environment; }
 
 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;
   ArchSpec m_arch;
   lldb::pid_t m_pid;
 };
 
 // ProcessInstanceInfo

lldb/source/API/SBLaunchInfo.cpp

SBFileSpec SBLaunchInfo::GetExecutableFile() {
   return LLDB_RECORD_RESULT(SBFileSpec(m_opaque_sp->GetExecutableFile()));
 }
 
 void SBLaunchInfo::SetExecutableFile(SBFileSpec exe_file,
                                      bool add_as_first_arg) {
   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() {
   LLDB_RECORD_METHOD_NO_ARGS(lldb::SBListener, SBLaunchInfo, GetListener);
 
   return LLDB_RECORD_RESULT(SBListener(m_opaque_sp->GetListener()));
 }
 

lldb/source/API/SBProcess.cpp

if (process_sp) {
     if (process_sp->GetState() == eStateConnected) {
       if (stop_at_entry)
         launch_flags |= eLaunchFlagStopAtEntry;
       ProcessLaunchInfo launch_info(FileSpec(stdin_path), FileSpec(stdout_path),
                                     FileSpec(stderr_path),
                                     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)
         launch_info.GetEnvironment() = Environment(envp);
       error.SetError(process_sp->Launch(launch_info));
     } else {
       error.SetErrorString("must be in eStateConnected to call RemoteLaunch");
     }

lldb/source/API/SBTarget.cpp

if (getenv("LLDB_LAUNCH_FLAG_DISABLE_STDIO"))
       launch_flags |= eLaunchFlagDisableSTDIO;
 
     ProcessLaunchInfo launch_info(FileSpec(stdin_path), FileSpec(stdout_path),
                                   FileSpec(stderr_path),
                                   FileSpec(working_directory), launch_flags);
 
     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 {
       auto default_launch_info = target_sp->GetProcessLaunchInfo();
       launch_info.GetArguments().AppendArguments(
           default_launch_info.GetArguments());
     }
     if (envp) {
StateType state = eStateInvalid;
           return LLDB_RECORD_RESULT(sb_process);
         }
       }
     }
 
     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();
     if (arch_spec.IsValid())
       launch_info.GetArchitecture() = arch_spec;
 
     error.SetError(target_sp->Launch(launch_info, nullptr));
     sb_launch_info.set_ref(launch_info);

lldb/source/Commands/CommandObjectProcess.cpp

bool DoExecute(Args &launch_args, CommandReturnObject &result) override {
     if (target->GetDisableSTDIO())
       m_options.launch_info.GetFlags().Set(eLaunchFlagDisableSTDIO);
 
     // Merge the launch info environment with the target environment.
     Environment target_env = target->GetEnvironment();
     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);
+
+        exe_module_sp->GetPlatformFileSpec());
 
     if (launch_args.GetArgumentCount() == 0) {
       m_options.launch_info.GetArguments().AppendArguments(
           target->GetProcessLaunchInfo().GetArguments());
     } else {
       m_options.launch_info.GetArguments().AppendArguments(launch_args);
       // Save the arguments for subsequent runs in the current target.
       target->SetRunArguments(launch_args);

lldb/source/Host/common/MonitoringProcessLauncher.cpp

if (!fs.Exists(exe_spec))
     FileSystem::Instance().ResolveExecutableLocation(exe_spec);
 
   if (!fs.Exists(exe_spec)) {
     error.SetErrorStringWithFormatv("executable doesn't exist: '{0}'",
                                     exe_spec);
     return HostProcess();
   }
 
-  resolved_info.SetExecutableFile(exe_spec, false);
+  resolved_info.SetExecutableFile(exe_spec);
   assert(!resolved_info.GetFlags().Test(eLaunchFlagLaunchInTTY));
 
   HostProcess process =
       m_delegate_launcher->LaunchProcess(resolved_info, error);
 
   if (process.GetProcessId() != LLDB_INVALID_PROCESS_ID) {
     Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
 

lldb/source/Host/common/ProcessLaunchInfo.cpp

if (GetFlags().Test(eLaunchFlagLaunchInShell)) {
     if (m_shell) {
       std::string shell_executable = m_shell.GetPath();
 
       const char **argv = GetArguments().GetConstArgumentVector();
       if (argv == nullptr || argv[0] == nullptr)
         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())
         shell_arguments.AppendArgument(llvm::StringRef("/C"));
       else
         shell_arguments.AppendArgument(llvm::StringRef("-c"));
 
       StreamString shell_command;

lldb/source/Host/macosx/objcxx/Host.mm

LaunchInNewTerminalWithAppleScript(const char *exe_path,
   for (const auto &KV : launch_info.GetEnvironment()) {
     auto host_entry = host_env.find(KV.first());
     if (host_entry == host_env.end() || host_entry->second != KV.second)
       command.Format(" --env='{0}'", Environment::compose(KV));
   }
 
   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)
         command.Printf(" '%s'", exe_path);
       else
         command.Printf(" '%s'", argv[i]);
     }
   } else {
if (is_graphical && launch_info.GetFlags().Test(eLaunchFlagDebug)) {
     error.SetError(setup_posix_spawn_responsible_flag(&attr), eErrorTypePOSIX);
     if (error.Fail()) {
       LLDB_LOG(log, "error: {0}, setup_posix_spawn_responsible_flag(&attr)",
                error);
       return error;
     }
   }
 
-  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
     if (__pthread_chdir(working_dir.GetCString()) < 0) {
       if (errno == ENOENT) {
         error.SetErrorStringWithFormat("No such file or directory: %s",
                                        working_dir.GetCString());
       } else if (errno == ENOTDIR) {

lldb/source/Host/posix/ProcessLauncherPosixFork.cpp

case FileAction::eFileActionOpen:
       DupDescriptor(error_fd, action.GetFileSpec(), action.GetFD(),
                     action.GetActionArgument());
       break;
     case FileAction::eFileActionNone:
       break;
     }
   }
 
-  const char **argv = info.GetArguments().GetConstArgumentVector();
+  const char **argv = info.GetAllArguments().GetConstArgumentVector();
 
   // Change working directory
   if (info.GetWorkingDirectory() &&
       0 != ::chdir(info.GetWorkingDirectory().GetCString()))
     ExitWithError(error_fd, "chdir");
 
   DisableASLRIfRequested(error_fd, info);
   Environment env = info.GetEnvironment();

lldb/source/Host/windows/ProcessLauncherWindows.cpp

if (launch_info.GetFlags().Test(eLaunchFlagDisableSTDIO))
     flags &= ~CREATE_NEW_CONSOLE;
 
   LPVOID env_block = nullptr;
   ::CreateEnvironmentBuffer(launch_info.GetEnvironment(), environment);
   env_block = environment.data();
 
   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);
   llvm::ConvertUTF8toWide(launch_info.GetWorkingDirectory().GetCString(),
                           wworkingDirectory);
   // If the command line is empty, it's best to pass a null pointer to tell
   // CreateProcessW to use the executable name as the command line.  If the
   // command line is not empty, its contents may be modified by CreateProcessW.

lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp

 }
 
 bool ProcessElfCore::GetProcessInfo(ProcessInstanceInfo &info) {
   info.Clear();
   info.SetProcessID(GetID());
   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;
 }

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

bool GDBRemoteCommunicationClient::GetLaunchSuccess(std::string &error_str) {
   } else {
     error_str.assign("timed out waiting for app to launch");
   }
   return false;
 }
 
 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 (!argv.empty()) {
+  if (exe_file) {
+    launch_args.InsertArgumentAtIndex(0, exe_file.GetPath(false));
+
+  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(',');
       packet.Printf("%i,%i,", arg_len * 2, (int)i);
       packet.PutBytesAsRawHex8(arg, arg_len);
     }
 
     StringExtractorGDBRemote response;

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

     int mib[] = { CTL_KERN, KERN_PROC, KERN_PROC_PID,
                   static_cast<int>(process_info.GetProcessID()) };
     struct kinfo_proc processInfo;
     size_t bufsize = sizeof(processInfo);
     if (sysctl(mib, (unsigned)(sizeof(mib)/sizeof(int)), &processInfo,
                &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
 
     int communication_fd = -1;
 #ifdef USE_SOCKETPAIR_FOR_LOCAL_CONNECTION
     // Use a socketpair on non-Windows systems for security and performance
     // reasons.

lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp

 }
 
 bool ProcessMinidump::GetProcessInfo(ProcessInstanceInfo &info) {
   info.Clear();
   info.SetProcessID(GetID());
   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;
 }
 
 // For minidumps there's no runtime generated code so we don't need JITLoader(s)
 // Avoiding them will also speed up minidump loading since JITLoaders normally
 // try to set up symbolic breakpoints, which in turn may force loading more
 // debug information than needed.

lldb/source/Target/Target.cpp

               }
               if (remote_file) {
                 error = platform_sp->Install(local_file, remote_file);
                 if (error.Success()) {
                   module_sp->SetPlatformFileSpec(remote_file);
                   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;
               }
             }
           }
         }
       }

lldb/source/Utility/ProcessInfo.cpp

void ProcessInfo::Dump(Stream &s, Platform *platform) const {
   s << "\n";
 
   s << "Arguments:\n";
   m_arguments.Dump(s);
 
   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); }
 
 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);
 
   if (m_parent_pid != LLDB_INVALID_PROCESS_ID)
     s.Printf(" parent = %" PRIu64 "\n", m_parent_pid);