8299343: Windows: Invalid thread_native_entry declaration

[kimbarrett]

Please review this change to the Windows version of thread_native_entry. It
is changed from being inconsistently declared (both file-scoped and external)
to consistently being a private static member function in os::win32. It's
signature is also changed to match where it's used, eliminating the need for a
function pointer cast.

Testing:
mach5 tier1

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8299343: Windows: Invalid thread_native_entry declaration

Reviewers

• David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to c96a8f45
• Ioi Lam (@iklam - Reviewer) ⚠️ Review applies to c96a8f45

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11787/head:pull/11787
$ git checkout pull/11787

Update a local copy of the PR:
$ git checkout pull/11787
$ git pull https://git.openjdk.org/jdk pull/11787/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11787

View PR using the GUI difftool:
$ git pr show -t 11787

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11787.diff

[bridgekeeper]

👋 Welcome back kbarrett! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@kimbarrett The following label will be automatically applied to this pull request:

• hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (a8b14577)
• 00: Full (c96a8f45)

[dholmes-ora]

Why do we want/need this to be a C++ static member function? The thread entry points are always plain C functions.

[TheShermanTanker]

Why do we want/need this to be a C++ static member function? The thread entry points are always plain C functions.

8299343 mentions how the Windows declaration is shaky due to the friend specification causing it to be initially declared extern implicitly, while it is later marked static at the definition, which makes it very likely to fail if flags such as -permissive- are passed, which is going to become the default compilation mode for Visual C++ in the future. Other solutions like leaving the thread native entry definitions extern seem to be undesirable, if memory serves me right. Maybe we could change all the thread native entry declarations to be static members if they should be uniform? All are statically scoped at the moment meaning changing them to members doesn't seem to have much of an effect on them, at least from my perspective

[kimbarrett]

Why do we want/need this to be a C++ static member function? The thread entry points are always plain C functions.

As discussed in the bug report, it needs to be a static member function
because it refers to private members of os::win32. The alternatives are also
discussed in the bug.

Of the various alternatives, this seems best to me. There's no difference
between a static member function and a non-member function, other than scope
and access rules.

And referring to the definitions for the other platforms as "plain C
functions" is something of a misnomer; they are file-scoped C++ functions. It
just so happens that's sufficient on those platforms. To make them actually be
"plain C functions" would involve extern "C" linkage specifications, which
may, in particular, indicate a different (from C++) calling convention.

[dholmes-ora]

And referring to the definitions for the other platforms as "plain C functions" is something of a misnomer;

Perhaps, but the thread creation functions (_beginthreadex and pthread_create) are themselves plain C API's not C++. It seems to me that the fact a static C++ member function is equivalent to a static C++ non-member function here and both work, is itself the fortuitous accident. If there were indeed an issue with C++ versus C calling convention then we would need to switch to the C convention.

I agree it is nicer from an encapsulation pov that these are member functions, but when we are using thread creation routines that are extra-lingua we have to expect to make some concessions.

I don't object to the proposed approach as long as it works, but I would have gone for a different option that keeps these routines more C-like and provide a public os::win32 function for the thread entry code to use.

[kimbarrett]

And referring to the definitions for the other platforms as "plain C functions" is something of a misnomer;

Perhaps, but the thread creation functions (_beginthreadex and pthread_create) are themselves plain C API's not C++. It seems to me that the fact a static C++ member function is equivalent to a static C++ non-member function here and both work, is itself the fortuitous accident. If there were indeed an issue with C++ versus C calling convention then we would need to switch to the C convention.

I agree it is nicer from an encapsulation pov that these are member functions, but when we are using thread creation routines that are extra-lingua we have to expect to make some concessions.

A non-member C++ function (whether file-scoped or having external linkage) is
no more C-like than a static member function. The differences are in naming
and scoping rules, which are C++ properties. They must be ABI-compatible with
each other, because C++ doesn't make a distinction between them for purposes
of passing them around as function pointers.

I think a C++ function is not required to be ABI-compatible with a C function,
but implementations don't make them unnecessarily different because that would
be painful for interoperability. So the compatibility is not a fortuitous
accident; it's intentional.

MSVC has multiple function call ABIs, on which the function definition and
call site must agree; hence the use of the compiler-specific __stdcall
qualifier when declaring/defining the function in question.

[dholmes-ora]

Nit: please terminate existing comment with a period - thanks.

[kimbarrett]

Done.

[openjdk]

@kimbarrett This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8299343: Windows: Invalid thread_native_entry declaration

Reviewed-by: dholmes, iklam

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[TheShermanTanker]

Could we refactor the rest of the platform specific thread entry definitions to be static members too after this change? As Kim points out there is not really an actual difference between a static member and file scoped func in terms of calling convention, and the rest of the platforms have identical definitions and callsites to Windows too meaning such a change wouldn't affect them much either, all of them are defined in os_*.cpp and used solely for os::create_thread. Only thing is that they don't have the corresponding declaration in the header like Windows does, at least until this is integrated

[dholmes-ora]

Could we refactor the rest of the platform specific thread entry definitions to be static members too after this change?

I prefer not. There is no general reason these thread entry routines should be part of the os::XXX classes. All the crud in the Windows version that required access to os::win32 is to address a win32 bug.

[TheShermanTanker]

Could we refactor the rest of the platform specific thread entry definitions to be static members too after this change?

I prefer not. There is no general reason these thread entry routines should be part of the os::XXX classes. All the crud in the Windows version that required access to os::win32 is to address a win32 bug.

Fair Point. I'm now a little curious as to why we couldn't make create_process_or_thread and Ept public though

[dholmes-ora]

I'm now a little curious as to why we couldn't make create_process_or_thread and Ept public though

To get rid of the friend declaration there were two options - Kim prefers the one he presented.

[kimbarrett]

Could we refactor the rest of the platform specific thread entry definitions to be static members too after this change?

I prefer not. There is no general reason these thread entry routines should be part of the os::XXX classes. All the crud in the Windows version that required access to os::win32 is to address a win32 bug.

I concur with David, preferring not to do the suggested refactoring.

Out of curiosity, I took a look at the referenced bug. Wow! That took some
serious sleuthing! (Sorry, but the bug is closed to non-Oracle access.)

[kimbarrett]

I'm now a little curious as to why we couldn't make create_process_or_thread and Ept public though

To get rid of the friend declaration there were two options - Kim prefers the one he presented.

I preferred moving the function into the class to avoid widening the public
API of the class to provide access to what is really an implementation detail
of the class. Of course, having os::win32 already befriending os helped. (We
resort to friendship way too often :( )

[kimbarrett]

Thanks for reviews @dholmes-ora and @iklam .

[kimbarrett]

/integrate

[openjdk]

Going to push as commit dfdbd0f.

[openjdk]

@kimbarrett Pushed as commit dfdbd0f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.