On Fri, 11 Jul 2025 22:18:09 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> Serguei Spitsyn has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains seven additional
>> commits since the last revision:
>>
>> - Merge
>> - review: minor tweak of previous change
>> - review: corrected OPAQUE_FRAME clarification for NotifyFramePop function
>> - review: (1) remove vthread specific clarifications; unify GetLocal* and
>> SetLocal* with other functions
>> - review: tweak the OPAQUE_FRAME clarifications for ForceEarlyReturn*
>> functions
>> - review: tweak OPAQUE_FRAME clarification for NotifyFramePop function
>> - 8309399: JVMTI spec needs to clarify when OPAQUE_FRAME is thrown for
>> reasons other than a native method
>
> src/hotspot/share/prims/jvmti.xml line 5904:
>
>> 5902: <error id="JVMTI_ERROR_OPAQUE_FRAME">
>> 5903: The implementation is unable to get the frame locals
>> 5904: (e.g. the frame at <code>depth</code> is executing a native
>> method).
>
> Is this true, especially the native method handling? I'm looking at changes
> needed on the JDWP and JDI side, and currently they don't even handle
> OPAQUE_FRAME. I get the feeling native methods produce
> JVMTI_ERROR_INVALID_SLOT .
>
> For JDWP and JDI things are similar for GetLocalXXX() with the exception that
> they expect OPAQUE_FRAME when not dealing with a mounted virtual thread
> suspended at a breakpoint. So basically that means OPAQUE_FRAME handling did
> not exist before virtual threads.
Thank you for the concern. But I'm kind of puzzled with your observation.
The JVMTI implementation should return `JVMTI_ERROR_OPAQUE_FRAME` for platform
threads.
Though I wonder if it also was the case before virtual thread support was added.
The `VM_BaseGetOrSetLocal::doit()` have the following checks in place:
void VM_BaseGetOrSetLocal::doit() {
. . .
frame fr = _jvf->fr();
if (_set && _depth != 0 &&
Continuation::is_frame_in_continuation(_jvf->thread(), fr)) {
_result = JVMTI_ERROR_OPAQUE_FRAME; // deferred locals are not fully
supported in continuations
return;
}
Method* method = _jvf->method();
if (getting_receiver()) {
if (method->is_static()) {
_result = JVMTI_ERROR_INVALID_SLOT; ### This check is for
GetLocalInstance
return;
}
} else {
if (method->is_native()) {
_result = JVMTI_ERROR_OPAQUE_FRAME; ### This check is before calls to
### check_slot_type_no_lvt() and
check_slot_type_lvt()
return;
}
if (!check_slot_type_no_lvt(_jvf)) { ### This has checks for
JVMTI_ERROR_INVALID_SLOT
return;
}
if (method->has_localvariable_table() &&
!check_slot_type_lvt(_jvf)) { ### This has checks for
JVMTI_ERROR_INVALID_SLOT
return;
}
}
. . .
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26111#discussion_r2205878604
