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

[Chris Plummer]

On Tue, 21 Jun 2022 18:28:16 GMT, Serguei Spitsyn <sspitsyn at 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