On Tue, 2 May 2023 19:11:14 GMT, Serguei Spitsyn <[email protected]> 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
