Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]

2024-11-08 Thread Magnus Ihse Bursie
On Wed, 6 Nov 2024 21:24:14 GMT, Kim Barrett  wrote:

>> @kimbarrett I added this to https://bugs.openjdk.org/browse/JDK-8343703. You 
>> are not as explicit here as the other places you commented that it is okay 
>> to do as a follow-up, but I'll assume that was what you meant. If not, let 
>> me know, and I'll look at fixing it for this PR already.
>
> The first part, eliminating the (IMO not actually helpful) helper function, I 
> wanted done here.  The second part,
> cleaning up or commenting the calculation of the length and dealing with 
> perhaps unneeded conditionals, I'm
> okay with being in a followup.  I guess I can live with the first part being 
> in that followup too.

Ok, fixed.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1834182702


Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]

2024-11-07 Thread Julian Waters
On Thu, 7 Nov 2024 12:08:50 GMT, Aleksey Shipilev  wrote:

>> src/hotspot/os/windows/os_windows.cpp line 510:
>> 
>>> 508: // Thread start routine for all newly created threads.
>>> 509: // Called with the associated Thread* as the argument.
>>> 510: static unsigned thread_native_entry(void* t) {
>> 
>> Whoa! Hold on there. The `_stdcall` is required here and nothing to do with 
>> 32-bit. We use `begindthreadex` to start threads and the entry function is 
>> required to be `_stdcall`.
>> https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/beginthread-beginthreadex?view=msvc-170
>
> Not sure why this comment was marked as "Resolved". I have the same question 
> here.

@shipilev See addressing comments below:

> https://learn.microsoft.com/en-us/cpp/cpp/stdcall?view=msvc-170
> On ARM and x64 processors, __stdcall is accepted and ignored by the compiler; 
> on ARM and x64 architectures, by convention, arguments are passed in 
> registers when possible, and subsequent arguments are passed on the stack.






> To my knowledge the only thing __cdecl and __stdcall do is affect the 
> argument passing on the stack since 32 bit uses the stack to pass arguments. 
> Since 64 bit passes arguments inside registers and then only later uses the 
> stack if there are too many parameters to fit in the parameter registers 
> (Basically permanent __fastcall), these specifiers are probably ignored in 
> all 64 bit platforms

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1832581212


Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]

2024-11-07 Thread Aleksey Shipilev
On Wed, 6 Nov 2024 00:56:49 GMT, David Holmes  wrote:

>> Magnus Ihse Bursie has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   fix: jvm_md.h was included, but not jvm.h...
>
> src/hotspot/os/windows/os_windows.cpp line 510:
> 
>> 508: // Thread start routine for all newly created threads.
>> 509: // Called with the associated Thread* as the argument.
>> 510: static unsigned thread_native_entry(void* t) {
> 
> Whoa! Hold on there. The `_stdcall` is required here and nothing to do with 
> 32-bit. We use `begindthreadex` to start threads and the entry function is 
> required to be `_stdcall`.
> https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/beginthread-beginthreadex?view=msvc-170

Not sure why this comment was marked as "Resolved". I have the same question 
here.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1832573434


Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]

2024-11-06 Thread Kim Barrett
On Wed, 6 Nov 2024 15:27:16 GMT, Magnus Ihse Bursie  wrote:

>> src/java.base/share/native/libjava/NativeLibraries.c line 67:
>> 
>>> 65: strcat(jniEntryName, "_");
>>> 66: strcat(jniEntryName, cname);
>>> 67: }
>> 
>> I would prefer this be directly inlined at the sole call (in 
>> findJniFunction),
>> to make it easier to verify there aren't any buffer overrun problems. (I 
>> don't
>> think there are, but looking at this in isolation triggered warnings in my
>> head.)
>> 
>> Also, it looks like all callers of findJniFunction ensure the cname argument
>> is non-null. So there should be no need to handle the null case in
>> findJniFunction (other than perhaps an assert or something). That could be
>> addressed in a followup. (I've already implicitly suggested elsewhere in this
>> PR revising this function in a followup because of the JNI_ON[UN]LOAD_SYMBOLS
>> thing.)
>
> @kimbarrett I added this to https://bugs.openjdk.org/browse/JDK-8343703. You 
> are not as explicit here as the other places you commented that it is okay to 
> do as a follow-up, but I'll assume that was what you meant. If not, let me 
> know, and I'll look at fixing it for this PR already.

