Page MenuHomePhabricator

Delete ProcessLauncherPosix
ClosedPublic

Authored by labath on Jun 15 2017, 6:25 AM.

Details

Summary

ProcessLauncherPosix was using posix_spawn for launching the process,
but this function is not available on all platforms we support, and even
where it was avaialable, it did not support the full range of options we
require for launching (most importantly, launching in stop-on-entry
mode). For these reasons, the set of ifdefs around these functions has
grown untractably large, and we were forced to implement our own
launcher from more basic primitives anyway (ProcessLauncherPosixFork --
used on Linux, Android, and NetBSD).

Therefore, I remove this class, and move the relevant parts of the code
to the darwin-specific Host.mm file. This is the platform that code was
originally written for anyway, and it's the only platform where this
implementation makes sense (e.g. the lack of the "thread-specific
working directory" concept makes these functions racy on all other
platforms). This allows us to remove a lot of ifdefs and simplify the
code.

Effectively, the only change this introduces is that FreeBSD will now
use the fork-based launcher instead of posix_spawnp. That sholdn't be a
problem as this approach works at least on one other BSD-based system
already.

Event Timeline

labath created this revision.Jun 15 2017, 6:25 AM
krytarowski accepted this revision.Jun 15 2017, 8:11 AM

NetBSD part looks good. In next step we can rename ProcessLauncherPosixFork to ProcessLauncherPosix.

This revision is now accepted and ready to land.Jun 15 2017, 8:11 AM
rnk added a subscriber: rnk.Jun 15 2017, 10:22 AM

posix_spawn is considered the preferred, efficient, canonical way to launch processes on Mac. Are you sure you want to remove support for it?

jingham edited edge metadata.Jun 15 2017, 11:35 AM

This patch doesn't remove support for posix_spawn on OS X - it is moving to the macOS Host.mm. Kind of sad as posix_spawn was supposed to remove a lot of the platform-dependent and error-prone parts of process launching from the system. But you have to live with what you have...

Unfortunately, posix_spawn does not satisfy all our needs for launching processes on non-darwin platforms (according to its design rationale, that was never the intention). The most notable one is the "launch a process for debugging". Darwin seems to have added extensions which make it possible, so it makes sense to keep using it. However, it also makes the usage of that function non-portable. The only reason FreeBSD was able to get away with it was because it rolled it's own version of "launch for debug" code (source/Plugins/Process/FreeBSD/ProcessMonitor.cpp). After switching to a more generic launching primitive, that code could disappear.

So, overall, I sense no opposition to this from the darwin or netbsd side. I'll wait with it a while though, to see if the freebsd side wakes up.

emaste edited edge metadata.Jun 16 2017, 7:39 PM

FreeBSD build and test in progress.

emaste accepted this revision.Jun 17 2017, 3:59 AM

LGTM from the FreeBSD side. The launch code for FreeBSD came from the original (in-process) implementation that Linux and FreeBSD shared.

LGTM from the FreeBSD side. The launch code for FreeBSD came from the original (in-process) implementation that Linux and FreeBSD shared.

Thanks Ed.

If anyone's interested in archaeology, that launch code was then forked when linux went for lldb-server based debugging, and later it was made more generic and merged with another launch mechanism which was needed for android (no posix_spawn there). Finally it was moved to a common folder to enable NetBSD to use it. When freebsd switches to lldb-server, we can close the loop. :)

This revision was automatically updated to reflect the committed changes.