On Wed, 16 Jul 2025 03:32:34 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:

>> Fix how ThreadReference.popFrame() and ThreadReference.forceEarlyReturn deal 
>> with JDWP OPAQUE_FRAME error.
>> 
>> Before virtual threads, OpaqueFrameException did not exist and these API 
>> always threw NativeMethodException when JDWP OPAQUE_FRAME error was 
>> returned. For virtual threads OpaqueFrameException was added to handle the 
>> case where a virtual thread was not suspended at an event, so the JDI 
>> implementation was updated to throw OpaqueFrameException if it detected that 
>> a native method was not the cause. It turns out however that JVMTI (and 
>> therefore JDWP) can return OPAQUE_FRAME error for reasons other than a 
>> native method or the special virtual thread case, and for platform threads 
>> we were incorrectly throwing NativeMethodException in these cases. This PR 
>> fixes that. For platform threads we now only throw NativeMethodException if 
>> a native method is detected, and otherwise throw OpaqueFrameException.
>> 
>> The spec language is also being cleaned up to better align with JVMTI. 
>> Rather than calling out all the reasons for OpaqueFrameException, a more 
>> generic explanation is given.
>> 
>> This is somewhat of a preliminary PR so I can get some feedback. I still 
>> need to do a CR and complete testing.
>
> Chris Plummer has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update based on feedback

I've posted one concern on the spec updates. Otherwise, it looks okay to me.
I guess, a CSR will be filed for this update. I'm not sure what do you mean by 
CR in the description. Do I understand this right that a potential JDWP spec 
update we touched out of this PR will be considered separately?

src/jdk.jdi/share/classes/com/sun/jdi/ThreadReference.java line 406:

> 404:      *
> 405:      * @throws OpaqueFrameException if target VM is unable to pop this 
> frame
> 406:      * (e.g. a virtual thread is not suspended at an event).

Nit: I'm not sure the comment at line 406 is fully correct. The 
`IncompatibleThreadStateException` will be thrown if a virtual thread is not 
suspended. So, the `OpaqueFrameException` will be thrown only when a virtual 
thread is suspended but not at an event. The same concern should apply for the 
line  488.

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

Marked as reviewed by sspitsyn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/26335#pullrequestreview-3028456818
PR Review Comment: https://git.openjdk.org/jdk/pull/26335#discussion_r2212652535

Reply via email to