On Tue, 29 Oct 2024 20:22:03 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 32/64-bit confusion in comment in VirtualMachineImpl.c I didn't spend much time looking for more places that could use updating. We can always do more cleaning up later if more are found. make/scripts/compare.sh line 79: > 77: > 78: if [ "$OPENJDK_TARGET_OS" = "windows" ]; then > 79: DIS_DIFF_FILTER="$SED -r \ This is now being defined for windows-aarch64 too, when it previously wasn't. Is that intentional? make/scripts/compare.sh line 1457: > 1455: THIS_SEC_BIN="$THIS_SEC_DIR/sec-bin.zip" > 1456: if [ "$OPENJDK_TARGET_OS" = "windows" ]; then > 1457: JGSS_WINDOWS_BIN="jgss-windows-x64-bin.zip" This is now being defined for windows-aarch64 too, when it previously wasn't. That seems wrong, given the "x64" suffix. src/hotspot/cpu/x86/sharedRuntime_x86_32.cpp line 1433: > 1431: // instructions that are SP relative. After the jni call we switch to > FP > 1432: // relative instructions instead of re-adjusting the stack on windows. > 1433: // > **************************************************************************** I think it might be better to keep this comment. It might be helpful information for someone who needs to touch this code between now and when we remove all 32bit x86 support (which might be soonish, but not immediate). And this comment will go away when that change happens. src/hotspot/os/windows/os_windows.cpp line 2592: > 2590: ctx->Rdx = (DWORD)0; // remainder > 2591: // Continue the execution > 2592: #else Maybe retain `#else` clause as an `#error`? src/hotspot/share/adlc/main.cpp line 494: > 492: } > 493: > 494: #if !defined(_WIN32) || defined(_WIN64) Removing the conditionalization is fine for this change. But see also https://bugs.openjdk.org/browse/JDK-8342639 I've added a note there that this change removed the conditionalization. src/java.base/windows/native/libjava/gdefs_md.h line 31: > 29: > 30: #include <stddef.h> > 31: #ifndef _WIN64 I suspect the unix/windows gdefs_md.h files could be eliminated, and just make gdefs.h use portable headers. That can be done as a separate cleanup. src/java.base/windows/native/libjava/jlong_md.h line 66: > 64: #define jlong_zero_init ((jlong) 0) > 65: > 66: #ifdef _WIN64 After this change I think the differences between the unix and windows variants of this file are trivial and could be resolved in favor of moving everything directly into jlong.h. Though note there are some places in java.desktop that currently directly include jlong_md.h. This can be done as a separate cleanup. ------------- Changes requested by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21744#pullrequestreview-2403283976 PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1821670031 PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1821671116 PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1821680493 PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1821684248 PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1821796117 PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1821806395 PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1821809957
