8368274: [lworld] Unsafe.compareAndExchangeFlatValue() wrongly assumes padding bytes are the same for identical value objects by liach · Pull Request #1734 · openjdk/valhalla

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 312b0427

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 no new commits pushed to the lworld branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

mcimadamore · 2026-01-22T15:43:27Z

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

-        // caller pass the array so it can capture the witness if needed
-        // we must witness the raw value instead of the value object, otherwise
-        // garbage value can cause failure
+        // We can convert between a value object and a binary value (of suitable size) using array elements.


Great! Thanks!

openjdk · 2026-01-22T15:45:39Z

@liach this pull request can not be integrated into lworld due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout fix/unsafe-flat-cas
git fetch https://git.openjdk.org/valhalla.git lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push

dcubed-ojdk · 2026-01-22T22:36:14Z

test/jdk/ProblemList.txt


 # valhalla
 build/CheckFiles.java 8376088  generic-all
-java/lang/invoke/VarHandles/VarHandleTestMethodHandleAccessValue.java 8367346 generic-all


This PR's bugID is 8368274 while the ProblemList entry's bugID is 8367346.
It looks like JDK-8367346 is closed as a duplicate of JDK-8368274 so all the
accounting is good here, but I wanted to document what we've done.

liach · 2026-01-22T23:16:31Z

Thanks for the reviews!

/integrate

openjdk · 2026-01-22T23:18:52Z

Going to push as commit ccb0ca8.

openjdk · 2026-01-22T23:19:00Z

@liach Pushed as commit ccb0ca8.

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

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

mcimadamore reviewed Jan 22, 2026

View reviewed changes

mcimadamore approved these changes Jan 22, 2026

View reviewed changes

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Jan 22, 2026

openjdk bot removed the ready Pull request is ready to be integrated label Jan 22, 2026

Merge branch 'lworld' into fix/unsafe-flat-cas

d9a914c

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Jan 22, 2026

dcubed-ojdk reviewed Jan 22, 2026

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 22, 2026

openjdk bot closed this Jan 22, 2026

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 22, 2026

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!

mcimadamore Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jan 22, 2026

Uh oh!

dcubed-ojdk Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

liach commented Jan 22, 2026

Uh oh!

openjdk bot commented Jan 22, 2026

Uh oh!

openjdk bot commented Jan 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 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