[Lldb-commits] [PATCH] D85275: [RFC/WIP] Fix the assumption that m_arguments in ProcessInfo does not include arg0

[Jonas Devlieghere via Phabricator via lldb-commits]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D85275.283131.patch
Type: text/x-patch
Size: 16149 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20200805/f6ea099d/attachment-0001.bin>