On Tue, 28 May 2024 22:24:53 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
> Please, review the following `interp-only` issue related to carrier threads.
> There are 3 problems fixed here:
> - The `EnterInterpOnlyModeClosure::do_threads` is taking the
> `JvmtiThreadState` with the `jt->jvmti_thread_state()` which is incorrect
> when we have a deal with a carrier thread. The target state is known at the
> point when the `HandshakeClosure` is set, so the fix is to pass it as a
> constructor parameter.
> - The `state->is_pending_interp_only_mode())` was processed at mounts only
> but it has to be processed for unmounts as well.
> - The test
> `test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp`
> has a wrong assumption that there can't be `MethodExit` event on the carrier
> thread when the function `breakpoint_hit1` is being executed. However, it can
> happen if the virtual thread gets unmounted.
>
> The fix also includes new test case `vthread/CarrierThreadEventNotification`
> developed by Patricio.
>
> Testing:
> - Ran new test case locally
> - Ran mach5 tiers 1-6
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/CarrierThreadEventNotification.java
line 2:
> 1: /*
> 2: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
(c) 2024
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
line 2:
> 1: /*
> 2: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
(c) 2024
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
line 40:
> 38:
> 39: static const char* CTHREAD_NAME_START = "ForkJoinPool";
> 40: static const int CTHREAD_NAME_START_LEN = (int)strlen("ForkJoinPool");
should be `size_t` (the value is used for `strncmp` which expects `size_t`)
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
line 44:
> 42: static jint
> 43: get_cthreads(JNIEnv* jni, jthread** cthreads_p) {
> 44: jthread* tested_cthreads = NULL;
Suggestion:
jthread* tested_cthreads = nullptr;
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
line 44:
> 42: static jint
> 43: get_cthreads(JNIEnv* jni, jthread** cthreads_p) {
> 44: jthread* tested_cthreads = NULL;
This local variable has the same name as global.
I'd suggest to rename the local var or remove it (and the function should set
both `tested_cthreads` and ` cthread_cnt`)
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
line 91:
> 89: for (int i = 0; i < cthread_cnt; i++) {
> 90: jthread thread = tested_cthreads[i];
> 91: char* tname = get_thread_name(jvmti, jni, thread);
`tname` is not needed
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619642814
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619642981
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619643931
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619660102
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619662506
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619665290
