8335977: Deoptimization fails with assert "object should be reallocated already"

[JohnTortugo]

Please, review this patch to fix an issue that may occur when serializing debug information related to reduce allocation merges. The problem happens when there are more than one JVMS in a uncommon_trap and a younger JVMS doesn't have the RAM inputs as a local/expression/monitor but an older JVMS does. In that situation the loop at line 1173 of output.cpp will set the is_root property of the ObjectValue to false when processing the younger JVMS even though it may have been set to true when visiting the older JVMS.

Tested on:

• Win, Mac & Linux tier1-4 on x64 & Aarch64.
• CTW with some thousands of jars.

---

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-8335977: Deoptimization fails with assert "object should be reallocated already" (Bug - P2)

Reviewers

• Tobias Hartmann (@TobiHartmann - Reviewer)
• Vladimir Ivanov (@iwanowww - Reviewer) Review applies to 8e345b59
• Vladimir Kozlov (@vnkozlov - Reviewer)

Contributors

• Christian Hagedorn <chagedorn@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21624

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8335977: Deoptimization fails with assert "object should be reallocated already"

Co-authored-by: Christian Hagedorn <chagedorn@openjdk.org>
Reviewed-by: thartmann, kvn, vlivanov

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

• b269493: 8338981: Access to private classes should be permitted inside the permits clause of the enclosing top-level class
• 3ccd2f7: 8342458: More consistent constant instruction handling
• 29ae265: 8341861: GHA: Use only retention mechanism to remove bundles
• e5bbad0: 8334999: RISC-V: implement AES single block encryption/decryption intrinsics
• c40bb76: 8343296: IGV: Show pre/main/post at CountedLoopNodes
• dee0982: 8343132: Remove temporary transitions from Virtual thread implementation
• 2f1ba5e: 8343307: Throw ZipException instead of IAE in ZipFile.Source::initCEN
• 388d44f: 8342156: C2: Compilation failure with fewer arguments after JDK-8329032
• 688e92e: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper
• f340ab2: 8342181: Update tests to use stronger Key and Salt size
• ... and 232 more: https://git.openjdk.org/jdk/compare/bd6264420b9f248999dd8387c25c549b08bd193a...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TobiHartmann, @iwanowww, @vnkozlov) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

@JohnTortugo 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 (818b09e6)
• 01: Full - Incremental (26b0b869)
• 00: Full (8e345b59)

[TobiHartmann]

The fix looks good to me.

Now that the -XX:+StressUnstableIfTraps option was added with JDK-8335334, could you please also add Test.java that I used originally to reproduce the issue?

Does your test also reproduce the NPE example that @chhagedorn triggered with his Test3.java? If not, I think that one should be added as well.

The more tests we have for this, the better :)

[TobiHartmann]

I executed some extended testing. All green.

[JohnTortugo]

Does your test also reproduce the NPE example that @chhagedorn triggered with his Test3.java? If not, I think that one should be added as well.

The test I added in this PR is based on @chhagedorn Test3.java. I was able to reproduce the issue on my end fairly easily.

Now that the -XX:+StressUnstableIfTraps option was added with JDK-8335334, could you please also add Test.java that I used originally to reproduce the issue?

I wasn't able to reproduce the issue using Test.java, how often does it reproduce for you with the flags that you listed at the top of Test.java ?

Thanks for testing @TobiHartmann !

[iwanowww]

Looks good.

[TobiHartmann]

The test I added in this PR is based on @chhagedorn Test3.java. I was able to reproduce the issue on my end fairly easily.

Ah right, all good then. Please add @chhagedorn as co-contributor since he extracted that test.

I wasn't able to reproduce the issue using Test.java, how often does it reproduce for you with the flags that you listed at the top of Test.java ?

Right, I just tried and it does not seem to reproduce anymore. Too bad but let's leave it out then.

[JohnTortugo]

/contributor add @chhagedorn

[openjdk]

@JohnTortugo
Contributor Christian Hagedorn <chagedorn@openjdk.org> successfully added.

[TobiHartmann]

Looks good to me too.

[JohnTortugo]

/integrate

[openjdk]

@JohnTortugo This pull request has not yet been marked as ready for integration.

[JohnTortugo]

/integrate

[openjdk]

@JohnTortugo
Your change (at version 818b09e) is now ready to be sponsored by a Committer.

[vnkozlov]

/sponsor

[openjdk]

Going to push as commit 7d8bd21.
Since your change was applied there have been 242 commits pushed to the master branch:

• b269493: 8338981: Access to private classes should be permitted inside the permits clause of the enclosing top-level class
• 3ccd2f7: 8342458: More consistent constant instruction handling
• 29ae265: 8341861: GHA: Use only retention mechanism to remove bundles
• e5bbad0: 8334999: RISC-V: implement AES single block encryption/decryption intrinsics
• c40bb76: 8343296: IGV: Show pre/main/post at CountedLoopNodes
• dee0982: 8343132: Remove temporary transitions from Virtual thread implementation
• 2f1ba5e: 8343307: Throw ZipException instead of IAE in ZipFile.Source::initCEN
• 388d44f: 8342156: C2: Compilation failure with fewer arguments after JDK-8329032
• 688e92e: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper
• f340ab2: 8342181: Update tests to use stronger Key and Salt size
• ... and 232 more: https://git.openjdk.org/jdk/compare/bd6264420b9f248999dd8387c25c549b08bd193a...master

Your commit was automatically rebased without conflicts.

[openjdk]

@vnkozlov @JohnTortugo Pushed as commit 7d8bd21.

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