On Fri, 3 Jul 2026 02:15:44 GMT, David Holmes <[email protected]> wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Dan's comments
>> - David's comments
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 2405:
>
>> 2403: return m->method_holder() == vmClasses::VirtualThread_klass()
>> 2404: // Checks for VirtualThread$VThreadContinuation$1.run
>> 2405: || (m->name() == vmSymbols::run_method_name() &&
>> m->jvmti_hide_events());
>
> Suggestion:
>
> return (m != nullptr) &&
> (m->method_holder() == vmClasses::VirtualThread_klass() ||
> // Checks for VirtualThread$VThreadContinuation$1.run
> (m->name() == vmSymbols::run_method_name() &&
> m->jvmti_hide_events()));
Done.
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 2428:
>
>> 2426:
>> 2427: void
>> 2428: StopThreadClosure::doit(JavaThread *target) {
>
> Suggestion:
>
> StopThreadClosure::doit(JavaThread* target) {
Done.
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 2435:
>
>> 2433:
>> 2434: void
>> 2435: StopThreadClosure::do_thread(Thread *target) {
>
> Suggestion:
>
> StopThreadClosure::do_thread(Thread* target) {
Done.
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 2469:
>
>> 2467: vframeStream vfst(_target_jt);
>> 2468: Method* m = vfst.method();
>> 2469: if (is_async_unsafe_method(m)) {
>
> I assume `m` is the current Java method. Simply checking if that method is
> unsafe seems inadequate as we may be in the VT code but calling a method of
> another class when we process the async-exception. Doesn't this need to be an
> actual walk of the stack to look for any unsafe method?
The intention was to fix the reported case, and only as best-effort try to
cover other potential ones. That’s why `is_async_unsafe_method` simply checks
for any `VirtualThread` method at the top. It covers the known problematic
cases that could lead to the reported crash (`yieldContinuation`,
`VirtualThread$VThreadContinuation$1.run`, and the ones marked as
`@JvmtiMountTransition`), while possibly covering other unsafe cases. Walking
the stack like that would take it a step further. We could do that if you
prefer, we would just have to exclude checking for methods `VirtualThread.run`
and `VirtualThread$VThreadContinuation$1.run` (except at the top) as they are
always present.
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 2476:
>
>> 2474: }
>> 2475:
>> 2476: if (_target_jt->at_no_async_entry_count() > 0
>
> The count is set by the target thread but is read here by the current thread
> - we need some use of atomics here.
Yes, but this is executed inside the handshake which already provides the
required synchronization.
> src/hotspot/share/prims/jvmtiEnvBase.hpp line 545:
>
>> 543: _exception(thread, exception) {}
>> 544: void doit(JavaThread *target);
>> 545: void do_thread(Thread *target);
>
> Suggestion:
>
> void doit(JavaThread* target);
> void do_thread(Thread* target);
Done.
> src/hotspot/share/runtime/javaThread.hpp line 236:
>
>> 234: friend class HandshakeState;
>> 235:
>> 236: int _at_no_async_entry_count;
>
> Suggestion:
>
> int _at_no_async_entry_count; // Tracks whether we are within a
> JRT_ENTRY_NOASYNC function.
Added a similar abbreviated comment. It is expanded below on
`AtNoAsyncEntryMark`.
> src/hotspot/share/runtime/javaThread.hpp line 1355:
>
>> 1353:
>> 1354: class AtNoAsyncEntryMark : public StackObj {
>> 1355: JavaThread *_target;
>
> Suggestion:
>
> JavaThread* _target;
Done.
> src/hotspot/share/runtime/javaThread.hpp line 1358:
>
>> 1356: bool _count;
>> 1357: public:
>> 1358: AtNoAsyncEntryMark(JavaThread *t, bool b)
>
> Suggestion:
>
> AtNoAsyncEntryMark(JavaThread* t, bool allows_async)
I renamed `_count` and `b` based on Dan’s suggestion above.
> test/hotspot/jtreg/serviceability/jvmti/vthread/StopThreadTest2/StopThreadTest2.java
> line 63:
>
>> 61: suspendAllVirtualThreads();
>> 62: for (Thread vthread : vthreads) {
>> 63: stopThread(vthread, new ThreadDeath());
>
> `ThreadDeath` is deprecated for removal so perhaps better to define a custom
> exception type, or just use a generic type.
Replaced with `RuntimeException`.
> test/hotspot/jtreg/serviceability/jvmti/vthread/StopThreadTest2/libStopThreadTest2.cpp
> line 46:
>
>> 44:
>> 45: JNIEXPORT void JNICALL
>> 46: Java_StopThreadTest2_stopThread(JNIEnv *jni, jclass cls, jthread thread,
>> jobject exception) {
>
> Suggestion:
>
> Java_StopThreadTest2_stopThread(JNIEnv* jni, jclass cls, jthread thread,
> jobject exception) {
Done.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547478415
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547487093
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547487669
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547491785
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547494953
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547492383
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547493590
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547495687
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547499243
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547503693
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547480847
