8368274: [lworld] Unsafe.compareAndExchangeFlatValue() wrongly assumes padding bytes are the same for identical value objects #1734

liach · 2025-11-12T22:02:07Z

Use the raw value get as witness, because the flat value get may ignore garbage value and cause infinite loop as a result. Waiting for a test case.

Progress

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

Issue

JDK-8368274: [lworld] Unsafe.compareAndExchangeFlatValue() wrongly assumes padding bytes are the same for identical value objects (Bug - P4)

Reviewers

Maurizio Cimadamore (@mcimadamore - Committer) ⚠️ Review applies to c1659649

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1734

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1734.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-11-12T22:04:04Z

👋 Welcome back liach! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-11-12T22:05:50Z

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

8368274: [lworld] Unsafe.compareAndExchangeFlatValue() wrongly assumes padding bytes are the same for identical value objects

Reviewed-by: mcimadamore

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 4 new commits pushed to the lworld branch:

8f8398e: 8374729: [lworld] Enabling CDS crash with UseAltSubstitutabilityMethod
fa557ef: 8375740: [lworld] javap prints "value class" while class flags indicate ACC_IDENTITY
a446415: 8375328: [lworld] JNI IsSameObject function doesn't use the alternate substitutability method
... and 1 more: https://git.openjdk.org/valhalla/compare/a4fb7ebd5af316d3e99d10bffb44d5be3aab5548...lworld

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 lworld branch, type /integrate in a new comment.

…/unsafe-flat-cas

mlbridge · 2025-12-08T17:02:14Z

Webrevs

TobiHartmann · 2025-12-12T06:24:58Z

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCompareAndExchange.java

+    // compareAndExchange to flattened field in object, non-inline arguments to compare and set
+    @Test
+    public Object test70(Object expected, Object x) {
+        return U.compareAndExchangeFlatValue(this, TEST63_VT_OFFSET, 4, SmallValue.class, expected, x);


Aren't existing tests using compareAndExchangeFlatValue in test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestIntrinsics.java sufficient? If not, I'd suggest adding your test case there.

bridgekeeper · 2026-01-09T11:30:50Z

@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

merykitty · 2026-01-15T13:32:27Z

src/java.base/share/classes/jdk/internal/misc/Unsafe.java

-            }
-        }
+        Object[] array = newSpecialArray(valueType, 2, layout);
+        return compareAndSetFlatValueAsBytes(array, o, offset, layout, valueType, expected, x) ? x : array[0];


This method should return array[0] in all cases.

TobiHartmann · 2026-01-15T14:04:46Z

@liach Please un-problem list VarHandleTestMethodHandleAccessValue.java now that JDK-8367346 was closed as duplicate. Thanks.

…/unsafe-flat-cas

liach · 2026-01-17T00:30:58Z

Should I keep TestCompareAndExchange any more? I don't know what's the best way to create a C2 test for this.

TobiHartmann · 2026-01-19T09:11:43Z

Thanks. I just discussed with @chhagedorn offline and we think that it's sufficient to have VarHandleTestMethodHandleAccessValue.java as regression test that you now un-problem listed. You can remove TestCompareAndExchange. Please verify though that VarHandleTestMethodHandleAccessValue.java now passes.

…/unsafe-flat-cas

liach · 2026-01-19T19:22:08Z

Ran the VarHandleTestMethodHandleAccessValue test again with no failure.

mcimadamore · 2026-01-21T15:55:16Z

src/java.base/share/classes/jdk/internal/misc/Unsafe.java

-        putFlatValue(expectedArray, base, layout, valueType, expected);
-        putFlatValue(xArray, base, layout, valueType, x);
+
+        // array 0: witness (to translate to object), 1: x (to translate to raw)


This comment is hard to read, for a number of reasons:

it uses 0/1, but then the code uses base/base + scale (understandably so)

translate to object/translate to raw -- yes, x starts off as a value object and we use the array to derive its "raw" representation, whereas witness is always a "raw" value, and stored into the array so we can get it back "as a value". But since this is finicky logic, all this should be spelled out more.

the caller can "capture the witness" -- maybe "observe" the (last) witness would be more correct?

"we must witness the raw value instead of the value object" -- well, even the old code was witnessing a raw value (of sort) -- the issue is how you compute such a raw value. The fix in this PR is that the raw value is accessed directly from memory (e.g. the content of o at offset offset). E.g. it feels like you are "overloading" what "raw" means -- in some cases you mean "raw == numeric, or !value", in other cases you mean "raw as in -- as accessed from memory"

mcimadamore

The code changes look good (a test would be good tho). I left some comments to try and clarify the docs.

liach · 2026-01-21T22:15:35Z

I have updated the comment. Please help proofread again.

Attempt to fix garbage cas

56cd588

openjdk bot changed the title ~~8368274~~ 8368274: [lworld] Unsafe.compareAndExchangeFlatValue() wrongly assumes padding bytes are the same for identical value objects Nov 12, 2025

liach added 3 commits November 17, 2025 14:11

Merge branch 'lworld' of https://github.com/openjdk/valhalla into fix…

fcdb32e

…/unsafe-flat-cas

Merge branch 'lworld' of https://github.com/openjdk/valhalla into fix…

47a1034

…/unsafe-flat-cas

A test

88965da

liach marked this pull request as ready for review December 8, 2025 16:58

openjdk bot added the rfr Pull request is ready for review label Dec 8, 2025

TobiHartmann reviewed Dec 12, 2025

View reviewed changes

merykitty reviewed Jan 15, 2026

View reviewed changes

Merge branch 'lworld' of https://github.com/openjdk/valhalla into fix…

dbc3fa1

…/unsafe-flat-cas

Fixes, VHTMHAV now passes

0b95903

liach added 2 commits January 19, 2026 11:49

Remove test as permitted

2c373de

Merge branch 'lworld' of https://github.com/openjdk/valhalla into fix…

c165964

…/unsafe-flat-cas

mcimadamore reviewed Jan 21, 2026

View reviewed changes

mcimadamore approved these changes Jan 21, 2026

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 21, 2026

Comment update

312b042

8368274: [lworld] Unsafe.compareAndExchangeFlatValue() wrongly assumes padding bytes are the same for identical value objects #1734

Are you sure you want to change the base?

8368274: [lworld] Unsafe.compareAndExchangeFlatValue() wrongly assumes padding bytes are the same for identical value objects #1734

Conversation

liach commented Nov 12, 2025 • 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 Nov 12, 2025

Uh oh!

openjdk bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TobiHartmann Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Jan 9, 2026

Uh oh!

merykitty Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

TobiHartmann commented Jan 15, 2026

Uh oh!

liach commented Jan 17, 2026

Uh oh!

TobiHartmann commented Jan 19, 2026

Uh oh!

liach commented Jan 19, 2026

Uh oh!

mcimadamore Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

mcimadamore left a comment

Choose a reason for hiding this comment

Uh oh!

liach commented Jan 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

liach commented Nov 12, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Nov 12, 2025 •

edited

Loading

mlbridge bot commented Dec 8, 2025 •

edited

Loading