The first part, eliminating the (IMO not actually helpful) helper function, I 
wanted done here.  The second part,
cleaning up or commenting the calculation of the length and dealing with 
perhaps unneeded conditionals, I'm
okay with being in a followup.  I guess I can live with the first part being in 
that followup too.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1831728737


Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]

2024-11-06 Thread Magnus Ihse Bursie
On Tue, 5 Nov 2024 08:58:00 GMT, Kim Barrett  wrote:

>> Magnus Ihse Bursie has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   fix: jvm_md.h was included, but not jvm.h...
>
> src/hotspot/os/windows/os_windows.cpp line 5863:
> 
>> 5861: return nullptr;
>> 5862:   }
>> 5863: 
> 
> [pre-existing, and can't comment on line 5858 because it's not sufficiently 
> near a change.]
> The calculation of `len` is wasting a byte when `lib_name` is null.  The `+2` 
> accounts for the
> terminating `NUL` and the underscore separator between the sym_name part and 
> the lib_name
> part.  That underscore isn't added when there isn't a lib_name part.  I think 
> the simplest fix would
> be to change `name_len` to `(name_len +1)` and `+2` to `+1` in that 
> calculation.  And add some
> commentary.
> 
> This might be deemed not worth fixing as there is likely often no actual 
> wastage, due to alignment
> padding, and it slightly further complicates the calculation.  But additional 
> commentary would still
> be desirable, to guide the next careful reader.  In which case it might be 
> simpler to describe the
> fixed version.
> 
> Since this is pre-existing and relatively harmless in execution, it can be 
> addressed in a followup
> change.

I've created https://bugs.openjdk.org/browse/JDK-8343703 for this, amongst 
other things.

> src/hotspot/share/include/jvm.h line 1165:
> 
>> 1163: #define AGENT_ONLOAD_SYMBOLS{"Agent_OnLoad"}
>> 1164: #define AGENT_ONUNLOAD_SYMBOLS  {"Agent_OnUnload"}
>> 1165: #define AGENT_ONATTACH_SYMBOLS  {"Agent_OnAttach"}
> 
> There is more cleanup that can be done here.  These definitions are used as
> array initializers (hence the surrounding curly braces).  They are now always
> singleton, rather than sometimes having 2 elements.  The uses iterate over the
> now always singleton arrays.  Those iterations are no longer needed and could
> be eliminated.  And these macros could be eliminated, using the corresponding
> string directly in each use.  This can all be done as a followup change.

Handled by https://bugs.openjdk.org/browse/JDK-8343703.

> src/java.base/share/native/libjava/NativeLibraries.c line 67:
> 
>> 65: strcat(jniEntryName, "_");
>> 66: strcat(jniEntryName, cname);
>> 67: }
> 
> I would prefer this be directly inlined at the sole call (in findJniFunction),
> to make it easier to verify there aren't any buffer overrun problems. (I don't
> think there are, but looking at this in isolation triggered warnings in my
> head.)
> 
> Also, it looks like all callers of findJniFunction ensure the cname argument
> is non-null. So there should be no need to handle the null case in
> findJniFunction (other than perhaps an assert or something). That could be
> addressed in a followup. (I've already implicitly suggested elsewhere in this
> PR revising this function in a followup because of the JNI_ON[UN]LOAD_SYMBOLS
> thing.)

@kimbarrett I added this to https://bugs.openjdk.org/browse/JDK-8343703. You 
are not as explicit here as the other places you commented that it is okay to 
do as a follow-up, but I'll assume that was what you meant. If not, let me 
know, and I'll look at fixing it for this PR already.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1831240264
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1831240942
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1831243370


Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]

2024-11-06 Thread Magnus Ihse Bursie
On Tue, 5 Nov 2024 16:28:04 GMT, Kim Barrett  wrote:

>> Magnus Ihse Bursie has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   fix: jvm_md.h was included, but not jvm.h...
>
> src/hotspot/os/windows/os_windows.cpp line 5820:
> 
>> 5818: }
>> 5819: 
>> 5820: // FIXME
> 
> ???

I apologize this slipped through. It was a marker for myself which I added when 
searching for code that did _stdcall name mangling operations.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1831227000


Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]

2024-11-06 Thread Thomas Stuefe
On Wed, 6 Nov 2024 09:12:02 GMT, Thomas Stuefe  wrote:

