RFR: 8261846: [JVMCI] c2v_iterateFrames can get out of sync with the StackFrameStream

[Tom Rodriguez-3]

c2v_iterateFrames mixes a StackFrameSteam and vframes and the vframe factory method can silently skip stub frames. The could leave the StackFrameStream out of sync with the vframe walk. This can cause the iteration fail in strange ways and assert in fastdebug builds.

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

Commit messages:
 - Keep StackFrameStream in sync with vframes

Changes: https://git.openjdk.java.net/jdk/pull/2594/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2594&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261846
  Stats: 13 lines in 3 files changed: 9 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2594.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2594/head:pull/2594

PR: https://git.openjdk.java.net/jdk/pull/2594

[Vladimir Kozlov-2]

On Tue, 16 Feb 2021 20:17:30 GMT, Tom Rodriguez <[hidden email]> wrote:

> c2v_iterateFrames mixes a StackFrameSteam and vframes and the vframe factory method can silently skip stub frames. The could leave the StackFrameStream out of sync with the vframe walk. This can cause the iteration fail in strange ways and assert in fastdebug builds.

Update copyright year in vframe.hpp.
Otherwise good.

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

Marked as reviewed by kvn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2594

[Dean Long-2]

On Wed, 17 Feb 2021 00:46:12 GMT, Vladimir Kozlov <[hidden email]> wrote:

>> c2v_iterateFrames mixes a StackFrameSteam and vframes and the vframe factory method can silently skip stub frames. The could leave the StackFrameStream out of sync with the vframe walk. This can cause the iteration fail in strange ways and assert in fastdebug builds.
>
> Update copyright year in vframe.hpp.
> Otherwise good.

Hi Tom.  This code could be simplified and made faster using vframeStream as the iterator and vframeStream:asJavaVFrame to get the vframe.  If you don't need the locals of every frame, then vframeStream::next is faster than vframe::sender.  See JDK-8214329.

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

PR: https://git.openjdk.java.net/jdk/pull/2594

[Tom Rodriguez-3]

On Wed, 17 Feb 2021 02:33:02 GMT, Dean Long <[hidden email]> wrote:

>> Update copyright year in vframe.hpp.
>> Otherwise good.
>
> Hi Tom.  This code could be simplified and made faster using vframeStream as the iterator and vframeStream:asJavaVFrame to get the vframe.  If you don't need the locals of every frame, then vframeStream::next is faster than vframe::sender.  See JDK-8214329.

@dean-long I think it would be possible to rewrite this to use vframeStream though it would still need to build vframes because of the introspection it does on compiled frames.  It would be a major rewrite though and in general this code is working just fine so I don't think I want to tackle that.

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

PR: https://git.openjdk.java.net/jdk/pull/2594

[Doug Simon]

On Wed, 17 Feb 2021 19:34:53 GMT, Tom Rodriguez <[hidden email]> wrote:

>> Hi Tom.  This code could be simplified and made faster using vframeStream as the iterator and vframeStream:asJavaVFrame to get the vframe.  If you don't need the locals of every frame, then vframeStream::next is faster than vframe::sender.  See JDK-8214329.
>
> @dean-long I think it would be possible to rewrite this to use vframeStream though it would still need to build vframes because of the introspection it does on compiled frames.  It would be a major rewrite though and in general this code is working just fine so I don't think I want to tackle that.

Looks good to me.

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

PR: https://git.openjdk.java.net/jdk/pull/2594

[Dean Long-2]

On Wed, 17 Feb 2021 19:34:53 GMT, Tom Rodriguez <[hidden email]> wrote:

>> Hi Tom.  This code could be simplified and made faster using vframeStream as the iterator and vframeStream:asJavaVFrame to get the vframe.  If you don't need the locals of every frame, then vframeStream::next is faster than vframe::sender.  See JDK-8214329.
>
> @dean-long I think it would be possible to rewrite this to use vframeStream though it would still need to build vframes because of the introspection it does on compiled frames.  It would be a major rewrite though and in general this code is working just fine so I don't think I want to tackle that.

@tkrodriguez OK, I filed a separate RFE for my suggestion.

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

PR: https://git.openjdk.java.net/jdk/pull/2594

[Tom Rodriguez-3]

> c2v_iterateFrames mixes a StackFrameSteam and vframes and the vframe factory method can silently skip stub frames. The could leave the StackFrameStream out of sync with the vframe walk. This can cause the iteration fail in strange ways and assert in fastdebug builds.

Tom Rodriguez has updated the pull request incrementally with one additional commit since the last revision:

  Update copyright year

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2594/files
  - new: https://git.openjdk.java.net/jdk/pull/2594/files/45f83f12..1da3a26f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2594&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2594&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2594.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2594/head:pull/2594

PR: https://git.openjdk.java.net/jdk/pull/2594

[Tom Rodriguez-3]

On Tue, 16 Feb 2021 20:17:30 GMT, Tom Rodriguez <[hidden email]> wrote:

> c2v_iterateFrames mixes a StackFrameSteam and vframes and the vframe factory method can silently skip stub frames. The could leave the StackFrameStream out of sync with the vframe walk. This can cause the iteration fail in strange ways and assert in fastdebug builds.

This pull request has now been integrated.

Changeset: 97e1657b
Author:    Tom Rodriguez <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/97e1657b
Stats:     14 lines in 3 files changed: 9 ins; 0 del; 5 mod

8261846: [JVMCI] c2v_iterateFrames can get out of sync with the StackFrameStream

Reviewed-by: kvn

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

PR: https://git.openjdk.java.net/jdk/pull/2594