Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
> 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