On Tue, 29 Nov 2022 17:07:02 GMT, Julian Waters <[email protected]> wrote:
>> Digging into this some more, the friend declaration exists to provide access
>> to the private `os::win32::enum Ept`.
>>
>> One obvious and cheap solution to that would be to make that enum public. I
>> think that would be an improvement vs the current friend declaration. But
>> there are some other things one could complain about there, such as the type
>> of the function requiring a complicated function pointer cast where it's
>> used. Here's a patch that I think cleans this up.
>>
>>
>> diff --git a/src/hotspot/os/windows/os_windows.cpp
>> b/src/hotspot/os/windows/os_windows.cpp
>> index 0651f0868f3..bf9e759b1d6 100644
>> --- a/src/hotspot/os/windows/os_windows.cpp
>> +++ b/src/hotspot/os/windows/os_windows.cpp
>> @@ -511,7 +511,9 @@ JNIEXPORT
>> LONG WINAPI topLevelExceptionFilter(struct _EXCEPTION_POINTERS*
>> exceptionInfo);
>>
>> // Thread start routine for all newly created threads
>> -static unsigned __stdcall thread_native_entry(Thread* thread) {
>> +// Called with the associated Thread* as the argument.
>> +unsigned __stdcall os::win32::thread_native_entry(void* t) {
>> + Thread* thread = static_cast<Thread*>(t);
>>
>> thread->record_stack_base_and_size();
>> thread->initialize_thread_current();
>> @@ -744,7 +746,7 @@ bool os::create_thread(Thread* thread, ThreadType
>> thr_type,
>> thread_handle =
>> (HANDLE)_beginthreadex(NULL,
>> (unsigned)stack_size,
>> - (unsigned (__stdcall *)(void*))
>> thread_native_entry,
>> + &os::win32::thread_native_entry,
>> thread,
>> initflag,
>> &thread_id);
>> diff --git a/src/hotspot/os/windows/os_windows.hpp
>> b/src/hotspot/os/windows/os_windows.hpp
>> index 94d7c3c5e2d..197797078d7 100644
>> --- a/src/hotspot/os/windows/os_windows.hpp
>> +++ b/src/hotspot/os/windows/os_windows.hpp
>> @@ -36,7 +36,6 @@ typedef void (*signal_handler_t)(int);
>>
>> class os::win32 {
>> friend class os;
>> - friend unsigned __stdcall thread_native_entry(Thread*);
>>
>> protected:
>> static int _processor_type;
>> @@ -70,6 +69,10 @@ class os::win32 {
>> static HINSTANCE load_Windows_dll(const char* name, char *ebuf, int
>> ebuflen);
>>
>> private:
>> + // The handler passed to _beginthreadex().
>> + // Called with the associated Thread* as the argument.
>> + static unsigned __stdcall thread_native_entry(void*);
>> +
>> enum Ept { EPT_THREAD, EPT_PROCESS, EPT_PROCESS_DIE };
>> // Wrapper around _endthreadex(), exit() and _exit()
>> static int exit_process_or_thread(Ept what, int exit_code);
>
> The issue with that would be that thread_native_entry is declared as static
> to the compilation unit on other other Operating Systems as well, and having
> it as a static member on the win32 class instead would end up breaking this
> convention, for which I'm not sure if there's a reason why all of them are
> declared like this
The thread entry functions are expected to be plain C functions as we use C
library calls to create threads (`_beginthreadex`, `pthread_create`) not C++.
They don't need to be visible outside the compilation unit hence static.
-------------
PR: https://git.openjdk.org/jdk/pull/11081
