RFR: 8286580: serviceability/jvmti/vthread/GetSetLocalTest failed with assert: Not supported for heap frames [v2]

[Serguei Spitsyn]

On Thu, 23 Jun 2022 00:36:19 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> 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