On Tue, 7 Jul 2026 17:05:22 GMT, Daniel D. Daugherty <[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 2413:
>
>> 2411:
>> 2412: class StopThreadAsyncClosure : public AsyncExceptionHandshakeClosure {
>> 2413: public:
>
> Nit: Needs a one space indent.
Done.
> src/hotspot/share/prims/jvmtiEnvBase.hpp line 537:
>
>> 535:
>> 536: // HandshakeClosure to send an asynchronous exception to thread.
>> 537: class StopThreadClosure : public JvmtiUnitedHandshakeClosure {
>
> Not to `thread`. This should be to `target`.
Changed.
> src/hotspot/share/prims/jvmtiEnvBase.hpp line 538:
>
>> 536: // HandshakeClosure to send an asynchronous exception to thread.
>> 537: class StopThreadClosure : public JvmtiUnitedHandshakeClosure {
>> 538: private:
>
> Nit: Needs a one space indent.
Done.
> src/hotspot/share/prims/jvmtiEnvBase.hpp line 540:
>
>> 538: private:
>> 539: Handle _exception;
>> 540: public:
>
> Nit: Needs a one space indent.
Done.
> src/hotspot/share/prims/jvmtiEnvBase.hpp line 543:
>
>> 541: StopThreadClosure(JavaThread* thread, oop exception)
>> 542: : JvmtiUnitedHandshakeClosure("StopThread"),
>> 543: _exception(thread, exception) {}
>
> Nit: I think `thread` should `current_thread` in two places.
Yes, changed.
> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 129:
>
>> 127:
>> 128: class ThreadInVMfromJava : public ThreadStateTransition {
>> 129: AtNoAsyncEntryMark nam;
>
> Nit: usually these variable names match the uppercase
> case part of the helper class, e.g.: `anaem` instead of `nam`.
Changed.
> src/hotspot/share/runtime/javaThread.hpp line 1359:
>
>> 1357: public:
>> 1358: AtNoAsyncEntryMark(JavaThread *t, bool b)
>> 1359: : _target(t), _count(!b) {
>
> Nit: I usually use `jt` for a JavaThread* var and `t` for a Thread* var.
>
> I'm having trouble with the `b` parameter name that is assigned to `_count`
> after negation.
> Why is this naming so obscure? Perhaps `_do_count` for the local and
> something better
> for `b`?
Right, I renamed them and now pass `!check_asyncs` from `ThreadInVMfromJava` so
it should be easier to read.
> test/hotspot/jtreg/serviceability/jvmti/vthread/StopThreadTest2/libStopThreadTest2.cpp
> line 48:
>
>> 46: Java_StopThreadTest2_stopThread(JNIEnv *jni, jclass cls, jthread thread,
>> jobject exception) {
>> 47: jvmtiError err = jvmti->StopThread(thread, exception);
>> 48: if (err != JVMTI_ERROR_OPAQUE_FRAME) {
>
> A 1-line comment about why it is okay to ignore OPAQUE_FRAME would be good.
Added comment.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547524686
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547526974
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547524129
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547524364
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547525619
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547518469
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547523332
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3547527581
