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

[tkrodriguez]

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.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

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

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to 45f83f1

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2594/head:pull/2594
$ git checkout pull/2594

[bridgekeeper]

👋 Welcome back never! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@tkrodriguez The following label will be automatically applied to this pull request:

• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (1da3a26)
• 00: Full (45f83f1)

[vnkozlov]

Update copyright year in vframe.hpp.
Otherwise good.

[openjdk]

@tkrodriguez This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: kvn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 26 new commits pushed to the master branch:

• b8fd614: 8261939: os::strdup_check_oom() should be used in os::same_files() in os_windows.cpp
• 5f30829: 8202750: Reduce the use of get_canonical_path() in CDS
• ea5bf45: 8261621: Delegate Unicode history from JLS to j.l.Character
• d5a4d22: 8261843: incorrect info in docs/building.html
• bf75a3a: 8261851: update ReflectionCallerCacheTest.java test to use ForceGC from test library
• 05301f5: 8257497: Update keytool to create AKID from the SKID of the issuing certificate as specified by RFC 5280
• cb84539: 8261553: Efficient mask generation using BMI2 BZHI instruction
• a065879: 8261791: (sctp) handleSendFailed in SctpChannelImpl.c potential leaks
• 9ba2b71: 8261657: [PPC64] Cleanup StoreCM nodes after CMS removal
• f639df4: 8261401: Add sanity check for UseSHM large pages similar to the one used with hugetlb large pages
• ... and 16 more: https://git.openjdk.java.net/jdk/compare/8ba390d1e243c5ec2e637263fa44907b664e8ceb...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[dean-long]

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.

[tkrodriguez]

@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.

[dougxc]

Looks good to me.

[dean-long]

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

[tkrodriguez]

/integrate

[openjdk]

@tkrodriguez Since your change was applied there have been 26 commits pushed to the master branch:

• b8fd614: 8261939: os::strdup_check_oom() should be used in os::same_files() in os_windows.cpp
• 5f30829: 8202750: Reduce the use of get_canonical_path() in CDS
• ea5bf45: 8261621: Delegate Unicode history from JLS to j.l.Character
• d5a4d22: 8261843: incorrect info in docs/building.html
• bf75a3a: 8261851: update ReflectionCallerCacheTest.java test to use ForceGC from test library
• 05301f5: 8257497: Update keytool to create AKID from the SKID of the issuing certificate as specified by RFC 5280
• cb84539: 8261553: Efficient mask generation using BMI2 BZHI instruction
• a065879: 8261791: (sctp) handleSendFailed in SctpChannelImpl.c potential leaks
• 9ba2b71: 8261657: [PPC64] Cleanup StoreCM nodes after CMS removal
• f639df4: 8261401: Add sanity check for UseSHM large pages similar to the one used with hugetlb large pages
• ... and 16 more: https://git.openjdk.java.net/jdk/compare/8ba390d1e243c5ec2e637263fa44907b664e8ceb...master

Your commit was automatically rebased without conflicts.

Pushed as commit 97e1657.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.