On Thu, 23 Jun 2022 07:09:38 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> The JVMTI SetLocalXXX requires the target virtual thread suspended at a >> breakpoint or single step event. >> >> This is the relevant statement from the "Local Variable" section: >> >> `The SetLocalXXX functions may be used to set the value of a local variable >> in the topmost frame of a virtual thread suspended at a breakpoint or single >> step event. >> ` >> This fix is to return the JVMTI_ERROR_OPAQUE_FRAME in cases when the target >> thread is not at a breakpoint or single step event. In this case the assert >> described in the bug report is avoided: >> >> >> open/src/hotspot/share/runtime/vframe.cpp:300 >> # assert(stack_chunk() == __null) failed: Not supported for heap frames >> >> Also, this is an analysis from Ron: >> >> The problem occurs because the thread is suspended at a safepoint poll on >> return in the oldest thawed frame. The safepoint happens after the frame is >> popped but before it returns to the return barrier, thawing the caller (and >> so the stack looks as if we're in enterSpecial). In that case the virtual >> thread's topmost frame is still frozen on the heap, even though it is >> mounted. >> >> However, the spec says that a set local operation should succeed for the >> topmost frame of a mounted virtual thread only if the thread is suspended >> *at a breakpoint or a single-step event*, and I don't think we can stop at >> that safepoint in that case. >> >> If so, the fix is simple: if we're trying to set, even if the virtual thread >> is mounted and the depth is zero, if the frame is a heap frame, we should >> return an OPAQUE_FRAME error. The test should be changed as well to accept >> such an error if the thread is suspended not at a breakpoint/single-step. >> >> >> Changes `GetSetLocalTest` test are taken from the repo-loom repository. It >> was an update from Leonid which is needed to reproduce this problem. >> >> The fix was tested with thousands of runs of the `GetSetLocalTest` on Linux, >> Windows and MacOs. >> >> Will also submit nsk.jvmti and nsk.jdi test runs on mach5. > > 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 four additional > commits since the last revision: > > - resolved 8286580 review comments; problem-listed test GetSetLocalTest > under 8286836 > - Merge branch 'master' into br2 > - remove _depth == 0 clause from sanity check assert > - 8286580: serviceability/jvmti/vthread/GetSetLocalTest failed with assert: > Not supported for heap frames src/hotspot/share/prims/jvmtiImpl.cpp line 654: > 652: // frame at this point, because we aren't truly in the callee yet. > 653: // fr.is_heap_frame() is impossible if a continuation is at a > single step or breakpoint. > 654: // In such cases the top frames can't be frozen because of an > agent callback frame. "frames" should be "frame". I don't understand the agent callback frame reference. ------------- PR: https://git.openjdk.org/jdk19/pull/42
