Re: RFR: 8316456: StackWalker may skip Continuation::yield0 frame mistakenly [v3]
> `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. Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: minor clean up - Changes: - all: https://git.openjdk.org/jdk/pull/15804/files - new: https://git.openjdk.org/jdk/pull/15804/files/ddaeaf99..14f13223 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15804&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15804&range=01-02 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/15804.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15804/head:pull/15804 PR: https://git.openjdk.org/jdk/pull/15804
Re: RFR: 8316456: StackWalker may skip Continuation::yield0 frame mistakenly [v2]
On Thu, 21 Sep 2023 19:29:30 GMT, Patricio Chilano Mateo wrote: >> Mandy Chung 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 three additional >> commits since the last revision: >> >> - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8316456 >> - call setBatch to update origin and fence for an empty batch >> - 8316456: StackWalker may skip Continuation::yield0 frame mistakenly > > src/hotspot/share/prims/stackwalk.cpp line 189: > >> 187: // skip hidden frames for default StackWalker option (i.e. >> SHOW_HIDDEN_FRAMES >> 188: // not set) and when StackWalker::getCallerClass is called >> 189: LogTarget(Debug, stackwalk) lt; > > Nit, leftover. Thanks for the review. Will clean up before it's integrated - PR Review Comment: https://git.openjdk.org/jdk/pull/15804#discussion_r1333551895
Re: RFR: 8316456: StackWalker may skip Continuation::yield0 frame mistakenly [v2]
On Thu, 21 Sep 2023 18:20:36 GMT, Mandy Chung 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. > > Mandy Chung 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 three additional > commits since the last revision: > > - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8316456 > - call setBatch to update origin and fence for an empty batch > - 8316456: StackWalker may skip Continuation::yield0 frame mistakenly Looks good to me, thanks. Patricio src/hotspot/share/prims/stackwalk.cpp line 189: > 187: // skip hidden frames for default StackWalker option (i.e. > SHOW_HIDDEN_FRAMES > 188: // not set) and when StackWalker::getCallerClass is called > 189: LogTarget(Debug, stackwalk) lt; Nit, leftover. - Marked as reviewed by pchilanomate (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15804#pullrequestreview-1638561745 PR Review Comment: https://git.openjdk.org/jdk/pull/15804#discussion_r1333514103
Re: RFR: 8316456: StackWalker may skip Continuation::yield0 frame mistakenly [v2]
> `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. Mandy Chung 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 three additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8316456 - call setBatch to update origin and fence for an empty batch - 8316456: StackWalker may skip Continuation::yield0 frame mistakenly - Changes: - all: https://git.openjdk.org/jdk/pull/15804/files - new: https://git.openjdk.org/jdk/pull/15804/files/f3ec0dac..ddaeaf99 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15804&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15804&range=00-01 Stats: 19285 lines in 295 files changed: 11045 ins; 7282 del; 958 mod Patch: https://git.openjdk.org/jdk/pull/15804.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15804/head:pull/15804 PR: https://git.openjdk.org/jdk/pull/15804
Re: RFR: 8316456: StackWalker may skip Continuation::yield0 frame mistakenly
On Thu, 21 Sep 2023 05:15:26 GMT, Patricio Chilano Mateo 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
Re: RFR: 8316456: StackWalker may skip Continuation::yield0 frame mistakenly
On Mon, 18 Sep 2023 23:00:09 GMT, Mandy Chung 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? - PR Review Comment: https://git.openjdk.org/jdk/pull/15804#discussion_r1332471224
Re: RFR: 8316456: StackWalker may skip Continuation::yield0 frame mistakenly
On Mon, 18 Sep 2023 23:00:09 GMT, Mandy Chung 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. Marked as reviewed by rpressler (Committer). - PR Review: https://git.openjdk.org/jdk/pull/15804#pullrequestreview-1635330981
RFR: 8316456: StackWalker may skip Continuation::yield0 frame mistakenly
`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. - Commit messages: - 8316456: StackWalker may skip Continuation::yield0 frame mistakenly Changes: https://git.openjdk.org/jdk/pull/15804/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15804&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8316456 Stats: 128 lines in 7 files changed: 36 ins; 11 del; 81 mod Patch: https://git.openjdk.org/jdk/pull/15804.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15804/head:pull/15804 PR: https://git.openjdk.org/jdk/pull/15804
