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

[Kim Barrett]

On Fri, 27 Oct 2023 04:41:49 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 incrementally with one additional commit since the last revision:
> 
>   Revert to exit_code in os_windows.cpp

Changes requested by kbarrett (Reviewer).

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

> 508: enum Ept { EPT_THREAD, EPT_PROCESS, EPT_PROCESS_DIE };
> 509: // Wrapper around _endthreadex(), exit() and _exit()
> 510: [[noreturn]]

We (currently) have ATTRIBUTE_NORETURN.  However, this is Windows/MSVC-specific code, so I'm okay
with using the portable attribute directly, since we require a sufficiently recent version of MSVC to have this.
Note, however, that this won't work with older versions of clang, which is the raison d'etre for the attribute
macro.  Just so you know, since you've been trying to build for Windows with other compilers.

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

> 513: // The handler passed to _beginthreadex().
> 514: // Called with the associated Thread* as the argument.
> 515: static unsigned __stdcall thread_native_entry(void*);

This forward declaration is being added for a function that is defined a few lines later, with no intervening
references.  That seems pointless.

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

> 569:   // let it proceed to exit normally
> 570:   exit_process_or_thread(EPT_THREAD, res);
> 571:   return res;

exit_process_or_thread is now marked noreturn.  So it looks like thread_native_entry never returns either
now, making the return of res (and res itself) effectively unused, and the comment obsolete.

I don't know what the implications of marking thread_native_entry noreturn might be.  Passing it as a
parameter to _beginthreadex might be a problem.  But it kind of seems like thread_native_entry shouldn't
be calling exit_process_or_thread?  The semantics of the "start_address" function for _beginthreadex
are rather lightly documented so far as I could find.

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

PR Review: https://git.openjdk.org/jdk/pull/16303#pullrequestreview-1703425611
PR Review Comment: https://git.openjdk.org/jdk/pull/16303#discussion_r1375783845
PR Review Comment: https://git.openjdk.org/jdk/pull/16303#discussion_r1375780181
PR Review Comment: https://git.openjdk.org/jdk/pull/16303#discussion_r1375776786