RFR: 8306471: Add virtual threads support to JDWP ThreadReference.Stop and JDI ThreadReference.stop() [v4]

[Chris Plummer]

On Tue, 2 May 2023 19:11:14 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> Chris Plummer has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Some test logging improvements.
>
> src/java.se/share/data/jdwp/jdwp.spec line 2024:
> 
>> 2022:             (Error THREAD_NOT_SUSPENDED "The thread is a virtual thread and was not suspended.")
>> 2023:             (Error OPAQUE_FRAME   "The thread is a suspended virtual thread and the implementation "
>> 2024:                                   "was unable to throw an asynchronous exception from this frame.")
> 
> Should it be aligned with JVMTI and some other places in your fix and say
>  "from the current frame" instead of "from this frame"?

I'm waiting for your JVMTI PR to finish review. I don't want to have to change this more than once.

> src/jdk.jdi/share/classes/com/sun/jdi/ThreadReference.java line 132:
> 
>> 130:      * @throws OpaqueFrameException if the thread is a suspended
>> 131:      * virtual thread and the implementation was unable to throw an
>> 132:      * asynchronous exception from this frame
> 
> The same comment as for the jdwp spec:
> Should it be aligned with JVMTI and some other places in your fix and say
>  "from the current frame" instead of "from this frame"?

I'm waiting for your JVMTI PR to finish review. I don't want to have to change this more than once.

> src/jdk.jdi/share/classes/com/sun/tools/jdi/ThreadReferenceImpl.java line 279:
> 
>> 277:             case JDWP.Error.OPAQUE_FRAME:
>> 278:                 assert isVirtual(); // can only happen with virtual threads
>> 279:                 throw new OpaqueFrameException();
> 
> Should the OpaqueFrameException also provide a message?

The implementation tends to only include an exception message when it helps to clarify the reason for the exception. The spec only gives one possible reason for this exception, so no clarification should be needed. Plus it would be hard to meaningfully convey this reason in a short message. Here's what the spec says:

     * @throws OpaqueFrameException if the thread is a suspended
     * virtual thread and the implementation was unable to throw an
     * asynchronous exception from this frame

> test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/stop/stop002t.java line 78:
> 
>> 76:         /*
>> 77:          * TEST #2: async exception while suspended at a breakpoint.
>> 78:          */
> 
> Where is a similar comment for TEST #1 ?
> Would it make sense to implement each subtest as a separate method?

Test #1 does not involve the debuggee since it is suppose to fail on the JDI side with an exception.

I don't think separate methods helps much here. It might even make it a bit harder to understand the flow.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1182992243
PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1182992404
PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1182995311
PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1182997188