RFR: 8304939: os::win32::exit_process_or_thread should be marked noreturn [v3]

[David Holmes]

On Tue, 24 Oct 2023 09:46:51 GMT, Julian Waters <jwaters at openjdk.org> wrote:

>> On Windows we have a non-trivial function (exit_process_or_thread) that provides the implementation of various functions like os::die, os::abort, &etc. Those os functions are marked as noreturn, so this implementation helper should also be noreturn.
>> 
>> The current change does several things around exit_process_or_thread, which is moved out of the os::win32 class since it does not need access to anything in that class other than the Ept enum, which is also moved to os_windows.cpp as well. The signature is changed to void, since exit_process_or_thread's current return value is simply the exit code parameter that it is passed, and to qualify as noreturn it should not return its current value of int. The only usage of this return value is in thread_native_entry, and this usage can easily be replaced by returning the res local that exit_process_or_thread is passed. For this os::infinite_sleep takes the place of the return statement in exit_process_or_thread, to make sure it qualifies as noreturn.
>> 
>> Although not mentioned in the title, raise_fail_fast has also been fixed. RaiseFailFastException is not itself noreturn, so os::infinite_sleep has been added there as well, to make raise_fail_fast qualify as a noreturn method
>> 
>> All the changes mentioned above fixes any Undefined Behaviour on Windows with regards to noreturn, by making all code marked noreturn (os::die() os::exit() etc) never return. This is not a problem on other platforms since their implementations of os::abort or os::die etc already call noreturn methods.
>> 
>> This fix also fixes compilation on gcc windows, which warns about noreturn methods returning prior to this changeset
>
> Julian Waters has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge branch 'openjdk:master' into noreturn
>  - Minor Style Change in os_windows.cpp
>  - 8304939

Supporting no_return seems a reasonable thing to do.

General complaint is that there is too much incidental change in the PR that to me adds no value. 

Some specific objections below.

Thanks

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

> 892:   sleep:            // sleep forever ...
> 893:     ::sleep(100);   // ... 100 seconds at a time
> 894:   goto sleep;

I don't recall now why this was written the way it was, but I certainly do not understand why you rewrote it this way with a goto!

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

> 3933:   sleep:            // sleep forever ...
> 3934:     Sleep(100000);  // ... 100 seconds at a time
> 3935:   goto sleep;

Again there needs to be a very strong justification for doing it this way.

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

> 4116: }
> 4117: 
> 4118: static void exit_process_or_thread(Ept what, int code) {

exit_code was clearer and avoids changes below

src/hotspot/os/windows/vmError_windows.cpp line 71:

> 69: void VMError::raise_fail_fast(void* exrecord, void* context) {
> 70:   DWORD flags = (exrecord == nullptr) ? FAIL_FAST_GENERATE_EXCEPTION_ADDRESS : 0;
> 71:   RaiseFailFastException(static_cast<PEXCEPTION_RECORD>(exrecord), static_cast<PCONTEXT>(context), flags);

Curious that MS do not declare this as no_return themselves.

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16303#pullrequestreview-1698641909
PR Review Comment: https://git.openjdk.org/jdk/pull/16303#discussion_r1372520252
PR Review Comment: https://git.openjdk.org/jdk/pull/16303#discussion_r1372521965
PR Review Comment: https://git.openjdk.org/jdk/pull/16303#discussion_r1372522384
PR Review Comment: https://git.openjdk.org/jdk/pull/16303#discussion_r1372525405