8366490: C2 SuperWord: wrong result because CastP2X is missing ctrl and floats over SafePoint creating stale oops

[eme64]

Analysis

A CastP2X without ctrl can float. If it floats over a SafePoint (or call), we may GC and move the oop. But the CastP2X value does not end up on the oop-map, and so the pointer is stale (old).

With StressGCM, the aliasing runtime check has one CastP2X that floats over the SafePoint, and another that stays after the SafePoint. Both read the oop of the same array, so instead of getting the same address, we now get the old and the new oop. And so the aliasing runtime check passes (thinks there is no aliasing), even though there is aliasing. We end up vectorizing, which reorders the loads/stores and would only be safe if there is no aliasing.

Fix: add control to the CastP2X so that it cannot float too far.

Details

rbp = Allcoate array
spill <- rbp + 0x20

call to allocateArrays
-> allocates a lot, and triggers GC. That moves the allocated array behind rbp
-> rbp is oop-mapped, so it is updated automatically to the new oop
-> spill value remains based on the old oop

We now compute the aliasing runtime check:
-> one side of the comparison is computed from rbp (new oop)
-> the other side is computed from the the spill value (old oop)
-> the cmp returns a nonsensical value, and we take the wrong branch
-> vectorize even though we have aliasing!

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8366490: C2 SuperWord: wrong result because CastP2X is missing ctrl and floats over SafePoint creating stale oops (Bug - P3)

Reviewers

• Tobias Hartmann (@TobiHartmann - Reviewer)
• Manuel Hässig (@mhaessig - Committer) Review applies to 13f70d31
• Christian Hagedorn (@chhagedorn - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27045/head:pull/27045
$ git checkout pull/27045

Update a local copy of the PR:
$ git checkout pull/27045
$ git pull https://git.openjdk.org/jdk.git pull/27045/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27045

View PR using the GUI difftool:
$ git pr show -t 27045

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27045.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back epeter! 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]

@eme64 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:

8366490: C2 SuperWord: wrong result because CastP2X is missing ctrl and floats over SafePoint creating stale oops

Reviewed-by: thartmann, chagedorn, mhaessig

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 20 new commits pushed to the master branch:

• a029245: 8365983: Tests should throw SkippedException when SCTP not supported
• 7103543: 8366063: Parallel: Refactor copy_unmarked_to_survivor_space
• 1feb9bd: 8365557: Parallel: Refactor ParallelScavengeHeap::mem_allocate_work
• ... and 17 more: https://git.openjdk.org/jdk/compare/fc77e7600f217cc91c24d4e512c685e176a66e4a...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.

[openjdk]

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

• hotspot-compiler

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

• 02: Full - Incremental (d1a35d12)
• 01: Full - Incremental (13f70d31)
• 00: Full (91652115)

[TobiHartmann]

Nice analysis! Looks good to me.

[mhaessig]

Thank you for the fix and the easy to follow analysis, @eme64. I just have a few minor comments. Otherwise, this looks good.

[mhaessig]

No clue if this is the right use case, but maybe this would be a good use of -XX:+GCALot?

[eme64]

Maybe, you could be right!

[chhagedorn]

Otherwise, looks good to me, too!