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

[plummercj]

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.

---

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)

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]

👋 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]

@plummercj This change is no longer ready for integration - check the PR body for details.

[openjdk]

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

Webrevs

• 00: Full (51ac7ac1)

[plummercj]

@reinrich Can you review this PR? Thanks!

[lmesnik]

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.

[plummercj]

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?

[lmesnik]

The approve was set by mistake.