> > I think you may be throwing the baby out with the bath water when it comes 
> > to `__stdcall`. It may be that 32-bit requires `__stdcall` but I don't see 
> > anything that states `__stdcall` is ONLY for 32-bit!
> 
> stdcall and cdecl are 32-bit Windows calling conventions. On x64 and arm64, 
> as on all other platforms we support, there is just one calling convention. 
> See 
> https://en.wikipedia.org/wiki/X86_calling_conventions#Microsoft_x64_calling_convention.
> 
> I am sure __stdcall is ignored by the compiler on x64 or arm64.

Never mind my noise, other people did already answer this :-)

-

PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2459080708


Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]

2024-11-06 Thread Thomas Stuefe
On Wed, 6 Nov 2024 04:40:24 GMT, Julian Waters  wrote:

> I think you may be throwing the baby out with the bath water when it comes to 
> `__stdcall`. It may be that 32-bit requires `__stdcall` but I don't see 
> anything that states `__stdcall` is ONLY for 32-bit!

stdcall and cdecl are 32-bit Windows calling conventions. On x64 and arm64, as 
on all other platforms we support, there is just one calling convention. See 
https://en.wikipedia.org/wiki/X86_calling_conventions#Microsoft_x64_calling_convention.

I am sure __stdcall is ignored by the compiler on x64 or arm64.

-

PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2459078526


Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]

2024-11-05 Thread David Holmes
On Wed, 6 Nov 2024 01:44:48 GMT, Alex Menkov  wrote:

> On ARM and x64 processors, __stdcall is accepted and ignored by the compiler;

@alexmenkov and @TheShermanTanker , I stand corrected and my apologies to 
@magicus .

-

PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2458778303


Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]

2024-11-05 Thread Julian Waters
On Wed, 6 Nov 2024 01:44:48 GMT, Alex Menkov  wrote:

> I think you may be throwing the baby out with the bath water when it comes to 
> `__stdcall`. It may be that 32-bit requires `__stdcall` but I don't see 
> anything that states `__stdcall` is ONLY for 32-bit!

To my knowledge the only thing __cdecl and __stdcall do is affect the argument 
passing on the stack since 32 bit uses the stack to pass arguments. Since 64 
bit passes arguments inside registers and then only later uses the stack if 
there are too many parameters to fit in the parameter registers (Basically 
permanent __fastcall), these specifiers are probably ignored in all 64 bit 
platforms

-

PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2458712195


Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]

2024-11-05 Thread Alex Menkov
On Wed, 6 Nov 2024 00:58:10 GMT, David Holmes  wrote:

> I think you may be throwing the baby out with the bath water when it comes to 
> `__stdcall`. It may be that 32-bit requires `__stdcall` but I don't see 
> anything that states `__stdcall` is ONLY for 32-bit!

https://learn.microsoft.com/en-us/cpp/cpp/stdcall?view=msvc-170
`On ARM and x64 processors, __stdcall is accepted and ignored by the compiler; 
on ARM and x64 architectures, by convention, arguments are passed in registers 
when possible, and subsequent arguments are passed on the stack.`

-

PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2458534929


Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]

2024-11-05 Thread David Holmes
On Mon, 4 Nov 2024 20:42:59 GMT, Magnus Ihse Bursie  wrote:

>> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86 
>> Port_](https://openjdk.org/jeps/479).
>> 
>> This is the summary of JEP 479:
>>> Remove the source code and build support for the Windows 32-bit x86 port. 
>>> This port was [deprecated for removal in JDK 
>>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a 
>>> future release.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   fix: jvm_md.h was included, but not jvm.h...

I think you may be throwing the baby out with the bath water when it comes to 
`__stdcall`. It may be that 32-bit requires `__stdcall` but I don't see 
anything that states `__stdcall` is ONLY for 32-bit!

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

> 508: // Thread start routine for all newly created threads.
> 509: // Called with the associated Thread* as the argument.
> 510: static unsigned thread_native_entry(void* t) {

Whoa! Hold on there. The `_stdcall` is required here and nothing to do with 
32-bit. We use `begindthreadex` to start threads and the entry function is 
required to be `_stdcall`.
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/beginthread-beginthreadex?view=msvc-170

-

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21744#pullrequestreview-2417056020
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1830259353


Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]

2024-11-05 Thread Kim Barrett
On Mon, 4 Nov 2024 20:42:59 GMT, Magnus Ihse Bursie  wrote:

>> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86 
>> Port_](https://openjdk.org/jeps/479).
>> 
>> This is the summary of JEP 479:
>>> Remove the source code and build support for the Windows 32-bit x86 port. 
>>> This port was [deprecated for removal in JDK 
>>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a 
>>> future release.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   fix: jvm_md.h was included, but not jvm.h...

I kind of wish the __cdecl / __stdcall changes had been done separately.  Oh 
well.

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

> 5818: }
> 5819: 
> 5820: // FIXME

???

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

> 5861: return nullptr;
> 5862:   }
> 5863: 

[pre-existing, and can't comment on line 5858 because it's not sufficiently 
near a change.]
The calculation of `len` is wasting a byte when `lib_name` is null.  The `+2` 
accounts for the
terminating `NUL` and the underscore separator between the sym_name part and 
the lib_name
part.  That underscore isn't added when there isn't a lib_name part.  I think 
the simplest fix would
be to change `name_len` to `(name_len +1)` and `+2` to `+1` in that 
calculation.  And add some
commentary.

This might be deemed not worth fixing as there is likely often no actual 
wastage, due to alignment
padding, and it slightly further complicates the calculation.  But additional 
commentary would still
be desirable, to guide the next careful reader.  In which case it might be 
simpler to describe the
fixed version.

Since this is pre-existing and relatively harmless in execution, it can be 
addressed in a followup
change.

src/hotspot/share/include/jvm.h line 1165:

> 1163: #define AGENT_ONLOAD_SYMBOLS{"Agent_OnLoad"}
> 1164: #define AGENT_ONUNLOAD_SYMBOLS  {"Agent_OnUnload"}
> 1165: #define AGENT_ONATTACH_SYMBOLS  {"Agent_OnAttach"}

There is more cleanup that can be done here.  These definitions are used as
array initializers (hence the surrounding curly braces).  They are now always
singleton, rather than sometimes having 2 elements.  The uses iterate over the
now always singleton arrays.  Those iterations are no longer needed and could
be eliminated.  And these macros could be eliminated, using the corresponding
string directly in each use.  This can all be done as a followup change.

src/java.base/share/native/libjava/NativeLibraries.c line 67:

> 65: strcat(jniEntryName, "_");
> 66: strcat(jniEntryName, cname);
> 67: }

I would prefer this be directly inlined at the sole call (in findJniFunction),
to make it easier to verify there aren't any buffer overrun problems. (I don't
think there are, but looking at this in isolation triggered warnings in my
head.)

Also, it looks like all callers of findJniFunction ensure the cname argument
is non-null. So there should be no need to handle the null case in
findJniFunction (other than perhaps an assert or something). That could be
addressed in a followup. (I've already implicitly suggested elsewhere in this
PR revising this function in a followup because of the JNI_ON[UN]LOAD_SYMBOLS
thing.)

-

Changes requested by kbarrett (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21744#pullrequestreview-2415002837
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1829659373
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1828969105
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1829478432
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1829684759


Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]

2024-11-05 Thread Magnus Ihse Bursie
On Mon, 4 Nov 2024 20:42:59 GMT, Magnus Ihse Bursie  wrote:

>> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86 
>> Port_](https://openjdk.org/jeps/479).
>> 
>> This is the summary of JEP 479:
>>> Remove the source code and build support for the Windows 32-bit x86 port. 
>>> This port was [deprecated for removal in JDK 
>>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a 
>>> future release.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   fix: jvm_md.h was included, but not jvm.h...

This has now passed internal CI testing tier1-5 (except for one test that also 
fails in mainline).

-

PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2457376495


Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]

2024-11-04 Thread Magnus Ihse Bursie
> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86 
> Port_](https://openjdk.org/jeps/479).
> 
> This is the summary of JEP 479:
>> Remove the source code and build support for the Windows 32-bit x86 port. 
>> This port was [deprecated for removal in JDK 
>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a 
>> future release.

Magnus Ihse Bursie has updated the pull request incrementally with one 
additional commit since the last revision:

  fix: jvm_md.h was included, but not jvm.h...

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/21744/files
  - new: https://git.openjdk.org/jdk/pull/21744/files/40291b9b..9b10e74c

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=21744&range=29
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21744&range=28-29

  Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/21744.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/21744/head:pull/21744

PR: https://git.openjdk.org/jdk/pull/21744