On Tue, 29 Nov 2022 17:07:02 GMT, Julian Waters <jwat...@openjdk.org> 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