On Wed, 23 Nov 2022 05:22:10 GMT, Kim Barrett <kbarr...@openjdk.org> wrote:
>> It's to avoid redefining the linkage as static in os_windows.cpp (where it's >> implemented) after an extern declaration (inside the class), which is >> forbidden by C++11: >> >>> The linkages implied by successive declarations for a given entity shall >>> agree. That is, within a given scope, each declaration declaring the same >>> variable name or the same overloading of a function name shall imply the >>> same linkage. >> >> While 2019 by default seems to ignore this rule and accepts the conflicting >> linkage as a language extension, this can cause issues with newer and >> stricter versions of the Visual C++ compiler (especially with -permissive- >> passed during compilation, which Magnus and Daniel have pointed out in >> another discussion will become the default mode of compilation in the >> future). It's not possible to declare a static friend inside a class, so the >> addition above takes advantage of another C++ feature instead: >> >>> ยง11.3/4 [class.friend] >> A function first declared in a friend declaration has external linkage >> (3.5). Otherwise, the function retains its previous linkage (7.1.1). > > I think the problem here is the friend declaration, which doesn't look like > it's needed and could be deleted. 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); ------------- PR: https://git.openjdk.org/jdk/pull/11081