RFR: 8316456: StackWalker may skip Continuation::yield0 frame mistakenly

[Mandy Chung]

On Thu, 21 Sep 2023 05:15:26 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> `JVM_MoreStackWalk` has a bug that always assumes that the Java frame
>> stream is currently at the frame decoded in the last patch and so always
>> advances to the next frame before filling in the new batch of stack frame.
>> However `JVM_MoreStackWalk` may return 0.  The library will set 
>> the continuation to its parent. It then call `JVM_MoreStackWalk` to continue
>> the stack walking but the last decoded frame has already been advanced.
>> The Java frame stream is already at the top frame of the parent continuation. .
>> The current implementation skips "Continuation::yield0" mistakenly.  This
>> only happens with `-XX:+ShowHiddenFrames` or `StackWalker.Option.SHOW_HIDDEN_FRAMES`.
>> 
>> The fix is to pass the number of frames decoded in the last batch to `JVM_MoreStackWalk`
>> so that the VM will determine if the current frame should be skipped or not.
>> 
>> `test/jdk/jdk/internal/vm/Continuation/Scoped.java` test now correctly checks
>> the expected result where "yield0" exists between "enter" and "run" frames.
>
> src/java.base/share/classes/java/lang/StackStreamFactory.java line 443:
> 
>> 441: 
>> 442:             // If the last batch didn't fetch any frames, keep the current batch size.
>> 443:             int lastBatchFrameCount = frameBuffer.numFrames();
> 
> I run some tests to understand the issue and I got the same failure if I now set MIN_BATCH_SIZE to 7. This just forces the same situation where Continuation::enter is the last frame in the buffer, otherwise since the patch also changes the batch sizes we don't fall into that issue anymore. The problem is with this numFrames() method which still returns a number > 0 after the fetch attempt that returns with no frames. Maybe there is a reset missing for origin and fence when fetching the next batch?

Thanks for catching this.   The problem is that `fetchStackFrames(int batchSize)` is supposed to call `setBatch` to set origin and fence for the new batch.  But if no frame is fetched, it skips not calling that.   I have a fix for it.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/15804#discussion_r1333433443