8381609: com/sun/jdi/EATests.java should not synchronize on an Integer instance by plummercj · Pull Request #30560 · openjdk/jdk

plummercj · 2026-04-03T01:55:56Z

com/sun/jdi/EATests.java synchronizes on an Integer instance. Although this currently works, it is discouraged. See the following doc on value based classes:

https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/lang/doc-files/ValueBased.html

"Synchronization on instances of value-based classes is strongly discouraged, because the programmer cannot guarantee exclusive ownership of the associated monitor.

Identity-related behavior of value-based classes may change in a future release. For example, synchronization may fail."

That second part is realized by Valhalla. The synchronization fails with:

java.lang.IdentityException: Cannot synchronize on an instance of value class java.lang.Integer

Valhalla CR JDK-8372831 covers that failure, but I thought it best to address this in mainline first.

Tested with all of CI tier1, tier2 svc, and tier5 svc.

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

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-8381609: com/sun/jdi/EATests.java should not synchronize on an Integer instance (Bug - P3)

Reviewers

Leonid Mesnik (@lmesnik - Reviewer)
Serguei Spitsyn (@sspitsyn - Reviewer)
Paul Hübner (@Arraying - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30560

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-04-03T01:57:30Z

👋 Welcome back cjplummer! 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 · 2026-04-03T01:57:55Z

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

8381609: com/sun/jdi/EATests.java should not synchronize on an Integer instance

Reviewed-by: lmesnik, sspitsyn, phubner

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

aa5677a: 8374496: C2: assert(val->find_edge(con) > 0) failed
2b716d7: 8381658: Update nsk/jvmti/scenarios/sampling to better test virtual threads
f7c0695: 8381998: Move UseCompressedClassPointers to obsolete flags section
... and 109 more: https://git.openjdk.org/jdk/compare/f46a6981137bd09e9387c168eb24e4bc2edf32c4...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 · 2026-04-03T01:58:47Z

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

serviceability

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 · 2026-04-03T02:03:01Z

Webrevs

01: Full - Incremental (9f7d4dc6)
00: Full (51ac7ac1)

plummercj · 2026-04-03T02:03:28Z

@reinrich Can you review this PR? Thanks!

lmesnik · 2026-04-03T02:13:03Z

test/jdk/com/sun/jdi/EATests.java

 * Test relocking eliminated @ValueBased object.
 */
-class EARelockingValueBased extends EATestCaseBaseDebugger {
+class EARelockingObject extends EATestCaseBaseDebugger {


The goal of testcase was to specifically lock on the ValueBased object.
I think this testacase now doing the same as EARelockingSimple and the best fix would be just to delete this testcase.

You are right. I thought there might be some reason to keep it in, but I see now that it basically duplicates EARelockingSimple.

It looks like we are losing some useful testing by not syncing on Integer. In #7626, which added the test, the description says:

"Can reproduce a crash by modifying test/jdk/com/sun/jdi/EATests.java and using -XX:DiagnoseSyncOnValueBasedClasses=2 with LM_LEGACY or running test/jdk/com/sun/jdi/EATests.java with LM_LIGHTWEIGHT/LM_MONITOR and -Xlog:monitorinflation=trace."

So we would be losing this testing, which seems like it might be important. Eventually it will have to go away when Valhalla moves out of preview. We could keep it for now and only run it when PreviewFeatures.isEnabled() returns false, but eventually Valhalla will be out of preview and this subtest will need to be removed.

@xmas92 Do you think it is useful to keep EARelockingValueBased around until Valhalla is out of preview, or should we just get rid of it now?

One other thing to point out is that running with DiagnoseSyncOnValueBasedClasses=2 was removed by JDK-8359437 (@toxaart), but then for some reason Alex added it back in for JDK-8377845. I'm not sure why he added it back in rather than just remove the comment that was missed by JDK-8359437.

If the test is no longer using a @ValueBased class then there is no need for this regression test, as it no longer exercises the relevant runtime code paths. However this means that we also no longer test that EA with DiagnoseSyncOnValueBasedClasses on non-preview runs.

From what I can see, there is no plan to remove DiagnoseSyncOnValueBasedClasses with JEP 401. So I wonder if we could not just have this specific ValueBased test case be conditional enabled based on enable-preview.

Removing the @ValueBased testing test case regardless of DiagnoseSyncOnValueBasedClasses means we have less coverage of any regressions that might be introduced after JEP 401 in non-preview mode w.r.t. EA and @ValueBased.

But if the consensus is that we want to remove the preview-mode illegal code from (non-preview mode) tests, then I see no reason to keep this test case either.

Isn't this just a test that should not be run in preview mode? By the time value types are out of preview DiagnoseSyncOnValueBasedClasses should be gone too, at which time the test can be removed.

I created #2313

The link above is for the jdk repo. This should be the correct link openjdk/valhalla#2313

I also updated this PR to remove the EARelockingValueBased test.

I'm fine with that. Merging into the valhalla repository will require a manual resolution, right?

Ok. I fixed the PR link.

There are periodic merges into valhalla. I don't anything special is needed to merge this PR.

But this pr and openjdk/valhalla#2313 both modify L262 and L388. I assume this will cause merge conflicts. That's actually good since the intention is not to remove the EARelockingValueBased in valhalla.

I see your point now. I'll only be integrating one of the PRs. We're just trying to figure out which one. And the intention of this PR is to remove EARelockingValueBased in valhalla also. If anything, we want to keep EARelockingValueBased here but not in valhalla. But once integration of valhalla into maiinline happens, it would be gone in mainline too. Bascially there are 4 options here:

This PR. Remove the subtest now, and do it in mainline. We'll never need to revisit this issue, but we lose the EARelockingValueBased testing now rather than later.

The valhalla PR. Avoid running the subtest when in preview mode. When value types are no longer in preview we will need to remove the preview mode checks and the subtest. This also requires using main instead of driver mode.

Remove the subtest from the valhalla repo. It would then be gone from mainline when valhalla is integrated, so that keeps the test around a bit longer.

Use @requires !java.enablePreview in the valhalla repo. The subtest can stay around until value types are no longer preview. EATests would get no testing in preview mode.

lmesnik

The approve was set by mistake.

reinrich · 2026-04-08T15:55:27Z

@reinrich Can you review this PR? Thanks!

Thanks for the notifying. I agree that EARelockingValueBased should only be run if JEP 401 preview features are disabled. Not sure though what the best way would be to implement this.

lmesnik

I think it is the best solution.

reinrich

I'm also ok with removing EARelockingValueBased already with this pr. I think running with DiagnoseSyncOnValueBasedClasses=2 can be removed then too.

sspitsyn

Looks good to me. This fix is needed in preparation for Valhalla preview release.

Arraying

From the perspective of a Valhalla VM developer, regarding #30560 (comment), I think (1) is the way to go, and we already have the code ready! (2) is also a good solution I'd be in favour of. I don't think it is wise to lose preview coverage as suggested by (4), and (3) would inflate the mainline diff when JEP 401 is under review (some might say at almost 200k additions this does not matter, I think if we can avoid more diffs we should).

reinrich · 2026-04-10T08:38:37Z

test/jdk/com/sun/jdi/EATests.java

 * @bug 8324881
 * @comment Regression test for using the wrong thread when logging during re-locking from deoptimization.
 *
 * @comment DiagnoseSyncOnValueBasedClasses=2 will cause logging when locking on \@ValueBased objects.
 * @run driver EATests
 *                 -XX:+UnlockDiagnosticVMOptions
 *                 -Xms256m -Xmx256m
 *                 -Xbootclasspath/a:.
 *                 -XX:CompileCommand=dontinline,*::dontinline_*
 *                 -XX:+WhiteBoxAPI
 *                 -Xbatch
 *                 -XX:+DoEscapeAnalysis -XX:+EliminateAllocations -XX:+EliminateLocks -XX:+EliminateNestedLocks
 *                 -XX:DiagnoseSyncOnValueBasedClasses=2
 *


This @run should be removed too.

Ok. The @bug 8324881 and the comment below it are confusing and I'm not sure if they both should be removed also. They got added as part of JDK-8324881, along with the 3 @run that follow. @xmas92 ?

plummercj · 2026-04-10T18:42:59Z

/template append

openjdk · 2026-04-10T18:44:57Z

@plummercj The pull request template has been appended to the pull request body

plummercj · 2026-04-10T18:59:36Z

/template append

openjdk · 2026-04-10T19:01:56Z

@plummercj The pull request template has been appended to the pull request body

don't sync on an Integer

51ac7ac

openjdk bot changed the title ~~8381609~~ 8381609: com/sun/jdi/EATests.java should not synchronize on an Integer instance Apr 3, 2026

openjdk bot added the serviceability serviceability-dev@openjdk.org label Apr 3, 2026

openjdk bot added the rfr Pull request is ready for review label Apr 3, 2026

lmesnik approved these changes Apr 3, 2026

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 3, 2026

lmesnik suggested changes Apr 3, 2026

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Apr 3, 2026

remove locking on Integer

9f7d4dc

plummercj mentioned this pull request Apr 9, 2026

8372831: [lworld] com/sun/jdi/EATests.java#id0 fails with --enable-preview openjdk/valhalla#2313

Open

3 tasks

lmesnik approved these changes Apr 9, 2026

View reviewed changes

openjdk bot removed the rfr Pull request is ready for review label Apr 9, 2026

reinrich suggested changes Apr 10, 2026

View reviewed changes

sspitsyn approved these changes Apr 10, 2026

View reviewed changes

Arraying approved these changes Apr 10, 2026

View reviewed changes

reinrich suggested changes Apr 10, 2026

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 10, 2026

Conversation

plummercj commented Apr 3, 2026 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Apr 3, 2026

Uh oh!

openjdk bot commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

plummercj commented Apr 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmesnik left a comment

Choose a reason for hiding this comment

Uh oh!

reinrich commented Apr 8, 2026

Uh oh!

lmesnik left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reinrich left a comment

Choose a reason for hiding this comment

Uh oh!

sspitsyn left a comment

Choose a reason for hiding this comment

Uh oh!

Arraying left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plummercj commented Apr 10, 2026

Uh oh!

openjdk bot commented Apr 10, 2026

Uh oh!

plummercj commented Apr 10, 2026

Uh oh!

openjdk bot commented Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

plummercj commented Apr 3, 2026 •

edited by openjdk bot

Loading

openjdk bot commented Apr 3, 2026 •

edited

Loading

openjdk bot commented Apr 3, 2026 •

edited

Loading

mlbridge bot commented Apr 3, 2026 •

edited

Loading

lmesnik left a comment •

edited

Loading