On Tue, 5 Nov 2024 08:58:00 GMT, Kim Barrett <kbarr...@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
