8342042: PPC64: compiler_fast_unlock_object flags failure instead of success

[reinrich]

This change inverts the EQ bit in flag with a condition register nand instruction in order to meet the post condition given at L2751-L2752:

  // flag == EQ indicates success, decrement held monitor count
  // flag == NE indicates failure

The fix passed our CI testing with LockingMode set to LM_LEGACY
Tier1-4 of hotspot and jdk. All of Langtools and jaxp. Renaissance Suite and SAP specific tests.
Testing was done on the main platforms and also on Linux/PPC64le and AIX.

The test runtime/logging/MonitorInflationTest.java failed on all platforms. Apparently it has issues with LM_LEGACY.

---

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-8342042: PPC64: compiler_fast_unlock_object flags failure instead of success (Bug - P4)

Reviewers

• Martin Doerr (@TheRealMDoerr - Reviewer)
• Axel Boldt-Christmas (@xmas92 - Reviewer)
• Fredrik Bredberg (@fbredber - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21496

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8342042: PPC64: compiler_fast_unlock_object flags failure instead of success

Reviewed-by: mdoerr, aboldtch, fbredberg

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

• 44151f4: 8342145: File libCreationTimeHelper.c compile fails on Alpine
• 709914f: 8338023: Support two vector selectFrom API
• c34fb2c: 8338883: Show warning when CreateCoredumpOnCrash set, but core dump will not happen
• 7625b29: 8329597: C2: Intrinsify Reference.clear
• 1cc3223: 8336911: ZGC: Division by zero in heuristics after JDK-8332717
• cf5bb12: Merge
• 490d099: 8335713: Enhance vectorization analysis
• c89f76c: 8332644: Improve graph optimizations
• d9f6383: 8331446: Improve deserialization support
• dadcee1: 8307383: Enhance DTLS connections
• ... and 33 more: https://git.openjdk.org/jdk/compare/ba3774dc5d27e762dfd61f8acf842ae11dec0cb7...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]

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

[reinrich]

/label add hotspot-compiler
/label remove hotspot

[openjdk]

@reinrich
The hotspot-compiler label was successfully added.

[openjdk]

@reinrich
The hotspot label was successfully removed.

[TheRealMDoerr]

Good catch! LGTM.

[reinrich]

Thanks for the quick review!

[mlbridge]

Webrevs

• 01: Full - Incremental (b24743a9)
• 00: Full (1963e884)

[xmas92]

Seems like compiler_fast_unlock_lightweight_object could do the same.

It does the same branch, but correctly, by going through a crorc(CCR0, Assembler::equal, CCR0, Assembler::equal) in the success path.

[reinrich]

Seems like compiler_fast_unlock_lightweight_object could do the same.

It does the same branch, but correctly, by going through a crorc(CCR0, Assembler::equal, CCR0, Assembler::equal) in the success path.

I think you're right. I wanted to see if set_eq_unlocked could be eliminated and then forgot about it.

[TheRealMDoerr]

LGTM. Thanks for improving it!

[fbredber]

@reinrich
Thank you for finding and fixing this. Also sorry for not finding this when I tested my PR.

[reinrich]

@reinrich Thank you for finding and fixing this. Also sorry for not finding this when I tested my PR.

Thanks for looking at this @fbredber. Even with the assertions from #21494 this didn't cause failures testing jdk 24. So no worries about the testing of your PR.
I found the issue working on the PPC port of the vthread monitor support in the loom repo. There an assertion in the runtime failed when the slow path was taken. I don't know why though.

[reinrich]

Thanks again for the reviews.
/integrate

[openjdk]

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

• e0dabfb: 8340279: Open source several AWT Dialog tests - Batch 2
• f6fe5ea: 8341072: Open source several AWT Canvas and Rectangle related tests
• 22a1fee: 8341328: Refactor initial Assertion Predicate creation into separate classes
• 58d39c3: 8340313: Crash due to invalid oop in nmethod after C1 patching
• 3da6890: 8338751: ConfigureNotify behavior has changed in KWin 6.2
• 4e703b2: 8340140: Open some dialog awt tests 3
• b4ab290: 8331959: Update PKCS#11 Cryptographic Token Interface to v3.1
• 2853852: 8342332: [JVMCI] Export CompilerToVM::Data::dtanh
• ed68096: 8341862: PPC64: C1 unwind_handler fails to unlock synchronized methods with LM_MONITOR
• d4f0ba7: 8342439: Build failure after 8338023
• ... and 43 more: https://git.openjdk.org/jdk/compare/ba3774dc5d27e762dfd61f8acf842ae11dec0cb7...master

Your commit was automatically rebased without conflicts.

[openjdk]

@reinrich Pushed as commit fa39e84.

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