On Mon, 4 Nov 2024 20:42:59 GMT, Magnus Ihse Bursie <i...@openjdk.org> 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
