Re: RFR: 8333149: ubsan : memset on nullptr target detected in jvmtiEnvBase.cpp get_object_monitor_usage
On Wed, 29 May 2024 09:09:16 GMT, Matthias Baesken wrote: > When running with ubsan - enabled binaries (--enable-ubsan), > in the vmTestbase/nsk/jdi tests some cases of memset on nullptr destinations > are detected in get_object_monitor_usage . > > // null out memory for robustness > memset(ret.waiters, 0, ret.waiter_count * sizeof(jthread *)); > memset(ret.notify_waiters, 0, ret.notify_waiter_count * sizeof(jthread *)); > > probably we should add checks there. > Example : > vmTestbase/nsk/jdi/ObjectReference/entryCount/entrycount002/TestDescription.jtr > > debugee.stderr> /src/hotspot/share/prims/jvmtiEnvBase.cpp:1560:11: runtime > error: null pointer passed as argument 1, which is declared to never be null > debugee.stderr> #0 0x7ffb2568559c in > JvmtiEnvBase::get_object_monitor_usage(JavaThread*, _jobject*, > jvmtiMonitorUsage*) src/hotspot/share/prims/jvmtiEnvBase.cpp:1560 > debugee.stderr> #1 0x7ffb27987bd7 in VM_GetObjectMonitorUsage::doit() > src/hotspot/share/prims/jvmtiEnvBase.hpp:594 > debugee.stderr> #2 0x7ffb28ddc2dd in VM_Operation::evaluate() > src/hotspot/share/runtime/vmOperations.cpp:75 > debugee.stderr> #3 0x7ffb28deac41 in > VMThread::evaluate_operation(VM_Operation*) > src/hotspot/share/runtime/vmThread.cpp:283 > debugee.stderr> #4 0x7ffb28decc4f in VMThread::inner_execute(VM_Operation*) > src/hotspot/share/runtime/vmThread.cpp:427 > debugee.stderr> #5 0x7ffb28ded7b9 in VMThread::loop() > src/hotspot/share/runtime/vmThread.cpp:493 > debugee.stderr> #6 0x7ffb28ded8a7 in VMThread::run() > src/hotspot/share/runtime/vmThread.cpp:177 > debugee.stderr> #7 0x7ffb28b7e31a in Thread::call_run() > src/hotspot/share/runtime/thread.cpp:225 > debugee.stderr> #8 0x7ffb281c4971 in thread_native_entry > src/hotspot/os/linux/os_linux.cpp:846 > debugee.stderr> #9 0x7ffb2df416e9 in start_thread > (/lib64/libpthread.so.0+0xa6e9) (BuildId: > 2f8d3c2d0f4d7888c2598d2ff6356537f5708a73) > debugee.stderr> #10 0x7ffb2d51550e in clone (/lib64/libc.so.6+0x11850e) > (BuildId: f732026552f6adff988b338e92d466bc81a01c37) > > vmTestbase/nsk/jdi/ObjectReference/owningThread/owningthread002/TestDescription.jtr > > debugee.stderr> /src/hotspot/share/prims/jvmtiEnvBase.cpp:1561:11: runtime > error: null pointer passed as argument 1, which is declared to never be null > debugee.stderr> #0 0x7f1e070855bb in > JvmtiEnvBase::get_object_monitor_usage(JavaThread*, _jobject*, > jvmtiMonitorUsage*) src/hotspot/share/prims/jvmtiEnvBase.cpp:1561 > debugee.stderr> #1 0x7f1e09387bd7 in VM_GetObjectMonitorUsage::doit() > src/hotspot/share/prims/jvmtiEnvBase.hpp:594 > debugee.stderr> #2 0x7f1e0a7dc2dd in VM_Operation::evaluate() src/hotsp... Please note that `allocate` sets `*mem_ptr` to `nullptr` if the size is 0. This is not an error. - PR Comment: https://git.openjdk.org/jdk/pull/19450#issuecomment-2139416153
Re: RFR: 8333149: ubsan : memset on nullptr target detected in jvmtiEnvBase.cpp get_object_monitor_usage
On Wed, 29 May 2024 09:09:16 GMT, Matthias Baesken wrote: > When running with ubsan - enabled binaries (--enable-ubsan), > in the vmTestbase/nsk/jdi tests some cases of memset on nullptr destinations > are detected in get_object_monitor_usage . > > // null out memory for robustness > memset(ret.waiters, 0, ret.waiter_count * sizeof(jthread *)); > memset(ret.notify_waiters, 0, ret.notify_waiter_count * sizeof(jthread *)); > > probably we should add checks there. > Example : > vmTestbase/nsk/jdi/ObjectReference/entryCount/entrycount002/TestDescription.jtr > > debugee.stderr> /src/hotspot/share/prims/jvmtiEnvBase.cpp:1560:11: runtime > error: null pointer passed as argument 1, which is declared to never be null > debugee.stderr> #0 0x7ffb2568559c in > JvmtiEnvBase::get_object_monitor_usage(JavaThread*, _jobject*, > jvmtiMonitorUsage*) src/hotspot/share/prims/jvmtiEnvBase.cpp:1560 > debugee.stderr> #1 0x7ffb27987bd7 in VM_GetObjectMonitorUsage::doit() > src/hotspot/share/prims/jvmtiEnvBase.hpp:594 > debugee.stderr> #2 0x7ffb28ddc2dd in VM_Operation::evaluate() > src/hotspot/share/runtime/vmOperations.cpp:75 > debugee.stderr> #3 0x7ffb28deac41 in > VMThread::evaluate_operation(VM_Operation*) > src/hotspot/share/runtime/vmThread.cpp:283 > debugee.stderr> #4 0x7ffb28decc4f in VMThread::inner_execute(VM_Operation*) > src/hotspot/share/runtime/vmThread.cpp:427 > debugee.stderr> #5 0x7ffb28ded7b9 in VMThread::loop() > src/hotspot/share/runtime/vmThread.cpp:493 > debugee.stderr> #6 0x7ffb28ded8a7 in VMThread::run() > src/hotspot/share/runtime/vmThread.cpp:177 > debugee.stderr> #7 0x7ffb28b7e31a in Thread::call_run() > src/hotspot/share/runtime/thread.cpp:225 > debugee.stderr> #8 0x7ffb281c4971 in thread_native_entry > src/hotspot/os/linux/os_linux.cpp:846 > debugee.stderr> #9 0x7ffb2df416e9 in start_thread > (/lib64/libpthread.so.0+0xa6e9) (BuildId: > 2f8d3c2d0f4d7888c2598d2ff6356537f5708a73) > debugee.stderr> #10 0x7ffb2d51550e in clone (/lib64/libc.so.6+0x11850e) > (BuildId: f732026552f6adff988b338e92d466bc81a01c37) > > vmTestbase/nsk/jdi/ObjectReference/owningThread/owningthread002/TestDescription.jtr > > debugee.stderr> /src/hotspot/share/prims/jvmtiEnvBase.cpp:1561:11: runtime > error: null pointer passed as argument 1, which is declared to never be null > debugee.stderr> #0 0x7f1e070855bb in > JvmtiEnvBase::get_object_monitor_usage(JavaThread*, _jobject*, > jvmtiMonitorUsage*) src/hotspot/share/prims/jvmtiEnvBase.cpp:1561 > debugee.stderr> #1 0x7f1e09387bd7 in VM_GetObjectMonitorUsage::doit() > src/hotspot/share/prims/jvmtiEnvBase.hpp:594 > debugee.stderr> #2 0x7f1e0a7dc2dd in VM_Operation::evaluate() src/hotsp... LGTM. - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19450#pullrequestreview-208522
Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v4]
On Wed, 20 Mar 2024 14:02:23 GMT, Richard Reingruber wrote: >> This pr changes `JfrJvmtiAgent::retransform_classes()` and >> `jfr_set_enabled` to switch to `WXWrite` before transitioning to the vm. >> >> Testing: >> make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java >> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync >> make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java >> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync >> >> More tests are pending. > > Richard Reingruber has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite > - 2 more locations > - Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite > - Set WXWrite at more locations > - Switch to WXWrite before entering the vm Good catch! - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18238#pullrequestreview-1948978584
Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]
On Mon, 18 Mar 2024 13:14:41 GMT, Richard Reingruber wrote: >> This pr changes `JfrJvmtiAgent::retransform_classes()` and >> `jfr_set_enabled` to switch to `WXWrite` before transitioning to the vm. >> >> Testing: >> make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java >> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync >> make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java >> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync >> >> More tests are pending. > > Richard Reingruber has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite > - Set WXWrite at more locations > - Switch to WXWrite before entering the vm +1 - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18238#pullrequestreview-1947160377
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v28]
On Tue, 27 Feb 2024 08:31:15 GMT, Suchismith Roy wrote: >> J2SE agent does not start and throws error when it tries to find the shared >> library ibm_16_am. >> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load >> fails.It fails to identify the shared library ibm_16_am.a shared archive >> file on AIX. >> Hence we are providing a function which will additionally search for .a file >> on AIX ,when the search for .so file fails. > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > indendt jdk21u is only open for critical backports and requires special approval. Please backport it to jdk21u-dev. What about jdk22u? - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1978239031
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v27]
On Tue, 27 Feb 2024 07:48:00 GMT, Suchismith Roy wrote:
>> J2SE agent does not start and throws error when it tries to find the shared
>> library ibm_16_am.
>> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load
>> fails.It fails to identify the shared library ibm_16_am.a shared archive
>> file on AIX.
>> Hence we are providing a function which will additionally search for .a file
>> on AIX ,when the search for .so file fails.
>
> Suchismith Roy has updated the pull request incrementally with one additional
> commit since the last revision:
>
>remove redundant logic
src/hotspot/os/aix/os_aix.cpp line 1176:
> 1174: if (result == nullptr && pointer_to_dot != nullptr &&
> strcmp(pointer_to_dot, old_extension) == 0) {
> 1175: snprintf(pointer_to_dot, sizeof(old_extension), "%s",
> new_extension);
> 1176: result = dll_load_library(file_path, ebuf, ebuflen);
You should have adapted the indentation, too.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1503817600
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v24]
On Mon, 19 Feb 2024 17:16:34 GMT, Suchismith Roy wrote:
>> J2SE agent does not start and throws error when it tries to find the shared
>> library ibm_16_am.
>> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load
>> fails.It fails to identify the shared library ibm_16_am.a shared archive
>> file on AIX.
>> Hence we are providing a function which will additionally search for .a file
>> on AIX ,when the search for .so file fails.
>
> Suchismith Roy has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - remove error_code
> - revert error code check
src/hotspot/os/aix/os_aix.cpp line 1185:
> 1183: // Shared object in .so format dont have braces, hence they get
> removed for archives with members.
> 1184: if (result == nullptr) {
> 1185: assert(strlen(new_extension) < strlen(old_extension),"New extension
> length must be less than existing one");
`<=` would be sufficient. Please add a whitespace after `,`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1495478067
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v23]
On Mon, 19 Feb 2024 10:05:17 GMT, Suchismith Roy wrote:
>> J2SE agent does not start and throws error when it tries to find the shared
>> library ibm_16_am.
>> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load
>> fails.It fails to identify the shared library ibm_16_am.a shared archive
>> file on AIX.
>> Hence we are providing a function which will additionally search for .a file
>> on AIX ,when the search for .so file fails.
>
> Suchismith Roy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments
src/hotspot/os/aix/os_aix.cpp line 1185:
> 1183: // Shared object in .so format dont have braces, hence they get
> removed for archives with members.
> 1184: if (error_code == EACCES || error_code == ENOENT) {
> 1185: if (strlen(new_extension) > strlen(old_extension)){
I think this should better be an assertion because the extensions are constants.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1494317702
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v13]
On Thu, 1 Feb 2024 10:31:14 GMT, Thomas Stuefe wrote: >> Suchismith Roy has updated the pull request incrementally with one >> additional commit since the last revision: >> >> spelling > > I'm busy with FOSDEM this week and probably next. Will look at this end of > next week or the week after. @tstuefe, @offamitkumar: You had requested changes. Should we wait for your feedback regarding the latest version? - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1945348564
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v22]
On Mon, 12 Feb 2024 18:04:21 GMT, Suchismith Roy wrote: >> J2SE agent does not start and throws error when it tries to find the shared >> library ibm_16_am. >> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load >> fails.It fails to identify the shared library ibm_16_am.a shared archive >> file on AIX. >> Hence we are providing a function which will additionally search for .a file >> on AIX ,when the search for .so file fails. > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > Remove not matched trailing whitespaces Thanks for improving it! My points have been addressed. Up to other reviewers, now. - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16604#pullrequestreview-1877020968
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v14]
On Thu, 8 Feb 2024 10:26:05 GMT, Suchismith Roy wrote: > > The trailing whitespace errors must get fixed (integration blocker). > > I am unable to resolve this. The spaces seem fine as i see it in the > terminal. Could it be some other error ? Strange. I don't see any whitespace problem, either. Maybe it's possible to rerun jcheck (GitHub Actions). Otherwise, you may need to create a new PR. - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1934260353
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v15]
On Tue, 6 Feb 2024 08:45:12 GMT, Suchismith Roy wrote:
>> J2SE agent does not start and throws error when it tries to find the shared
>> library ibm_16_am.
>> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load
>> fails.It fails to identify the shared library ibm_16_am.a shared archive
>> file on AIX.
>> Hence we are providing a function which will additionally search for .a file
>> on AIX ,when the search for .so file fails.
>
> Suchismith Roy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> change control flow
src/hotspot/os/aix/os_aix.cpp line 1178:
> 1176: if (pointer_to_dot == nullptr) {
> 1177: log_info(os)("Attempting to load a shared object without extension
> %s", filename);
> 1178: return result;
Memory leak: `file_path` not freed before returning.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1479524411
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v11]
On Wed, 31 Jan 2024 11:22:27 GMT, Joachim Kern wrote: >> The load library gets the entire library name, after construction from >> dll_build_name. This is always a .so file name. When .so file name fails to >> load, we fallback to .a filename. >> Do i need to mention the filename as libfilename.so then ? > > Yes, I think this would make it clear. Please use space after comma. - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1477871269
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v14]
On Mon, 5 Feb 2024 08:52:21 GMT, Suchismith Roy wrote:
>> J2SE agent does not start and throws error when it tries to find the shared
>> library ibm_16_am.
>> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load
>> fails.It fails to identify the shared library ibm_16_am.a shared archive
>> file on AIX.
>> Hence we are providing a function which will additionally search for .a file
>> on AIX ,when the search for .so file fails.
>
> Suchismith Roy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Change logging
The trailing whitespace errors must get fixed (integration blocker).
src/hotspot/os/aix/os_aix.cpp line 1183:
> 1181: // If the load fails,we try to reload by changing the extension to .a
> for .so files only.
> 1182: if (result == nullptr) {
> 1183: if (strcmp(pointer_to_dot, ".so") == 0) {
We could possibly reach here with `pointer_to_dot` == nullptr. Invoking strcmp
causes undefined behavior!
-
Changes requested by mdoerr (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16604#pullrequestreview-1862189297
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1477856818
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v13]
On Thu, 1 Feb 2024 09:42:00 GMT, Suchismith Roy wrote: >> In addition, the nullptr check is important to avoid undefined behavior when >> passing `pointer_to_dot` to any string function. > > Ok. So then may be we can return a nullptr and do a log_info(os) with the > correct error report ? @tstuefe @TheRealMDoerr I'd probably use `log_info(library)`. - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1474449287
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v13]
On Thu, 1 Feb 2024 04:13:52 GMT, Martin Doerr wrote: >> I didn't follow that. You mean i need to keep a check if it is null and >> print it out ? > > An assertion is only used for debug builds. Such an error should be handled > in product builds as well. I think an attempt to load an invalid library > should simply fail. You may add logging if needed. > @tstuefe: Do you agree or have another proposal to handle such errors? In addition, the nullptr check is important to avoid undefined behavior when passing `pointer_to_dot` to any string function. - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1473774551
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v13]
On Wed, 31 Jan 2024 15:06:54 GMT, Suchismith Roy wrote: >> src/hotspot/os/aix/os_aix.cpp line 1176: >> >>> 1174: strncpy(file_path,filename, buffer_length + 1); >>> 1175: char* const pointer_to_dot = strrchr(file_path, '.'); >>> 1176: assert(pointer_to_dot != nullptr, "Attempting to load a shared >>> object without extension? %s", filename); >> >> This should not only be an assertion. I think the check could be used >> instead of the strcmp below. > > I didn't follow that. You mean i need to keep a check if it is null and print > it out ? An assertion is only used for debug builds. Such an error should be handled in product builds as well. I think an attempt to load an invalid library should simply fail. You may add logging if needed. @tstuefe: Do you agree or have another proposal to handle such errors? - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1473772905
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v13]
On Wed, 31 Jan 2024 13:17:21 GMT, Suchismith Roy wrote: >> J2SE agent does not start and throws error when it tries to find the shared >> library ibm_16_am. >> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load >> fails.It fails to identify the shared library ibm_16_am.a shared archive >> file on AIX. >> Hence we are providing a function which will additionally search for .a file >> on AIX ,when the search for .so file fails. > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > spelling src/hotspot/os/aix/os_aix.cpp line 1176: > 1174: strncpy(file_path,filename, buffer_length + 1); > 1175: char* const pointer_to_dot = strrchr(file_path, '.'); > 1176: assert(pointer_to_dot != nullptr, "Attempting to load a shared object > without extension? %s", filename); This should not only be an assertion. I think the check could be used instead of the strcmp below. - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1472813890
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs
On Tue, 30 Jan 2024 14:02:41 GMT, Matthias Baesken wrote: >>> Yes there is a nice define in the AIX header >> >> *sigh* On linux, they go to some lengths to avoid this, using a __REDEFINE >> mechanism. Oh well. >> >> Anyway, I think this particular can be resolved by not including the >> standard includes in the header file (which is bad practice anyway!). I'm >> currently testing this build in our CI to see that it does not break >> anything else. I'd appreciate if you could take the latest version for a >> spin, particularly a debug build... > >> I'd appreciate if you could take the latest version for a spin, particularly >> a debug build... > > Yes we pick up the latest version of the PR in a couple of hours for the > build+'lots of tests' (and this includes a fastdebug too). @MBaesken, @JoKern65: This seems to break the debug build (fast and slow) on AIX: jdk/src/java.desktop/share/native/libawt/java2d/pipe/BufferedRenderPipe.c:101:24: error: no member named 'open64' in 'SpanIteratorFuncs'; did you mean 'open'? srData = (*pFuncs->open)(env, si); ^~~~ open /usr/include/fcntl.h:115:14: note: expanded from macro 'open' #define openopen64 ^ jdk/src/java.desktop/share/native/libawt/java2d/pipe/SpanIterator.h:37:17: note: 'open' declared here void *(*open)(JNIEnv *env, jobject iterator); ^ Ah, that has already been reported above. Yeah, interesting that the normal build has passed. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1918443702
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v9]
On Tue, 16 Jan 2024 08:36:49 GMT, Suchismith Roy wrote: >> J2SE agent does not start and throws error when it tries to find the shared >> library ibm_16_am. >> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load >> fails.It fails to identify the shared library ibm_16_am.a shared archive >> file on AIX. >> Hence we are providing a function which will additionally search for .a file >> on AIX ,when the search for .so file fails. > > Suchismith Roy has updated the pull request incrementally with three > additional commits since the last revision: > > - Update porting_aix.cpp > - Update porting_aix.cpp > - Update os_aix.cpp I think a comment like this would be ideal: Search order: "library.so" -> load "library.so", then "library.a" "library" -> load "library.so", then "library.a" ? "library.a" -> only load "library.a" ? (If that's correct.) I prefer that over a very verbose version. + References to Spec and J9 of course. - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1911737129
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v9]
On Tue, 16 Jan 2024 08:36:49 GMT, Suchismith Roy wrote: >> J2SE agent does not start and throws error when it tries to find the shared >> library ibm_16_am. >> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load >> fails.It fails to identify the shared library ibm_16_am.a shared archive >> file on AIX. >> Hence we are providing a function which will additionally search for .a file >> on AIX ,when the search for .so file fails. > > Suchismith Roy has updated the pull request incrementally with three > additional commits since the last revision: > > - Update porting_aix.cpp > - Update porting_aix.cpp > - Update os_aix.cpp Regarding `libclang.a`, Joachim told me that we would need to load `libclang.a(libclang.so.16)`. So that's a different issue which relates to `System.loadLibrary(libName)` (https://bugs.openjdk.org/browse/JDK-8319516) and not to this hotspot internal issue. - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1907311369
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v7]
On Fri, 5 Jan 2024 12:10:59 GMT, Martin Doerr wrote: > I have tried to build jextract > (https://github.com/openjdk/jextract/tree/jdk22) with LLVM > (https://github.com/llvm/llvm-project/releases/download/llvmorg-16.0.4/clang+llvm-16.0.4-powerpc64-ibm-aix-7.2.tar.xz). > I noticed that llvm mainly consists of .a files. So, I think we need to > support that for FFI compatibility with other libraries and open source > projects. Seems like this change is not sufficient for that. `clang` is compiled to `libclang.a` on AIX, but `libclang.so` on linux. I'm getting "System error: Exec format error" when trying to load `libclang.a` via `System.loadLibrary(libName);`. So the question remains: Are .a files really supposed to be dynamically loadable on AIX? If so, where is that documented? - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1894060171
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v12]
On Wed, 10 Jan 2024 11:49:05 GMT, Joachim Kern wrote: >> On AIX, repeated calls to dlopen referring to the same shared library may >> result in different, unique dl handles to be returned from libc. In that it >> differs from typical libc implementations that cache dl handles. >> >> This causes problems in the JVM with code that assumes equality of handles. >> One such problem is in the JVMTI agent handler. That problem was fixed with >> a local fix to said handler >> ([JDK-8315706](https://bugs.openjdk.org/browse/JDK-8315706)). However, this >> fix causes follow-up problems since it assumes that the file name passed to >> `os::dll_load()` is the file that has been opened. It prevents efficient, >> os_aix.cpp-local workarounds for other AIX issues like the *.so/*.a duality. >> See [JDK-8320005](https://bugs.openjdk.org/browse/JDK-8320005). As such, it >> is a hack that causes other, more uglier hacks to follow (see discussion of >> https://github.com/openjdk/jdk/pull/16604). >> >> We propose a different, cleaner way of handling this: >> >> - Handle this entirely inside the AIX versions of os::dll_load and >> os::dll_unload. >> - Cache dl handles; repeated opening of a library should return the cached >> handle. >> - Increase handle-local ref counter on open, Decrease it on close >> - Make sure calls to os::dll_load are matched to os::dll_unload (See >> [JDK-8320830](https://bugs.openjdk.org/browse/JDK-8320830)). >> >> This way we mimic dl handle equality as it is implemented on other >> platforms, and this works for all callers of os::dll_load. > > Joachim Kern has updated the pull request incrementally with two additional > commits since the last revision: > > - cosmetic changes > - cosmetic changes Looks correct to me. I'm not familiar with all AIX details, but they have been reviewed before. Test results look good. - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16920#pullrequestreview-1814204628
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v7]
On Wed, 20 Dec 2023 11:16:03 GMT, Suchismith Roy wrote: >> J2SE agent does not start and throws error when it tries to find the shared >> library ibm_16_am. >> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load >> fails.It fails to identify the shared library ibm_16_am.a shared archive >> file on AIX. >> Hence we are providing a function which will additionally search for .a file >> on AIX ,when the search for .so file fails. > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > Spaces fix I have tried to build jextract (https://github.com/openjdk/jextract/tree/jdk22) with LLVM (https://github.com/llvm/llvm-project/releases/download/llvmorg-16.0.4/clang+llvm-16.0.4-powerpc64-ibm-aix-7.2.tar.xz). I noticed that llvm mainly consists of .a files. So, I think we need to support that for FFI compatibility with other libraries and open source projects. - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1878570142
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v11]
On Fri, 22 Dec 2023 15:57:05 GMT, Joachim Kern wrote: >> On AIX, repeated calls to dlopen referring to the same shared library may >> result in different, unique dl handles to be returned from libc. In that it >> differs from typical libc implementations that cache dl handles. >> >> This causes problems in the JVM with code that assumes equality of handles. >> One such problem is in the JVMTI agent handler. That problem was fixed with >> a local fix to said handler >> ([JDK-8315706](https://bugs.openjdk.org/browse/JDK-8315706)). However, this >> fix causes follow-up problems since it assumes that the file name passed to >> `os::dll_load()` is the file that has been opened. It prevents efficient, >> os_aix.cpp-local workarounds for other AIX issues like the *.so/*.a duality. >> See [JDK-8320005](https://bugs.openjdk.org/browse/JDK-8320005). As such, it >> is a hack that causes other, more uglier hacks to follow (see discussion of >> https://github.com/openjdk/jdk/pull/16604). >> >> We propose a different, cleaner way of handling this: >> >> - Handle this entirely inside the AIX versions of os::dll_load and >> os::dll_unload. >> - Cache dl handles; repeated opening of a library should return the cached >> handle. >> - Increase handle-local ref counter on open, Decrease it on close >> - Make sure calls to os::dll_load are matched to os::dll_unload (See >> [JDK-8320830](https://bugs.openjdk.org/browse/JDK-8320830)). >> >> This way we mimic dl handle equality as it is implemented on other >> platforms, and this works for all callers of os::dll_load. > > Joachim Kern has updated the pull request incrementally with one additional > commit since the last revision: > > No need for malloc src/hotspot/os/aix/porting_aix.cpp line 975: > 973: return false; > 974: > 975: char* path2 = os::strdup (path); Whitespace between `os::strdup` and `(path)`. src/hotspot/os/aix/porting_aix.cpp line 1019: > 1017: } > 1018: > 1019: char* libpath = os::strdup (Libpath.base()); Whitespace! - PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1436947479 PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1436947817
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v8]
On Thu, 21 Dec 2023 11:45:36 GMT, Thomas Stuefe wrote: >> In principle you are right, but in my opinion 1024 is an academical limit. I >> never saw processes with more than a few dozen loaded libraries. > > Dynamic allocation also opens us up to potential initialization issues, > unless we explicitly use raw ::malloc. It should work, but I think its better > avoided unless we really need it. Well we're fixing an academic issue by introducing another one? Doesn't make sense to me. - PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433972207
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v8]
On Thu, 21 Dec 2023 09:57:08 GMT, Thomas Stuefe wrote: >> There will be only few libraries in the list. With this assumption Thomas >> suggested to use just a simple array. > > Let's keep it simple. A linear array of only a few items is easily scanned, > probably faster than pointer hopping hash table entries. Not that it matters > in any way for the few calls to dlopen. > > Also, avoiding hotspot structures preserves layer integrity (porting_aix does > not pull anything from hotspot so far) and prevents initialisation time > dependencies. Not sure whether ConcurrentHashTable works before VM init, but > with Joachimes current solution, we can call dlopen at any time in VM life. I don't like introducing unnecessary limitations. Are we sure nobody will ever need more than 1024 handles? Can't we at least use a GrowableArray or something like that? - PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433865222
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v8]
On Wed, 20 Dec 2023 14:53:06 GMT, Joachim Kern wrote:
>> On AIX, repeated calls to dlopen referring to the same shared library may
>> result in different, unique dl handles to be returned from libc. In that it
>> differs from typical libc implementations that cache dl handles.
>>
>> This causes problems in the JVM with code that assumes equality of handles.
>> One such problem is in the JVMTI agent handler. That problem was fixed with
>> a local fix to said handler
>> ([JDK-8315706](https://bugs.openjdk.org/browse/JDK-8315706)). However, this
>> fix causes follow-up problems since it assumes that the file name passed to
>> `os::dll_load()` is the file that has been opened. It prevents efficient,
>> os_aix.cpp-local workarounds for other AIX issues like the *.so/*.a duality.
>> See [JDK-8320005](https://bugs.openjdk.org/browse/JDK-8320005). As such, it
>> is a hack that causes other, more uglier hacks to follow (see discussion of
>> https://github.com/openjdk/jdk/pull/16604).
>>
>> We propose a different, cleaner way of handling this:
>>
>> - Handle this entirely inside the AIX versions of os::dll_load and
>> os::dll_unload.
>> - Cache dl handles; repeated opening of a library should return the cached
>> handle.
>> - Increase handle-local ref counter on open, Decrease it on close
>> - Make sure calls to os::dll_load are matched to os::dll_unload (See
>> [JDK-8320830](https://bugs.openjdk.org/browse/JDK-8320830)).
>>
>> This way we mimic dl handle equality as it is implemented on other
>> platforms, and this works for all callers of os::dll_load.
>
> Joachim Kern has updated the pull request incrementally with one additional
> commit since the last revision:
>
> improve error handling
A pretty complex solution, but I couldn't spot any real bug. Please consider my
suggestions.
src/hotspot/os/aix/porting_aix.cpp line 25:
> 23: */
> 24: // needs to be defined first, so that the implicit loaded xcoff.h header
> defines
> 25: // the right structures to analyze the loader header of 32 and 64 Bit
> executable files
I don't think we support 32 bit executables.
src/hotspot/os/aix/porting_aix.cpp line 916:
> 914: constexpr int max_handletable = 1024;
> 915: static int g_handletable_used = 0;
> 916: static struct handletableentry g_handletable[max_handletable] = {{0, 0,
> 0, 0}};
Wouldn't `ConcurrentHashTable` be a better data structure? It is already used
in hotspot, can grow dynamically and doesn't need linear search.
src/hotspot/os/aix/porting_aix.cpp line 921:
> 919: // If the libpath cannot be retrieved return an empty path
> 920: static const char* rtv_linkedin_libpath() {
> 921: static char buffer[4096];
Maybe define a constant for the buffer size?
src/hotspot/os/aix/porting_aix.cpp line 927:
> 925: // let libpath point to buffer, which then contains a valid libpath
> 926: // or an empty string
> 927: if (libpath) {
`!= nullptr` is common in hotspot.
src/hotspot/os/aix/porting_aix.cpp line 934:
> 932: // to open it
> 933: snprintf(buffer, 100, "/proc/%ld/object/a.out", (long)getpid());
> 934: FILE* f = 0;
Should be nullptr.
src/hotspot/os/aix/porting_aix.cpp line 990:
> 988: }
> 989: ret = (0 == stat64x(combined.base(), stat));
> 990: os::free (path2);
Please remove the extra whitespace.
src/hotspot/os/aix/porting_aix.cpp line 1026:
> 1024:
> 1025: os::free (libpath);
> 1026: os::free (path2);
Same here.
-
PR Review: https://git.openjdk.org/jdk/pull/16920#pullrequestreview-1791807521
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433267331
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433283111
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433273616
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433270399
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433289382
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433290839
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433291127
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v5]
On Mon, 18 Dec 2023 10:25:57 GMT, Thomas Stuefe wrote: >> Joachim Kern has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - trailing whitespace >> - Following most of Thomas proposals > > src/hotspot/os/aix/os_aix.cpp line 1113: > >> : } >> 1112: >> 1113: > > please remove whitespace change +1 - PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1430352685
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v6]
On Mon, 18 Dec 2023 11:30:59 GMT, Joachim Kern wrote:
>> On AIX, repeated calls to dlopen referring to the same shared library may
>> result in different, unique dl handles to be returned from libc. In that it
>> differs from typical libc implementations that cache dl handles.
>>
>> This causes problems in the JVM with code that assumes equality of handles.
>> One such problem is in the JVMTI agent handler. That problem was fixed with
>> a local fix to said handler
>> ([JDK-8315706](https://bugs.openjdk.org/browse/JDK-8315706)). However, this
>> fix causes follow-up problems since it assumes that the file name passed to
>> `os::dll_load()` is the file that has been opened. It prevents efficient,
>> os_aix.cpp-local workarounds for other AIX issues like the *.so/*.a duality.
>> See [JDK-8320005](https://bugs.openjdk.org/browse/JDK-8320005). As such, it
>> is a hack that causes other, more uglier hacks to follow (see discussion of
>> https://github.com/openjdk/jdk/pull/16604).
>>
>> We propose a different, cleaner way of handling this:
>>
>> - Handle this entirely inside the AIX versions of os::dll_load and
>> os::dll_unload.
>> - Cache dl handles; repeated opening of a library should return the cached
>> handle.
>> - Increase handle-local ref counter on open, Decrease it on close
>> - Make sure calls to os::dll_load are matched to os::dll_unload (See
>> [JDK-8320830](https://bugs.openjdk.org/browse/JDK-8320830)).
>>
>> This way we mimic dl handle equality as it is implemented on other
>> platforms, and this works for all callers of os::dll_load.
>
> Joachim Kern has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Followed Thomas proposals
I like getting rid of `#ifdef AIX` in shared code. The change is not simple,
but looks basically good to me. I'll take a closer look when I find more time.
I have some coding style requests. Please also see
https://wiki.openjdk.org/display/HotSpot/StyleGuide (especially section
Whitespace).
src/hotspot/os/aix/porting_aix.cpp line 964:
> 962:
> 963: return libpath;
> 964:
Empty line could get removed.
src/hotspot/os/aix/porting_aix.cpp line 985:
> 983: if (strchr(path2, '/')) {
> 984: stringStream combined;
> 985: if (*path2 == '/' || *path2 == '.')
We usually use `{` and `}` unless for extremely simple substatements on the
same line
src/hotspot/share/runtime/os.hpp line 1068:
> 1066: static bool pd_dll_unload(void* libhandle, char* ebuf, int ebuflen);
> 1067:
> 1068:
Please remove empty lines.
-
PR Review: https://git.openjdk.org/jdk/pull/16920#pullrequestreview-1787236876
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1430362306
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1430366154
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1430367434
Re: RFR: 8315966: Relativize initial_sp in interpreter frames [v2]
On Tue, 26 Sep 2023 11:41:52 GMT, Fredrik Bredberg wrote: >> Relativize initial_sp in interpreter frames. >> >> By changing the "initial_sp" (AKA "monitor_block_top" or "monitors" on >> PowerPC) member in interpreter frames from being an absolute address into an >> offset that is relative to the frame pointer, we don't need to change the >> value as we freeze and thaw frames of virtual threads. This is since we >> might freeze and thaw from and to different worker threads, so the absolute >> address to locals might change, but the offset from the frame pointer will >> be constant. >> >> This subtask only handles relativization of "initial_sp" and >> "monitor_block_top" since it's the same slot in interpreter frames (roughly >> the same as "monitors" on PowerPC). Relativization of other interpreter >> frame members are handled in other subtasks to JDK-8289296. >> >> Tested tier1-tier7 on supported platforms. The rest was sanity tested using >> Qemu. > > Fredrik Bredberg has updated the pull request incrementally with one > additional commit since the last revision: > > Updated after review. PPC64 part looks great! Thanks! - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15815#pullrequestreview-1644420114
Re: RFR: 8315966: Relativize initial_sp in interpreter frames
On Tue, 19 Sep 2023 09:00:01 GMT, Fredrik Bredberg wrote: > Relativize initial_sp in interpreter frames. > > By changing the "initial_sp" (AKA "monitor_block_top" or "monitors" on > PowerPC) member in interpreter frames from being an absolute address into an > offset that is relative to the frame pointer, we don't need to change the > value as we freeze and thaw frames of virtual threads. This is since we might > freeze and thaw from and to different worker threads, so the absolute address > to locals might change, but the offset from the frame pointer will be > constant. > > This subtask only handles relativization of "initial_sp" and > "monitor_block_top" since it's the same slot in interpreter frames (roughly > the same as "monitors" on PowerPC). Relativization of other interpreter frame > members are handled in other subtasks to JDK-8289296. > > Tested tier1-tier7 on supported platforms. The rest was sanity tested using > Qemu. Tests have passed and the PPC64 parts look good. - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15815#pullrequestreview-1635181336
Re: RFR: 8315966: Relativize initial_sp in interpreter frames
On Tue, 19 Sep 2023 09:00:01 GMT, Fredrik Bredberg wrote: > Relativize initial_sp in interpreter frames. > > By changing the "initial_sp" (AKA "monitor_block_top" or "monitors" on > PowerPC) member in interpreter frames from being an absolute address into an > offset that is relative to the frame pointer, we don't need to change the > value as we freeze and thaw frames of virtual threads. This is since we might > freeze and thaw from and to different worker threads, so the absolute address > to locals might change, but the offset from the frame pointer will be > constant. > > This subtask only handles relativization of "initial_sp" and > "monitor_block_top" since it's the same slot in interpreter frames (roughly > the same as "monitors" on PowerPC). Relativization of other interpreter frame > members are handled in other subtasks to JDK-8289296. > > Tested tier1-tier7 on supported platforms. The rest was sanity tested using > Qemu. Thanks for implementing the platform parts! Note that you could have used R0 which is typically available as scratch reg. But, ok, we have enough other regs. Looks correct. I'll run more tests. - PR Comment: https://git.openjdk.org/jdk/pull/15815#issuecomment-1725731457
Re: RFR: JDK-8314389: AttachListener::pd_set_flag obsolete
On Wed, 16 Aug 2023 08:09:42 GMT, Matthias Baesken wrote: > AttachListener::pd_set_flag is the same across platforms (always returning > JNI_ERR ). So it can be centralized or removed. Nice cleanup! - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15304#pullrequestreview-1582978311
Re: RFR: 8305341: Alignment should be enforced by alignas instead of compiler specific attributes [v6]
On Fri, 23 Jun 2023 02:43:38 GMT, Julian Waters wrote: >> C11 has been stable for a long time on all platforms, so native code can use >> the standard alignas operator for alignment requirements > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 18 commits: > > - Merge branch 'openjdk:master' into patch-6 > - Whitespace > - Revert > - _MSC_VER > - Restore visCPP > - Restore gcc attribute > - Revert gcc > - Revert > - Semicolon > - gcc offset_of > - ... and 8 more: https://git.openjdk.org/jdk/compare/5a82fa3b...6c289fb3 Marked as reviewed by mdoerr (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13258#pullrequestreview-1497130480
Integrated: 8309462: [AIX] vmTestbase/nsk/jvmti/RunAgentThread/agentthr001/TestDescription.java crashing due to empty while loop
On Mon, 12 Jun 2023 14:36:56 GMT, Martin Doerr wrote: > Test fix for test failing on AIX because of undefined behavior in current > implementation. This pull request has now been integrated. Changeset: 08eff92b Author:Martin Doerr URL: https://git.openjdk.org/jdk21/commit/08eff92b5e1e42cf299c1da8d5fa88d92b840505 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod 8309462: [AIX] vmTestbase/nsk/jvmti/RunAgentThread/agentthr001/TestDescription.java crashing due to empty while loop Reviewed-by: mbaesken Backport-of: cf9e6353cc6fe9e57a7a9883813d09892e7e7621 - PR: https://git.openjdk.org/jdk21/pull/9
Re: RFR: 8309462: [AIX] vmTestbase/nsk/jvmti/RunAgentThread/agentthr001/TestDescription.java crashing due to empty while loop
On Mon, 12 Jun 2023 14:36:56 GMT, Martin Doerr wrote: > Test fix for test failing on AIX because of undefined behavior in current > implementation. Thanks for the review! I'm planning to integrate tomorrow. - PR Comment: https://git.openjdk.org/jdk21/pull/9#issuecomment-1587497638
RFR: 8309462: [AIX] vmTestbase/nsk/jvmti/RunAgentThread/agentthr001/TestDescription.java crashing due to empty while loop
Test fix for test failing on AIX because of undefined behavior in current implementation. - Commit messages: - Backport cf9e6353cc6fe9e57a7a9883813d09892e7e7621 Changes: https://git.openjdk.org/jdk21/pull/9/files Webrev: https://webrevs.openjdk.org/?repo=jdk21=9=00 Issue: https://bugs.openjdk.org/browse/JDK-8309462 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk21/pull/9.diff Fetch: git fetch https://git.openjdk.org/jdk21.git pull/9/head:pull/9 PR: https://git.openjdk.org/jdk21/pull/9
Re: RFR: JDK-8309462: vmTestbase/nsk/jvmti/RunAgentThread/agentthr001/TestDescription.java crashing due to empty while loop [v3]
On Wed, 7 Jun 2023 14:04:14 GMT, JoKern65 wrote:
>> The sys_thread_3() function contains an empty while loop, which by the
>> standard can be optimized away. Please refer to discussion in
>> https://github.com/llvm/llvm-project/issues/60622
>> The xlc17 compiler is doing so, and IBM claims that they are following the
>> standard and will not fix this on compiler side.
>> So we have (at least) 3 ways to circumvent this behavior.
>>
>> 1. we can introduce the call of a nop library function, which will hinder
>> the optimizer to throw away the loop (This is our proposed solution, but
>> instead of a heavy looping thread, the result is a more or less idle thread):
>> `#include `
>> `static void`
>> `sys_thread_3(jvmtiEnv* jvmti, JNIEnv* jni, void *p)`
>> `{`
>> `while (1) {`
>> ` sleep(1);`
>> `}`
>> `}`
>>
>> 2. We can make use of a volatile variable in the loop body which also
>> hinders the optimizer to throw away the loop:
>> `static void`
>> `sys_thread_3(jvmtiEnv* jvmti, JNIEnv* jni, void *p)`
>> `{`
>> `volatile int i = 1;`
>> `while (i) {`
>> ` i += 2;`
>> `}`
>> `}`
>>
>> 3. we can use the __attribute__ ((optnone)) modifier in the function
>> declaration to suppress the optimization at all:
>> `static void`
>> `sys_thread_3(jvmtiEnv* jvmti, JNIEnv* jni, void *p) __attribute__
>> ((optnone))`
>> `{`
>> `while (1) {`
>> `}`
>> `}`
>>
>> To make the third approach platform independent, we can implement it in the
>> following way:
>> In globalDefinitions.hpp
>> `#ifndef OPTNONE`
>> `#define OPTNONE`
>> `#endif`
>>
>> In globalDefinitions_xlc.hpp
>> `// optnone support`
>> `//`
>> `// To use if a function should not be optimized`
>> `// Usage:`
>> `// void* func(size_t size) OPTNONE {...}`
>> `#define OPTNONE __attribute__(( optnone))`
>>
>> With this we can change libagentthr001.cpp in a platform independent way to
>> `static void`
>> `sys_thread_3(jvmtiEnv* jvmti, JNIEnv* jni, void *p) OPTNONE`
>> `{`
>> ` while (1) {`
>> ` }`
>> `}`
>
> JoKern65 has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove d042520_libagentthr.diff
I don't like empty endless loops in general, but I think your solution is ok
for a test.
-
Marked as reviewed by mdoerr (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14330#pullrequestreview-1467779695
Re: RFR: JDK-8309462: vmTestbase/nsk/jvmti/RunAgentThread/agentthr001/TestDescription.java crashing due to empty while loop
On Tue, 6 Jun 2023 09:51:09 GMT, JoKern65 wrote:
> The sys_thread_3() function contains an empty while loop, which by the
> standard can be optimized away. Please refer to discussion in
> https://github.com/llvm/llvm-project/issues/60622
> The xlc17 compiler is doing so, and IBM claims that they are following the
> standard and will not fix this on compiler side.
> So we have (at least) 3 ways to circumvent this behavior.
>
> 1. we can introduce the call of a nop library function, which will hinder the
> optimizer to throw away the loop (This is our proposed solution, but instead
> of a heavy looping thread, the result is a more or less idle thread):
> `#include `
> `static void`
> `sys_thread_3(jvmtiEnv* jvmti, JNIEnv* jni, void *p)`
> `{`
> `while (1) {`
> ` sleep(1);`
> `}`
> `}`
>
> 2. We can make use of a volatile variable in the loop body which also hinders
> the optimizer to throw away the loop:
> `static void`
> `sys_thread_3(jvmtiEnv* jvmti, JNIEnv* jni, void *p)`
> `{`
> `volatile int i = 1;`
> `while (i) {`
> ` i += 2;`
> `}`
> `}`
>
> 3. we can use the __attribute__ ((optnone)) modifier in the function
> declaration to suppress the optimization at all:
> `static void`
> `sys_thread_3(jvmtiEnv* jvmti, JNIEnv* jni, void *p) __attribute__
> ((optnone))`
> `{`
> `while (1) {`
> `}`
> `}`
>
> To make the third approach platform independent, we can implement it in the
> following way:
> In globalDefinitions.hpp
> `#ifndef OPTNONE`
> `#define OPTNONE`
> `#endif`
>
> In globalDefinitions_xlc.hpp
> `// optnone support`
> `//`
> `// To use if a function should not be optimized`
> `// Usage:`
> `// void* func(size_t size) OPTNONE {...}`
> `#define OPTNONE __attribute__(( optnone))`
>
> With this we can change libagentthr001.cpp in a platform independent way to
> `static void`
> `sys_thread_3(jvmtiEnv* jvmti, JNIEnv* jni, void *p) OPTNONE`
> `{`
> ` while (1) {`
> ` }`
> `}`
Changes requested by mdoerr (Reviewer).
test/hotspot/jtreg/vmTestbase/nsk/jvmti/RunAgentThread/agentthr001/agentthr001.cpp
line 26:
> 24: #include
> 25: #include
> 26: #include
Breaks Windows build: agentthr001.cpp(26): fatal error C1083: Cannot open
include file: 'unistd.h': No such file or directory
-
PR Review: https://git.openjdk.org/jdk/pull/14330#pullrequestreview-1466074354
PR Review Comment: https://git.openjdk.org/jdk/pull/14330#discussion_r1220327772
Re: RFR: JDK-8309225: Fix xlc17 clang 15 warnings in security and servicability
On Fri, 2 Jun 2023 10:19:53 GMT, JoKern65 wrote:
> This pr is a split off from JDK-8308288: Fix xlc17 clang warnings in shared
> code https://github.com/openjdk/jdk/pull/14146
> It handles the part in security and servicability.
>
> Compiling on AIX with xlc17 which contains the new clang 15 frontend shows
> the following warnings:
>
> src/java.security.jgss/share/native/libj2gss/NativeUtil.h:30:
> src/java.security.jgss/share/native/libj2gss/gssapi.h:48:5: error:
> 'TARGET_OS_MAC' is not defined, evaluates to 0 [-Werror,-Wundef]
> #if TARGET_OS_MAC && (defined(ppc) || defined(ppc64) || defined(i386) ||
> defined(x86_64))
> ^
> TARGET_OS_MAC is not defined. Instead of disabling the warning, I could
> ` #ifndef TARGET_OS_MAC`
> `#define TARGET_OS_MAC=0`
> `#endif`
> But this is already handled by disabling the warning for gcc.
>
> src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c:718:33: error:
> suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
> struct in6_addr mappedAny = IN6ADDR_ANY_INIT;
> ^~~~
> /usr/include/netinet/in.h:454:32: note: expanded from macro 'IN6ADDR_ANY_INIT'
> #define IN6ADDR_ANY_INIT {0, 0, 0, 0}
LGTM.
-
Marked as reviewed by mdoerr (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14282#pullrequestreview-1465140605
Re: RFR: JDK-8309462: vmTestbase/nsk/jvmti/RunAgentThread/agentthr001/TestDescription.java crashing due to empty while loop
On Tue, 6 Jun 2023 09:51:09 GMT, JoKern65 wrote:
> The sys_thread_3() function contains an empty while loop, which by the
> standard can be optimized away. Please refer to discussion in
> https://github.com/llvm/llvm-project/issues/60622
> The xlc17 compiler is doing so, and IBM claims that they are following the
> standard and will not fix this on compiler side.
> So we have (at least) 3 ways to circumvent this behavior.
>
> 1. we can introduce the call of a nop library function, which will hinder the
> optimizer to throw away the loop (This is our proposed solution, but instead
> of a heavy looping thread, the result is a more or less idle thread):
> `#include `
> `static void`
> `sys_thread_3(jvmtiEnv* jvmti, JNIEnv* jni, void *p)`
> `{`
> `while (1) {`
> ` sleep(1);`
> `}`
> `}`
>
> 2. We can make use of a volatile variable in the loop body which also hinders
> the optimizer to throw away the loop:
> `static void`
> `sys_thread_3(jvmtiEnv* jvmti, JNIEnv* jni, void *p)`
> `{`
> `volatile int i = 1;`
> `while (i) {`
> ` i += 2;`
> `}`
> `}`
>
> 3. we can use the __attribute__ ((optnone)) modifier in the function
> declaration to suppress the optimization at all:
> `static void`
> `sys_thread_3(jvmtiEnv* jvmti, JNIEnv* jni, void *p) __attribute__
> ((optnone))`
> `{`
> `while (1) {`
> `}`
> `}`
>
> To make the third approach platform independent, we can implement it in the
> following way:
> In globalDefinitions.hpp
> `#ifndef OPTNONE`
> `#define OPTNONE`
> `#endif`
>
> In globalDefinitions_xlc.hpp
> `// optnone support`
> `//`
> `// To use if a function should not be optimized`
> `// Usage:`
> `// void* func(size_t size) OPTNONE {...}`
> `#define OPTNONE __attribute__(( optnone))``
>
> With this we can change libagentthr001.cpp in a platform independent way to
> `static void`
> `sys_thread_3(jvmtiEnv* jvmti, JNIEnv* jni, void *p) OPTNONE`
> `{`
> ` while (1) {`
> ` }`
> `}`
LGTM. `sleep(1)` is my preferred solution. Thanks for fixing it!
-
Marked as reviewed by mdoerr (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14330#pullrequestreview-1465026778
Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental) [v4]
On Fri, 2 Jun 2023 18:24:16 GMT, Y. Srinivas Ramakrishna wrote: >> Kelvin Nilsen has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Force PLAB sizes to align on card-table size > > src/hotspot/cpu/riscv/gc/shenandoah/c1/shenandoahBarrierSetC1_riscv.cpp line > 4: > >> 2: * Copyright (c) 2018, 2019, Red Hat, Inc. All rights reserved. >> 3: * Copyright (c) 2020, 2021, Huawei Technologies Co., Ltd. All rights >> reserved. >> 4: * Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. > > This should be backed out, since it seems that there is no (other) change to > this fie. Yes. And also from files which were changed by non-Amazon employees only, please. - PR Review Comment: https://git.openjdk.org/jdk/pull/14185#discussion_r1214693296
Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental) [v4]
On Fri, 2 Jun 2023 02:49:25 GMT, Kelvin Nilsen wrote: >> OpenJDK Colleagues: >> >> Please review this proposed integration of Generational mode for Shenandoah >> GC under https://bugs.openjdk.org/browse/JDK-8307314. >> >> Generational mode of Shenandoah is enabled by adding >> `-XX:+UnlockExperimentalVMOptions -XX:ShenandoahGCMode=generational` to a >> command line that already specifies ` -XX:+UseShenandoahGC`. The >> implementation automatically adjusts the sizes of old generation and young >> generation to efficiently utilize the entire heap capacity. Generational >> mode of Shenandoah resembles G1 in the following regards: >> >> 1. Old-generation marking runs concurrently during the time that multiple >> young generation collections run to completion. >> 2. After old-generation marking completes, we perform a sequence of mixed >> collections. Each mixed collection combines collection of young generation >> with evacuation of a portion of the old-generation regions identified for >> collection based on old-generation marking information. >> 3. Unlike G1, young-generation collections and evacuations are entirely >> concurrent, as with single-generation Shenandoah. >> 4. As with single-generation Shenandoah, there is no explicit notion of eden >> and survivor space within the young generation. In practice, regions that >> were most recently allocated tend to have large amounts of garbage and these >> regions tend to be collected with very little effort. Young-generation >> objects that survive garbage collection tend to accumulate in regions that >> hold survivor objects. These regions tend to have smaller amounts of >> garbage, and are less likely to be collected. If they survive a sufficient >> number of young-generation collections, the “survivor” regions are promoted >> into the old generation. >> >> We expect to refine heuristics as we gain experience with more production >> workloads. In the future, we plan to remove the “experimental” qualifier >> from generational mode, at which time we expect that generational mode will >> become the default mode for Shenandoah. >> >> **Testing**: We continuously run jtreg tiers 1-4 + hotspot_gc_shenandoah, >> gcstress, jck compiler, jck runtime, Dacapo, SpecJBB, SpecVM, Extremem, >> HyperAlloc, and multiple AWS production workload simulators. We test on >> Linux x64 and aarch64, Alpine x64 and aarch64, macOS x64 and aarch64, and >> Windows x64. > > Kelvin Nilsen has updated the pull request incrementally with one additional > commit since the last revision: > > Force PLAB sizes to align on card-table size > # Internal Error (shenandoahVerifier.cpp:1244), pid=2951116, tid=2951124 > # Error: Verify init-mark remembered set violation; clean card should be > dirty I've seen the same issue on PPC64: https://bugs.openjdk.org/browse/JDK-8309371 - PR Comment: https://git.openjdk.org/jdk/pull/14185#issuecomment-1574125489
Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental)
On Fri, 26 May 2023 20:46:29 GMT, Kelvin Nilsen wrote: > OpenJDK Colleagues: > > Please review this proposed integration of Generational mode for Shenandoah > GC under https://bugs.openjdk.org/browse/JDK-8307314. > > Generational mode of Shenandoah is enabled by adding > `-XX:+UnlockExperimentalVMOptions -XX:ShenandoahGCMode=generational` to a > command line that already specifies ` -XX:+UseShenandoahGC`. The > implementation automatically adjusts the sizes of old generation and young > generation to efficiently utilize the entire heap capacity. Generational > mode of Shenandoah resembles G1 in the following regards: > > 1. Old-generation marking runs concurrently during the time that multiple > young generation collections run to completion. > 2. After old-generation marking completes, we perform a sequence of mixed > collections. Each mixed collection combines collection of young generation > with evacuation of a portion of the old-generation regions identified for > collection based on old-generation marking information. > 3. Unlike G1, young-generation collections and evacuations are entirely > concurrent, as with single-generation Shenandoah. > 4. As with single-generation Shenandoah, there is no explicit notion of eden > and survivor space within the young generation. In practice, regions that > were most recently allocated tend to have large amounts of garbage and these > regions tend to be collected with very little effort. Young-generation > objects that survive garbage collection tend to accumulate in regions that > hold survivor objects. These regions tend to have smaller amounts of > garbage, and are less likely to be collected. If they survive a sufficient > number of young-generation collections, the “survivor” regions are promoted > into the old generation. > > We expect to refine heuristics as we gain experience with more production > workloads. In the future, we plan to remove the “experimental” qualifier > from generational mode, at which time we expect that generational mode will > become the default mode for Shenandoah. > > **Testing**: We continuously run jtreg tiers 1-4 + hotspot_gc_shenandoah, > gcstress, jck compiler, jck runtime, Dacapo, SpecJBB, SpecVM, Extremem, > HyperAlloc, and multiple AWS production workload simulators. We test on Linux > x64 and aarch64, Alpine x64 and aarch64, macOS x64 and aarch64, and Windows > x64. Issues already reported to GenShen engineers: gc/shenandoah/TestElasticTLAB.java#generational # Internal Error (src\hotspot\share\gc\shenandoah\shenandoahFreeSet.cpp:695), pid=23288, tid=23784 # assert(size % CardTable::card_size_in_words() == 0) failed: size must be multiple of card table size, was 258 gc/stress/gcold/TestGCOldWithShenandoah.java#generational # Internal Error (src\hotspot\share\gc\shenandoah\heuristics\shenandoahOldHeuristics.cpp:82), pid=20828, tid=5836 # assert(_old_generation->available() > old_evacuation_budget) failed: Cannot budget more than is available gc/shenandoah/oom/TestAllocOutOfMemory.java#large Execution failed: `main' threw exception: java.lang.RuntimeException: 'java.lang.OutOfMemoryError: Java heap space' missing from stdout/stderr (Issue with 64k Pages) gc/shenandoah/TestRetainObjects.java#no-tlab gc/shenandoah/TestSieveObjects.java#no-tlab Timeouts. gc/shenandoah/TestAllocObjects.java#generational gc/shenandoah/TestDynamicSoftMaxHeapSize.java#generational # Internal Error src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp:664), pid=18434, tid=29955 # assert(is_global() || ShenandoahHeap::heap()->is_full_gc_in_progress() || (_used + _humongous_waste <= _affiliated_region_count * ShenandoahHeapRegion::region_size_bytes())) failed: used cannot exceed regions - PR Comment: https://git.openjdk.org/jdk/pull/14185#issuecomment-1571947174
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 16:58:41 GMT, Thomas Stuefe wrote: >> The crazy thing is that `malloc` is defined! That means all places where we >> use the term malloc are getting replaced without such a workaround. (E.g. >> for log tags.) > > So, we do this only for malloc? Not for calloc, posix_memalign, realloc etc? > What about free? > > As ugly as defining malloc is (and I remember QADRT), I hesitate about > removing that define. > > Removing that define and hard-coding it here assumes 1) our replacement is > equivalent (ok, easy to check) 2) it will always be equivalent in future AIX > versions 3) pointers it returns work with the unchanged free() and realloc() > the system provides, and will always do so. > > I don't know... I would not do this just to get rid of a warning. This one is not just to get rid of a warning. We get real test errors because malloc gets replaced by vec_malloc in log tags. - PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1207308708
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 08:31:46 GMT, JoKern65 wrote: >> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk >> on AIX , we run into various "warnings as errors". >> Some of those are in shared codebase and could be addressed by small >> adjustments. >> A lot of those changes are in hotspot, some might be somewhere else in the >> OpenJDK C/C++ code. > > JoKern65 has updated the pull request incrementally with one additional > commit since the last revision: > > forgotton _ Just a side note: The warning elimination is new for AIX compilers. We had given it up at earlier times: https://bugs.openjdk.org/browse/JDK-8202325 I still appreciate if we can get warning free and it makes sense to review all of them. But, I wouldn't require it for JDK21. - PR Comment: https://git.openjdk.org/jdk/pull/14146#issuecomment-1564406986
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code
On Thu, 25 May 2023 15:04:32 GMT, Kim Barrett wrote:
>> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk
>> on AIX , we run into various "warnings as errors".
>> Some of those are in shared codebase and could be addressed by small
>> adjustments.
>> A lot of those changes are in hotspot, some might be somewhere else in the
>> OpenJDK C/C++ code.
>
> src/hotspot/share/utilities/globalDefinitions_xlc.hpp line 47:
>
>> 45: #undef malloc
>> 46: extern void *malloc(size_t) asm("vec_malloc");
>> 47: #endif
>
> Wow! And I don't mean that in a good way. I'm not questioning whether this
> is correct, just commenting
> on how crazy it seems that such a thing might be needed.
The crazy thing is that `malloc` is defined! That means all places where we use
the term malloc are getting replaced without such a workaround. (E.g. for log
tags.)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1205867287
Re: RFR: 8307058: Implementation of Generational ZGC [v3]
On Thu, 4 May 2023 09:50:23 GMT, Axel Boldt-Christmas
wrote:
>>> I'm getting build warnings on all linux platforms with gcc-11.3.0:
>>>
>>> ```
>>> src/hotspot/share/gc/z/zDriver.cpp:84:13: error: In the GNU C Library,
>>> "minor" is defined
>>> by . For historical compatibility, it is
>>> currently defined by as well, but we plan to
>>> remove this soon. To use "minor", include
>>> directly. If you did not intend to use a system-defined macro
>>> "minor", you should undefine it after including . [-Werror]
>>>84 | ZDriverMinor* ZDriver::minor() {
>>> ```
>>
>> That's unfortunate as minor and major are quite central to Generational ZGC
>> and having to rename those functions will make the code look worse. I wonder
>> if we should undef minor and major where needed.
>
>> I'm getting build warnings on all linux platforms with gcc-11.3.0:
>>
>> ```
>> src/hotspot/share/gc/z/zDriver.cpp:84:13: error: In the GNU C Library,
>> "minor" is defined
>> by . For historical compatibility, it is
>> currently defined by as well, but we plan to
>> remove this soon. To use "minor", include
>> directly. If you did not intend to use a system-defined macro
>> "minor", you should undefine it after including . [-Werror]
>>84 | ZDriverMinor* ZDriver::minor() {
>> ```
>
> @TheRealMDoerr I cannot reproduce this with gcc but can see the issue with
> clangd.
> Can you check if this patch solves the issue you are seeing?
>
> diff --git a/src/hotspot/share/gc/z/zDriver.hpp
> b/src/hotspot/share/gc/z/zDriver.hpp
> index 640ea6575ef..7fa650b1fa1 100644
> --- a/src/hotspot/share/gc/z/zDriver.hpp
> +++ b/src/hotspot/share/gc/z/zDriver.hpp
> @@ -29,6 +29,14 @@
> #include "gc/z/zThread.hpp"
> #include "gc/z/zTracer.hpp"
>
> +#ifdef minor
> +#undef minor
> +#endif
> +
> +#ifdef major
> +#undef major
> +#endif
> +
> class VM_ZOperation;
> class ZDriverMinor;
> class ZDriverMajor;
@xmas92: Thanks for your quick solution. Your patch solves the problem. If you
want to integrate it, please also add a comment why this is needed.
-
PR Comment: https://git.openjdk.org/jdk/pull/13771#issuecomment-1534563624
Re: RFR: 8307058: Implementation of Generational ZGC [v3]
On Wed, 3 May 2023 19:36:55 GMT, Stefan Karlsson wrote: >> Hi all, >> >> Please review the implementation of Generational ZGC, which can be turned on >> by adding -XX:+ZGenerational in addition to using -XX:+UseZGC. Generational >> ZGC is a major rewrite of the non-generational ZGC version that exists in >> the openjdk/jdk repository. It splits the heap into two generations; the >> young generation where newly allocated objects are born, and the old >> generation where long-lived objects get promoted to. The motivation for >> introducing generations is to allow ZGC to reclaim memory faster by not >> having to walk the entire object graph every time a garbage collection is >> run. This should make Generational ZGC suitable for more workloads. In >> particular workloads that previously hit allocation stalls because of high >> allocation rates, large live sets, or limited spare machine resources, have >> the potential to work better with Generational ZGC. For an in-depth >> description of Generational ZGC, see https://openjdk.org/jeps/439. >> >> The development of Generational ZGC started around the same time as the >> development of JDK 17. At that point we forked off the Generational ZGC >> development into its own branch and let non-generational live unaffected in >> openjdk/jdk. This safe-guarded non-generational ZGC and allowed Generational >> ZGC to move unhindered, without the shackles of having to fit into another >> GC implementation's design and quirks. Since then, almost all of the ZGC >> files have been changed. Moving forward to today, when it's ready for us to >> upstream Generational ZGC, we now need to deliver Generational ZGC without >> disrupting our current user-base. We have therefore opted to initially >> include both versions of ZGC in the code base, but with the intention to >> deprecate non-generational ZGC in a future release. Existing users running >> with only -XX:+UseZGC will get the non-generational ZGC, and users that want >> the new Generational ZGC need to run with -XX:+ZGenerational in addition to >> -XX:+UseZGC. The intention is to give the users time to validate and deploy their workloads with the new GC implementation. >> >> Including both the new evolution of a GC and its legacy predecessor poses a >> few challenges for us GC developers. The first reaction could be to try to >> mash the two implementations together and sprinkle the GC code with >> conditional statements or dynamic dispatches. We have done similar >> experiments before. When ZGC was first born, we started an experiment where >> we converted G1 into getting the same features as the evolving ZGC. It was >> quite clear to us how time consuming and complex things end up being when we >> tried to keep both the original G1 working, and at the same time implemented >> the ZGC-alike G1. Given this experience, we don't see that as a viable >> solution to deliver a maintainable and evolving Generational ZGC. Our >> pragmatic suggestion to these challenges is to let Generational ZGC live >> under the current gc/z directories and let the legacy, non-generational ZGC >> be completely separated in its own directories. This way we can continue to >> move quickly with the continued develo pment of Generational ZGC and let the non-generational ZGC be mostly untouched until it gets deprecated, and eventually removed. The non-generational ZGC directory will be gc/x and all the classes of non-generational have been prefixed with X instead of Z. An alternative to this rename could be to namespace out non-generational ZGC. We experimented with that, but it was too easy to accidentally cross-compile Generational ZGC code into non-generational ZGC, so we didn't like that approach. >> >> Most of the stand-alone cleanups and enhancements outside of the ZGC code >> have already been upstreamed to openjdk/jdk. There are still a few patches >> that could/should be pushed separately, but they will be easier to >> understand by also looking at the Generational ZGC code, so they will be >> sent out after this PR has been published. The patches that could be >> published separately are: >> >> * 59d1e96af6a UPSTREAM: Introduce check_oop infrastructure to check oops in >> the oop class >> * ca9edf8aa79 UPSTREAM: RISCV tmp reg cleanup resolve_jobject >> * 4bec9c69b67 CLEANUP: barrierSetNMethod_aarch64.cpp >> * b67d03a3f04 UPSTREAM: Add relaxed add for aarch64 atomics >> * a2824734d23 UPSTREAM: lir_xchg >> * 36cd39c0126 UPSTREAM: assembler_ppc CMPLI >> * 447259cea42 UPSTREAM: assembler_ppc ANDI >> * 9417323499a UPSTREAM: Add VMErrorCallback infrastructure >> >> Regarding all the changesets you see in this PR, they form the history of >> the development of Generational ZGC. It might look a bit unconventional to >> what you are used to see in openjdk development. What we have done is to use >> merges with the 'ours' strategy to ignore the previous Generational ZGC >> patches,
Re: RFR: 8307058: Implementation of Generational ZGC [v2]
On Wed, 3 May 2023 10:55:49 GMT, Stefan Karlsson wrote: >> Hi all, >> >> Please review the implementation of Generational ZGC, which can be turned on >> by adding -XX:+ZGenerational in addition to using -XX:+UseZGC. Generational >> ZGC is a major rewrite of the non-generational ZGC version that exists in >> the openjdk/jdk repository. It splits the heap into two generations; the >> young generation where newly allocated objects are born, and the old >> generation where long-lived objects get promoted to. The motivation for >> introducing generations is to allow ZGC to reclaim memory faster by not >> having to walk the entire object graph every time a garbage collection is >> run. This should make Generational ZGC suitable for more workloads. In >> particular workloads that previously hit allocation stalls because of high >> allocation rates, large live sets, or limited spare machine resources, have >> the potential to work better with Generational ZGC. For an in-depth >> description of Generational ZGC, see https://openjdk.org/jeps/439. >> >> The development of Generational ZGC started around the same time as the >> development of JDK 17. At that point we forked off the Generational ZGC >> development into its own branch and let non-generational live unaffected in >> openjdk/jdk. This safe-guarded non-generational ZGC and allowed Generational >> ZGC to move unhindered, without the shackles of having to fit into another >> GC implementation's design and quirks. Since then, almost all of the ZGC >> files have been changed. Moving forward to today, when it's ready for us to >> upstream Generational ZGC, we now need to deliver Generational ZGC without >> disrupting our current user-base. We have therefore opted to initially >> include both versions of ZGC in the code base, but with the intention to >> deprecate non-generational ZGC in a future release. Existing users running >> with only -XX:+UseZGC will get the non-generational ZGC, and users that want >> the new Generational ZGC need to run with -XX:+ZGenerational in addition to >> -XX:+UseZGC. The intention is to give the users time to validate and deploy their workloads with the new GC implementation. >> >> Including both the new evolution of a GC and its legacy predecessor poses a >> few challenges for us GC developers. The first reaction could be to try to >> mash the two implementations together and sprinkle the GC code with >> conditional statements or dynamic dispatches. We have done similar >> experiments before. When ZGC was first born, we started an experiment where >> we converted G1 into getting the same features as the evolving ZGC. It was >> quite clear to us how time consuming and complex things end up being when we >> tried to keep both the original G1 working, and at the same time implemented >> the ZGC-alike G1. Given this experience, we don't see that as a viable >> solution to deliver a maintainable and evolving Generational ZGC. Our >> pragmatic suggestion to these challenges is to let Generational ZGC live >> under the current gc/z directories and let the legacy, non-generational ZGC >> be completely separated in its own directories. This way we can continue to >> move quickly with the continued develo pment of Generational ZGC and let the non-generational ZGC be mostly untouched until it gets deprecated, and eventually removed. The non-generational ZGC directory will be gc/x and all the classes of non-generational have been prefixed with X instead of Z. An alternative to this rename could be to namespace out non-generational ZGC. We experimented with that, but it was too easy to accidentally cross-compile Generational ZGC code into non-generational ZGC, so we didn't like that approach. >> >> Most of the stand-alone cleanups and enhancements outside of the ZGC code >> have already been upstreamed to openjdk/jdk. There are still a few patches >> that could/should be pushed separately, but they will be easier to >> understand by also looking at the Generational ZGC code, so they will be >> sent out after this PR has been published. The patches that could be >> published separately are: >> >> * 59d1e96af6a UPSTREAM: Introduce check_oop infrastructure to check oops in >> the oop class >> * ca9edf8aa79 UPSTREAM: RISCV tmp reg cleanup resolve_jobject >> * 4bec9c69b67 CLEANUP: barrierSetNMethod_aarch64.cpp >> * b67d03a3f04 UPSTREAM: Add relaxed add for aarch64 atomics >> * a2824734d23 UPSTREAM: lir_xchg >> * 36cd39c0126 UPSTREAM: assembler_ppc CMPLI >> * 447259cea42 UPSTREAM: assembler_ppc ANDI >> * 9417323499a UPSTREAM: Add VMErrorCallback infrastructure >> >> Regarding all the changesets you see in this PR, they form the history of >> the development of Generational ZGC. It might look a bit unconventional to >> what you are used to see in openjdk development. What we have done is to use >> merges with the 'ours' strategy to ignore the previous Generational ZGC >> patches,
Re: RFR: 8307058: Implementation of Generational ZGC [v2]
On Wed, 3 May 2023 10:55:49 GMT, Stefan Karlsson wrote: >> Hi all, >> >> Please review the implementation of Generational ZGC, which can be turned on >> by adding -XX:+ZGenerational in addition to using -XX:+UseZGC. Generational >> ZGC is a major rewrite of the non-generational ZGC version that exists in >> the openjdk/jdk repository. It splits the heap into two generations; the >> young generation where newly allocated objects are born, and the old >> generation where long-lived objects get promoted to. The motivation for >> introducing generations is to allow ZGC to reclaim memory faster by not >> having to walk the entire object graph every time a garbage collection is >> run. This should make Generational ZGC suitable for more workloads. In >> particular workloads that previously hit allocation stalls because of high >> allocation rates, large live sets, or limited spare machine resources, have >> the potential to work better with Generational ZGC. For an in-depth >> description of Generational ZGC, see https://openjdk.org/jeps/439. >> >> The development of Generational ZGC started around the same time as the >> development of JDK 17. At that point we forked off the Generational ZGC >> development into its own branch and let non-generational live unaffected in >> openjdk/jdk. This safe-guarded non-generational ZGC and allowed Generational >> ZGC to move unhindered, without the shackles of having to fit into another >> GC implementation's design and quirks. Since then, almost all of the ZGC >> files have been changed. Moving forward to today, when it's ready for us to >> upstream Generational ZGC, we now need to deliver Generational ZGC without >> disrupting our current user-base. We have therefore opted to initially >> include both versions of ZGC in the code base, but with the intention to >> deprecate non-generational ZGC in a future release. Existing users running >> with only -XX:+UseZGC will get the non-generational ZGC, and users that want >> the new Generational ZGC need to run with -XX:+ZGenerational in addition to >> -XX:+UseZGC. The intention is to give the users time to validate and deploy their workloads with the new GC implementation. >> >> Including both the new evolution of a GC and its legacy predecessor poses a >> few challenges for us GC developers. The first reaction could be to try to >> mash the two implementations together and sprinkle the GC code with >> conditional statements or dynamic dispatches. We have done similar >> experiments before. When ZGC was first born, we started an experiment where >> we converted G1 into getting the same features as the evolving ZGC. It was >> quite clear to us how time consuming and complex things end up being when we >> tried to keep both the original G1 working, and at the same time implemented >> the ZGC-alike G1. Given this experience, we don't see that as a viable >> solution to deliver a maintainable and evolving Generational ZGC. Our >> pragmatic suggestion to these challenges is to let Generational ZGC live >> under the current gc/z directories and let the legacy, non-generational ZGC >> be completely separated in its own directories. This way we can continue to >> move quickly with the continued develo pment of Generational ZGC and let the non-generational ZGC be mostly untouched until it gets deprecated, and eventually removed. The non-generational ZGC directory will be gc/x and all the classes of non-generational have been prefixed with X instead of Z. An alternative to this rename could be to namespace out non-generational ZGC. We experimented with that, but it was too easy to accidentally cross-compile Generational ZGC code into non-generational ZGC, so we didn't like that approach. >> >> Most of the stand-alone cleanups and enhancements outside of the ZGC code >> have already been upstreamed to openjdk/jdk. There are still a few patches >> that could/should be pushed separately, but they will be easier to >> understand by also looking at the Generational ZGC code, so they will be >> sent out after this PR has been published. The patches that could be >> published separately are: >> >> * 59d1e96af6a UPSTREAM: Introduce check_oop infrastructure to check oops in >> the oop class >> * ca9edf8aa79 UPSTREAM: RISCV tmp reg cleanup resolve_jobject >> * 4bec9c69b67 CLEANUP: barrierSetNMethod_aarch64.cpp >> * b67d03a3f04 UPSTREAM: Add relaxed add for aarch64 atomics >> * a2824734d23 UPSTREAM: lir_xchg >> * 36cd39c0126 UPSTREAM: assembler_ppc CMPLI >> * 447259cea42 UPSTREAM: assembler_ppc ANDI >> * 9417323499a UPSTREAM: Add VMErrorCallback infrastructure >> >> Regarding all the changesets you see in this PR, they form the history of >> the development of Generational ZGC. It might look a bit unconventional to >> what you are used to see in openjdk development. What we have done is to use >> merges with the 'ours' strategy to ignore the previous Generational ZGC >> patches,
Integrated: 8306823: Native memory leak in SharedRuntime::notify_jvmti_unmount/mount.
On Tue, 25 Apr 2023 13:19:36 GMT, Martin Doerr wrote: > The code introduced by > [JDK-8304303](https://bugs.openjdk.org/browse/JDK-8304303) uses > `JNIHandles::make_local` which requires `JNIHandles::destroy_local` which is > currently missing. This pull request has now been integrated. Changeset: d7476982 Author:Martin Doerr URL: https://git.openjdk.org/jdk/commit/d74769826ddb5e68df76407fb94c7560475249a0 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod 8306823: Native memory leak in SharedRuntime::notify_jvmti_unmount/mount. Reviewed-by: pchilanomate, sspitsyn - PR: https://git.openjdk.org/jdk/pull/13641
Re: RFR: 8306823: Native memory leak in SharedRuntime::notify_jvmti_unmount/mount.
On Tue, 25 Apr 2023 13:19:36 GMT, Martin Doerr wrote: > The code introduced by > [JDK-8304303](https://bugs.openjdk.org/browse/JDK-8304303) uses > `JNIHandles::make_local` which requires `JNIHandles::destroy_local` which is > currently missing. Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/13641#issuecomment-1522327187
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v17]
On Fri, 21 Apr 2023 20:26:12 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The value of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Correct comment on isPPC64() and refer to isLittleEndian() instead of > mentioning PPC64LE Thanks for the comment updates! - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13357#pullrequestreview-1397600716
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v15]
On Mon, 17 Apr 2023 20:59:06 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The value of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 17 commits: > > - Merge branch 'master' into 8304915-arch-enum > - ArchTest on Debian RISC-V 64 confirmed by reviewer > - Fixed isPPC64(). >Consolidated switch cases in ArchTest. >Moved mapping of build TARGET_OS and TARGET_CPU to the build >to avoid multiple mappings in more than one place. > - Correct mapping and test of ppc64 > - Add ppc64 as mapping to PPC64 Architecture > - Modified test to check Architecture is64bits() and isLittleEndian() >against Unsafe respective values. >Relocated code mapping OS name and arch name from PlatformProps to >OperatingSystem and Architecture. Kept the mapping of names >in the template close to where the values are filled in by the build. > - Remove unused static and import of Stabile > - Rename OperatingSystemProps to PlatformProps. >Refactor OperatingSystem initialization to use strings instead of integers. > - Revised mapping mechanism of build target architecture names to enum > values. >Unrecognized values from the build are mapped to enum value "OTHER". >Renamed PPC64LE to PPC64 to reflect only the architecture, not the > endianness. >Added an `isLittleEndian` method to return the endianness (not currently > used anywhere) > - Revert changes to jdk.accessibility AccessBridge > - ... and 7 more: https://git.openjdk.org/jdk/compare/8858d543...99a93b7e test/jdk/jdk/internal/util/ArchTest.java line 71: > 69: case "aarch64" -> AARCH64; > 70: case "riscv64" -> RISCV64; > 71: case "s390x", "s390" -> S390; // unverified This was also verified according to comments. Right, @offamitkumar? - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1171335657
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v13]
On Wed, 12 Apr 2023 17:31:49 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The value of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed isPPC64(). > Consolidated switch cases in ArchTest. > Moved mapping of build TARGET_OS and TARGET_CPU to the build > to avoid multiple mappings in more than one place. Thanks! PPC64 parts look good and the test has passed on both endianness versions. - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13357#pullrequestreview-1381866371
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v12]
On Tue, 11 Apr 2023 21:09:43 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The value of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Correct mapping and test of ppc64 Works on PPC64 Big Endian, now. However, little Endian fails: org.opentest4j.AssertionFailedError: Mismatch PPC64 == PPC64 vs isPPC64 ==> expected: but was: at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55) at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62) at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182) at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1152) at ArchTest.isArch(ArchTest.java:99) ... System property os.arch: "ppc64le", Architecture.current(): "PPC64" - PR Comment: https://git.openjdk.org/jdk/pull/13357#issuecomment-1504925182
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v11]
On Tue, 11 Apr 2023 18:35:56 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The value of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Add ppc64 as mapping to PPC64 Architecture Thanks for the updates! PPC64 looks basically good except of a upper / lower case issue in the test: STARTEDArchTest::nameVsCurrent 'nameVsCurrent()' org.opentest4j.AssertionFailedError: mismatch in Architecture.current vs ppc64 ==> expected: but was: ... FAILED ArchTest::nameVsCurrent 'nameVsCurrent()' JavaTest Message: JUnit Platform Failure(s): 1 - PR Comment: https://git.openjdk.org/jdk/pull/13357#issuecomment-1504064549
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v10]
On Tue, 11 Apr 2023 17:58:54 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The value of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Modified test to check Architecture is64bits() and isLittleEndian() > against Unsafe respective values. > Relocated code mapping OS name and arch name from PlatformProps to > OperatingSystem and Architecture. Kept the mapping of names > in the template close to where the values are filled in by the build. Another remark: Old JDK on s390 used "os.arch = zArch_64", current one "os.arch = s390x". @offamitkumar: You probably want to take a look. - PR Comment: https://git.openjdk.org/jdk/pull/13357#issuecomment-1503861585
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v10]
On Tue, 11 Apr 2023 17:58:54 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The value of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Modified test to check Architecture is64bits() and isLittleEndian() > against Unsafe respective values. > Relocated code mapping OS name and arch name from PlatformProps to > OperatingSystem and Architecture. Kept the mapping of names > in the template close to where the values are filled in by the build. test/jdk/jdk/internal/util/ArchTest.java line 74: > 72: case "riscv64" -> RISCV64; // unverified > 73: case "s390x", "s390" -> S390; // unverified > 74: case "ppc64le" -> PPC64; // unverified I think "ppc64" should also get mapped to "PPC64". - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1163161387
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v9]
On Tue, 11 Apr 2023 10:26:02 GMT, ExE Boss wrote: >> test/jdk/jdk/internal/util/ArchTest.java line 128: >> >>> 126: case RISCV64 -> true; >>> 127: case S390 -> false; >>> 128: case PPC64 -> true; >> >> This is not always true. The PPC64 architecture supports both endianness >> versions. AIX and legacy linux is Big Endian while recent linux is little >> Endian (determined by platform name "os.arch = ppc64le" instead of "os.arch >> = ppc64"). Querying the endianness is also possible, of course. > > This should (probably) be correct: > Suggestion: > > case PPC64 -> !OperatingSystem.isAix() && > Architecture.isLittleEndian(); Looks correct, but makes the test pointless for any linux on PPC64. - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1162628828
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v9]
On Sat, 8 Apr 2023 18:00:53 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The value of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Remove unused static and import of Stabile Would be great if you could support "os.arch = ppc64" for AIX and legacy linux, too. test/jdk/jdk/internal/util/ArchTest.java line 128: > 126: case RISCV64 -> true; > 127: case S390 -> false; > 128: case PPC64 -> true; This is not always true. The PPC64 architecture supports both endianness versions. AIX and legacy linux is Big Endian while recent linux is little Endian (determined by platform name "os.arch = ppc64le" instead of "os.arch = ppc64"). Querying the endianness is also possible, of course. - Changes requested by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13357#pullrequestreview-1378905956 PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1162600674
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v14]
On Tue, 28 Mar 2023 12:54:41 GMT, Amit Kumar wrote: >> Matias Saavedra Silva has updated the pull request incrementally with one >> additional commit since the last revision: >> >> RISCV patch and aarch64 improvement > > src/hotspot/cpu/ppc/templateInterpreterGenerator_ppc.cpp line 652: > >> 650: // Scale the index to be the entry index * >> sizeof(ResolvedInvokeDynamicInfo) >> 651: __ sldi(size, size, log2i_exact(sizeof(ResolvedIndyEntry))); >> 652: __ add(cache, cache, size); > > @reinrich Is there any specific reason, why you're not calling > load_resolved_indy_entry() method here. On s390x build/changes are stable > even with calling that helper method. It should work if we move the addition of `Array::base_offset_in_bytes()` into the other caller. - PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1150886679
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v11]
On Thu, 23 Mar 2023 15:06:21 GMT, Matias Saavedra Silva wrote: >> Matias Saavedra Silva has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Improved comment for load-acquire aarch64 > > src/hotspot/cpu/ppc/templateTable_ppc_64.cpp line 2294: > >> 2292: >> 2293: __ load_resolved_indy_entry(cache, index); >> 2294: __ ld_ptr(method, in_bytes(ResolvedIndyEntry::method_offset()), >> cache); > > @reinrich this load needs acquire semantics. We already have ordering by `isync(); // Order load wrt. succeeding loads.` below. - PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1146353107
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v6]
On Thu, 16 Mar 2023 09:07:27 GMT, Richard Reingruber wrote: >> src/hotspot/cpu/ppc/templateTable_ppc_64.cpp line 3398: >> >>> 3396: const Bytecodes::Code code = bytecode(); >>> 3397: const bool is_invokeinterface = code == >>> Bytecodes::_invokeinterface; >>> 3398: const bool is_invokedynamic= code == false; // should not reach >>> here with invokedynamic >> >> This looks strange! I guess you wanted to delete more? > > Basically I kept the local variable as a name for the (now) constant value > passed in the call at L3409. > > The parameter cannot be eliminated since `load_invoke_cp_cache_entry()` is > declared in a shared header. > > I could replace the variable reference in the call with `false /* > is_invokedynamic */` if you like that better. Personally I'd prefer the > current version. I meant `code == false`. That was probably not intended. - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v6]
On Wed, 15 Mar 2023 18:45:00 GMT, Matias Saavedra Silva wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC was provided by @reinrich and the RISCV port was provided by >> @DingliZhang and @zifeihan. >> >> This change supports the following platforms: x86, aarch64, PPC, and RISCV > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > Fixed aarch64 interpreter mistake src/hotspot/cpu/ppc/templateTable_ppc_64.cpp line 3398: > 3396: const Bytecodes::Code code = bytecode(); > 3397: const bool is_invokeinterface = code == Bytecodes::_invokeinterface; > 3398: const bool is_invokedynamic= code == false; // should not reach > here with invokedynamic This looks strange! I guess you wanted to delete more? - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8299795: Relativize locals in interpreter frames [v5]
On Thu, 19 Jan 2023 09:13:42 GMT, Fredrik Bredberg wrote: >> Implementation of relativized locals in interpreter frames for x86. x64, >> arm, aarch64, ppc64le and riscv. >> Not relativized locals on zero and s390 but done some changes to cope with >> the changed generic code. >> Tested tier1-tier8 on supported platforms. The rest was sanity tested using >> Qemu, except s390, which was only tested by GitHub Actions. > > Fredrik Bredberg has updated the pull request incrementally with one > additional commit since the last revision: > > Updated after review. Thanks! Your update and the PPC64 parts LGTM. - Marked as reviewed by mdoerr (Reviewer). PR: https://git.openjdk.org/jdk/pull/11902
Re: RFR: 8299795: Relativize locals in interpreter frames [v4]
On Wed, 18 Jan 2023 15:46:22 GMT, Fredrik Bredberg wrote: >> The code uses ()[R12_scratch2]. I think your comment is confusing. Please >> remove it or write something comprehensive. E.g. save R18_locals in >> compressed format relative to the new top frame's FP (= current SP). > > See what you mean, the R1_SP being the frame pointer is confusing. So how > about changing the comment into: > "// Store relativized R18_locals, see frame::interpreter_frame_locals()." > Would that work for you? If not I'll just remove it. That's fine. Thanks! - PR: https://git.openjdk.org/jdk/pull/11902
Re: RFR: 8299795: Relativize locals in interpreter frames [v4]
On Tue, 17 Jan 2023 17:02:04 GMT, Fredrik Bredberg wrote: >> src/hotspot/cpu/ppc/templateInterpreterGenerator_ppc.cpp line 1035: >> >>> 1033: __ sub(R12_scratch2, R18_locals, R1_SP); >>> 1034: __ srdi(R12_scratch2, R12_scratch2, >>> Interpreter::logStackElementSize); >>> 1035: // Now ()[R12_scratch2] == R18_locals >> >> I think this comment makes no sense. Please remove it. Otherwise, PPC64 code >> LGTM. > > My intention was to show how the contents of R18_locals (the absolute address > to locals) can be recreated by using R12_scratch2 as an index from the frame > pointer. Before we stored R18_locals in the stackframe, now we store > R12_scratch2. Does that make more sense? The code uses ()[R12_scratch2]. I think your comment is confusing. Please remove it or write something comprehensive. E.g. save R18_locals in compressed format relative to the frame's SP. - PR: https://git.openjdk.org/jdk/pull/11902
Re: RFR: 8299795: Relativize locals in interpreter frames [v4]
On Tue, 17 Jan 2023 08:35:45 GMT, Fredrik Bredberg wrote: >> Implementation of relativized locals in interpreter frames for x86. x64, >> arm, aarch64, ppc64le and riscv. >> Not relativized locals on zero and s390 but done some changes to cope with >> the changed generic code. >> Tested tier1-tier8 on supported platforms. The rest was sanity tested using >> Qemu, except s390, which was only tested by GitHub Actions. > > Fredrik Bredberg has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - Merge branch 'master' into relativize-locals-JDK-8299795_2023-01-09 > - Added references to JDK-8300197 > - Updated some copyright dates. > - Changed copyright date to 2023 > - 8299795: Relativize locals in interpreter frames src/hotspot/cpu/ppc/templateInterpreterGenerator_ppc.cpp line 1035: > 1033: __ sub(R12_scratch2, R18_locals, R1_SP); > 1034: __ srdi(R12_scratch2, R12_scratch2, Interpreter::logStackElementSize); > 1035: // Now ()[R12_scratch2] == R18_locals I think this comment makes no sense. Please remove it. Otherwise, PPC64 code LGTM. - PR: https://git.openjdk.org/jdk/pull/11902
Re: RFR: 8299795: Relativize locals in interpreter frames
On Tue, 17 Jan 2023 08:40:18 GMT, Fredrik Bredberg wrote: > > Works on PPC64. Thanks! Tests have passed on other platforms as well. > > Does "other platforms" include S390? No, @backwaterred you may want to check. - PR: https://git.openjdk.org/jdk/pull/11902
Re: RFR: 8299795: Relativize locals in interpreter frames
On Mon, 9 Jan 2023 10:30:06 GMT, Fredrik Bredberg wrote: > Implementation of relativized locals in interpreter frames for x86. x64, arm, > aarch64, ppc64le and riscv. > Not relativized locals on zero and s390 but done some changes to cope with > the changed generic code. > Tested tier1-tier8 on supported platforms. The rest was sanity tested using > Qemu, except s390, which was only tested by GitHub Actions. Works on PPC64. Thanks! Tests have passed on other platforms as well. - PR: https://git.openjdk.org/jdk/pull/11902
Re: RFR: 8296875: Generational ZGC: Refactor loom code [v5]
On Mon, 28 Nov 2022 12:14:59 GMT, Erik Österlund wrote: >> The current loom code makes some assumptions about GC that will not work >> with generational ZGC. We should make this code more GC agnostic, and >> provide a better interface for talking to the GC. >> >> In particular, >> 1) All GCs have a way of encoding oops inside of the heap differently to >> oops outside of the heap. For non-ZGC collectors, that is compressed oops. >> For ZGC, that is colored pointers. With generational ZGC, pointers on-heap >> will be colored and pointers off-heap will be "colorless". So we need to >> generalize encoding and decoding of oops in the heap, for loom. >> >> 2) The cont_oop is located on a stack. In order to access it we need to >> start_processing on that thread, if it isn't the current thread. This >> happened to work so far for ZGC, because the stale pointers had enough >> colors. But with generational ZGC, these on-stack oops will be colorless, so >> we have to be more accurate here and ensure processing really has started on >> any thread that cont_oop is used on. To make life a bit easier, I'm moving >> the oop processing responsibility for these oops to the thread instead. >> Currently there is no more than one of these, so doing it lazily per frame >> seems a bit overkill. >> >> 3) Refactoring the stack chunk allocation code >> >> Tested with tier1-5 and manually running Skynet. No regressions detected. We >> have also been running with this (yet a slightly different backend) in the >> generational ZGC repo for a while now. > > Erik Österlund has updated the pull request incrementally with one additional > commit since the last revision: > > Patricio concerns Thanks for the update! - PR: https://git.openjdk.org/jdk/pull/1
Re: RFR: 8296875: Generational ZGC: Refactor loom code [v5]
On Mon, 28 Nov 2022 12:14:59 GMT, Erik Österlund wrote: >> The current loom code makes some assumptions about GC that will not work >> with generational ZGC. We should make this code more GC agnostic, and >> provide a better interface for talking to the GC. >> >> In particular, >> 1) All GCs have a way of encoding oops inside of the heap differently to >> oops outside of the heap. For non-ZGC collectors, that is compressed oops. >> For ZGC, that is colored pointers. With generational ZGC, pointers on-heap >> will be colored and pointers off-heap will be "colorless". So we need to >> generalize encoding and decoding of oops in the heap, for loom. >> >> 2) The cont_oop is located on a stack. In order to access it we need to >> start_processing on that thread, if it isn't the current thread. This >> happened to work so far for ZGC, because the stale pointers had enough >> colors. But with generational ZGC, these on-stack oops will be colorless, so >> we have to be more accurate here and ensure processing really has started on >> any thread that cont_oop is used on. To make life a bit easier, I'm moving >> the oop processing responsibility for these oops to the thread instead. >> Currently there is no more than one of these, so doing it lazily per frame >> seems a bit overkill. >> >> 3) Refactoring the stack chunk allocation code >> >> Tested with tier1-5 and manually running Skynet. No regressions detected. We >> have also been running with this (yet a slightly different backend) in the >> generational ZGC repo for a while now. > > Erik Österlund has updated the pull request incrementally with one additional > commit since the last revision: > > Patricio concerns I think PPC64 needs the change, too, now: https://github.com/openjdk/jdk/blob/c05dc80234a6beff3fa4d2de3228928c639da083/src/hotspot/cpu/ppc/sharedRuntime_ppc.cpp#L1660 - PR: https://git.openjdk.org/jdk/pull/1
Integrated: 8297082: Remove sun/tools/jhsdb/BasicLauncherTest.java from problem list
On Thu, 24 Nov 2022 10:59:41 GMT, Martin Doerr wrote: > Related issue is fixed with > [JDK-8228649](https://bugs.openjdk.org/browse/JDK-8228649) > Test has passed. This pull request has now been integrated. Changeset: 012dafee Author: Martin Doerr URL: https://git.openjdk.org/jdk/commit/012dafee5bc035a2eb27ac5efab9658ade92bf90 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod 8297082: Remove sun/tools/jhsdb/BasicLauncherTest.java from problem list Reviewed-by: rrich, cjplummer - PR: https://git.openjdk.org/jdk/pull/11351
Re: RFR: 8297082: Remove sun/tools/jhsdb/BasicLauncherTest.java from problem list
On Thu, 24 Nov 2022 10:59:41 GMT, Martin Doerr wrote: > Related issue is fixed with > [JDK-8228649](https://bugs.openjdk.org/browse/JDK-8228649) > Test has passed. Thanks for the review! - PR: https://git.openjdk.org/jdk/pull/11351
Re: RFR: 8297082: Remove sun/tools/jhsdb/BasicLauncherTest.java from problem list
On Thu, 24 Nov 2022 10:59:41 GMT, Martin Doerr wrote: > Related issue is fixed with > [JDK-8228649](https://bugs.openjdk.org/browse/JDK-8228649) > Test has passed. Thanks for the review! I'll probably integrate on Monday. - PR: https://git.openjdk.org/jdk/pull/11351
RFR: 8297082: Remove sun/tools/jhsdb/BasicLauncherTest.java from problem list
Related issue is fixed with [JDK-8228649](https://bugs.openjdk.org/browse/JDK-8228649) Test has passed. - Commit messages: - 8297082: Remove sun/tools/jhsdb/BasicLauncherTest.java from problem list Changes: https://git.openjdk.org/jdk/pull/11351/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11351=00 Issue: https://bugs.openjdk.org/browse/JDK-8297082 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/11351.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11351/head:pull/11351 PR: https://git.openjdk.org/jdk/pull/11351
