Page MenuHomePhabricator

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

Authored by JDevlieghere on Aug 4 2020, 11:42 PM.


Group Reviewers
Restricted Project

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).

Diff Detail


Event Timeline

JDevlieghere requested review of this revision.Aug 4 2020, 11:42 PM
JDevlieghere created this revision.
JDevlieghere edited the summary of this revision. (Show Details)

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.

mib added a subscriber: mib.Jan 29 2021, 11:05 PM