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

[Magnus Ihse Bursie]

On Tue, 5 Nov 2024 08:58:00 GMT, Kim Barrett <kbarrett at openjdk.org> 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