On Thu, 23 Jun 2022 00:36:19 GMT, Serguei Spitsyn <[email protected]> wrote:
>> I guess what I'm not understanding here is how/why "fr.is_heap_frame())"
>> translates into "we are not single stepping or at a breakpoint". I know we
>> already did the check earlier to make sure we are "depth == 0", and I
>> understand that in this code we are handling the special case of "depth ==
>> 0" possibly not indicating we are truly in the topmost frame. From Ron's
>> explanation, the frame has been popped, but the callee frame has not been
>> thawed yet. Is the "fr.is_heap_frame())" check telling us the frame hasn't
>> been thawed. If so, I would call this out in the comment. Basically say
>> something like:
>>
>> "If the topmost frame is a heap frame, then it hasn't been thawed. This can
>> happen if we are executing at a return barrier safepoint. The callee frame
>> has been popped, but the caller frame has not been thawed. We can't support
>> a setlocal in the callee frame at this point, because we aren't truly in the
>> callee yet."
>>
>> Also, I think the following comment is misleading:
>>
>> `deferred locals currently unsupported in continuations`
>>
>> That's not true. They are supported when single stepping and at breakpoints,
>> or more accurately, in the topmost frame. Looks like this is copy-n-paste
>> from the following code with the same issue:
>>
>>
>> 613 if (_set && _depth != 0 &&
>> Continuation::is_frame_in_continuation(_jvf->thread(), fr)) {
>> 614 _result = JVMTI_ERROR_OPAQUE_FRAME; // deferred locals currently
>> unsupported in continuations
>> 615 return;
>> 616 }
>
>> I guess what I'm not understanding here is how/why "fr.is_heap_frame())"
>> translates into "we are not single stepping or at a breakpoint".
>
> It is impossible to have `fr.is_heap_frame()` if we are at a single step or
> at a breakpoint. The frame has to be active and really executed at any event
> point. It can't be frozen because a native callback is executed at the top.
> Do you think, we should explain this as well?
>
> I like your suggestion for the comment wording but I'm not sure if it is good
> enough.
>
>> Also, I think the following comment is misleading:
>
> I'll update this comment as well to make it more accurate.
How about this change:
git diff src/hotspot/share/prims/jvmtiImpl.cpp
diff --git a/src/hotspot/share/prims/jvmtiImpl.cpp
b/src/hotspot/share/prims/jvmtiImpl.cpp
index c34558c9ad5..8a7396e3cee 100644
--- a/src/hotspot/share/prims/jvmtiImpl.cpp
+++ b/src/hotspot/share/prims/jvmtiImpl.cpp
@@ -611,7 +611,7 @@ 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 currently
unsupported in continuations
+ _result = JVMTI_ERROR_OPAQUE_FRAME; // deferred locals are not fully
unsupported in continuations
return;
}
@@ -646,8 +646,13 @@ void VM_BaseGetOrSetLocal::doit() {
if (_set) {
if (fr.is_heap_frame()) { // we want this check after the check for
JVMTI_ERROR_INVALID_SLOT
assert(Continuation::is_frame_in_continuation(_jvf->thread(), fr),
"sanity check");
- // the safepoint could be as we return to the return barrier but before
we execute it (poll return)
- _result = JVMTI_ERROR_OPAQUE_FRAME; // deferred locals currently
unsupported in continuations
+ // If the topmost frame is a heap frame, then it hasn't been thawed.
This can happen
+ // if we are executing at a return barrier safepoint. The callee frame
has been popped,
+ // but the caller frame has not been thawed. We can't support a JVMTI
SetLocal in the callee
+ // frame at this point, because we aren't truly in the callee yet.
+ // fr.is_heap_frame() is impossible if a continuation is at a single
step or breakpoint.
+ // In such cases the frames can't be frozen because a native callback
frame is at the top.
+ _result = JVMTI_ERROR_OPAQUE_FRAME; // deferred locals are not fully
unsupported in continuations
return;
}
-------------
PR: https://git.openjdk.org/jdk19/pull/42
