On Tue, 21 Jun 2022 18:28:16 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 incrementally with one
> additional commit since the last revision:
>
> remove _depth == 0 clause from sanity check assert
src/hotspot/share/prims/jvmtiImpl.cpp line 647:
> 645: }
> 646: if (_set) {
> 647: if (fr.is_heap_frame()) { // we want this check after the check for
> JVMTI_ERROR_INVALID_SLOT
It would be good to elaborate a bit more on this check. Say something about it
being a "safepoint poll on return in the oldest thawed frame", and why that is
a problem.
test/hotspot/jtreg/serviceability/jvmti/vthread/GetSetLocalTest/GetSetLocalTest.java
line 103:
> 101: GetSetLocalTest obj = new GetSetLocalTest();
> 102:
> 103: for (int i = 0; i < 1000; i++) {
Does this test still run quickly?
-------------
PR: https://git.openjdk.org/jdk19/pull/42
