RFR: JDK-8262955: Unify os::fork_and_exec() across Posix platforms [v4]

[David Holmes]

On Mon, 8 Mar 2021 08:34:23 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> `os::fork_and_exec()` can be used from within the hotspot to start a child process. It is only called in fatal situations, in two cases:
>> a) to automatically start a debugger when ShowMessageBoxOnError is specified (uses *fork*())
>> b) to start a caller provided binary on OOM if -XX:OnOutOfMemoryError is specified (uses *vfork*())
>> 
>> The variants for AIX, Linux, Bsd are almost completely identical. So, this function can be unified under posix.
>> 
>> In addition to that, this patch does a number of small changes:
>> 
>> 1) Before, whether we would vfork() only on Linux and only for case (b). I changed this to always use vfork unconditionally, on all platforms, because: 
>>   - even though vfork() can be unsafe, the way we use it - calling  vfork()->exec()->_exit() with no intermediate steps - is safe.
>>   - Using vfork is good for OOM situations on all platforms, not just Linux, and also for starting the debugger in non-OOM cases. Keep in mind that we do this only for cases where the parent VM is about to die, so even if it were unsafe, the damage would be limited.
>> 2) I added a comment to the function to not use it outside of fatal error situations.
>> 3) I added a posix wrapper for getting the environ pointer, to hide MacOS specifics, and used it in two places to unify that coding.
>> 4) consistently used global scope :: for posix APIs.
>> 
>> Note that if we wanted to make os::fork_and_exec() a first class function, always safe to use, we should modify it to at least not leak any parent process file descriptors. Possibly safest would be to completely rewrite this function and use posix_spawn(). posix_spawn() we use in Runtime.exec() by default since JDK 13 (1). But as long as this is spawned by only dying VMs I think this function is fine.
>> 
>> ----
>> 
>> Tests: GAs, manual tests using -XX:ShowMessageBoxOnError
>
> Thomas Stuefe has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Restore use-vfork parameter
>  - Revert last commit

Hi Thomas,

I am fine with this version of the refactoring.

Thanks,
David

src/hotspot/os/windows/os_windows.cpp line 5517:

> 5515: // Run the specified command in a separate process. Return its exit value,
> 5516: // or -1 on failure (e.g. can't create a new process).
> 5517: int os::fork_and_exec(const char* cmd, bool dummy /* ignored */) {

You could just have `bool _ignored`

-------------

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2810