RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes

[Alex Menkov]

On Tue, 28 May 2024 22:24:53 GMT, Serguei Spitsyn <sspitsyn at 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