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

[David Holmes]

On Fri, 5 Mar 2021 17:01:26 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 one additional commit since the last revision:
> 
>   tidy up

Hi Thomas,

I appreciate the reworking to avoid vfork for the signal case, but have some concerns with the mechanism and I'm not sure a new mechanism is really needed. See comments below.

Thanks,
David

src/hotspot/os/posix/os_posix.cpp line 1804:

> 1802:   //  of signal handling.
> 1803:   const Thread* const t = Thread::current_or_null_safe();
> 1804:   const bool use_vfork = t != NULL && t->is_in_signal_handler() == false;

Nit: !t->is_in_signal_handler()

src/hotspot/os/posix/signals_posix.cpp line 573:

> 571:   InSignalHandlerMark() :
> 572:     _thread(Thread::current_or_null_safe()) {
> 573:     if (_thread) {

Style Nit: no implicit booleans - use `_thread != NULL`

src/hotspot/os/posix/signals_posix.cpp line 600:

> 598:   assert(info != NULL && ucVoid != NULL, "sanity");
> 599: 
> 600:   InSignalHandlerMark ishm;

Is this going to behave as expected if we actually crash during signal processing and so re-enter this routine with a signal mark already active?

src/hotspot/share/utilities/macros.hpp line 439:

> 437: #endif
> 438: #define POSIX_ONLY(code) code
> 439: #define NOT_POSIX(code)

Not sure this makes sense. At the moment only Windows is not-POSIX. If in the future we had another non-POSIX platform then I find it very unlikely that it would use the same code as Windows. So I would not like to see NOT_POSIX used incorrectly where we really only mean WINDOWS.

src/hotspot/share/runtime/thread.hpp line 826:

> 824:   static void SpinRelease(volatile int * Lock);
> 825: 
> 826: #ifdef POSIX

I hate seeing this in shared code. It really belongs in platform-dependent thread code - but osThread_posix doesn't exist. :(

But do we need this - can't the existing use_vfork_if_available parameter instead be renamed and interpreted as not_in_a_signal_handler?

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

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