8310053: VarHandle and slice handle derived from layout are lacking alignment check #14475

JornVernee · 2023-06-14T19:30:10Z

Add missing alignment checks for the alignment constraint of the root layout of a VarHandle created through MemoryLayout::varHandle and a MethodHandle MemoryLayout::sliceHandle.

Testing: jdk_foreign test suite

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
Change requires CSR request JDK-8310068 to be approved

Issues

JDK-8310053: VarHandle and slice handle derived from layout are lacking alignment check (Bug - P3)
JDK-8310068: VarHandle and slice handle derived from layout are lacking alignment check (CSR)

Reviewers

Maurizio Cimadamore (@mcimadamore - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14475

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-06-14T19:31:12Z

👋 Welcome back jvernee! 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 · 2023-06-14T19:32:55Z

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

core-libs

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 · 2023-06-14T22:03:00Z

Webrevs

mcimadamore · 2023-06-14T22:45:41Z

src/java.base/share/classes/jdk/internal/foreign/LayoutPath.java

-            // (J, ...) -> J to (J, J, ...) -> J
-            // i.e. new coord is prefixed. Last coord will correspond to innermost layout
-            handle = MethodHandles.collectCoordinates(handle, 1, collector);
+        handle = MethodHandles.collectCoordinates(handle, 1, offsetHandle());


nice simplification!

mcimadamore · 2023-06-14T22:48:34Z

test/jdk/java/foreign/TestLayoutPaths.java

+
+            String expectedMessage = "Target offset incompatible with alignment constraints: " + struct.byteAlignment();
+
+            try {


Can assertThrows be used to simplify this pattern?

Yeah. I thought assertThrows was only a junit thing, but it looks like TestNG also has an expectThrows which does the same and returns the thrown exception so that we can check the message.

mcimadamore · 2023-06-14T23:17:18Z

src/java.base/share/classes/java/lang/foreign/MemoryLayout.java

@@ -380,6 +380,10 @@ default MethodHandle byteOffsetHandle(PathElement... elements) {
     * Additionally, the provided dynamic values must conform to bounds which are derived from the layout path, that is,
     * {@code 0 <= x_i < b_i}, where {@code 1 <= i <= n}, or {@link IndexOutOfBoundsException} is thrown.
     * <p>
+     * The accessed address must be <a href="MemorySegment.html#segment-alignment">aligned</a> according to the {@linkplain


Very nice and clear!

Use expectThrows

JornVernee · 2023-06-14T23:32:57Z

I've switched to expectThrows. As discussed offline, I've also dropped the alignment check from the inner accessed element, as this is redundant due to layouts enforcing alignment of nested elements already. So, if the root layout is aligned, so is the element. (this turned out to be simper than I initially thought)

mcimadamore

Bonus points!

mcimadamore · 2023-06-15T00:22:09Z

I'm wondering... for dereference path elements, we already perform a root layout check as part of the memory segment expansion - e.g. we use the target layout to resize the segment (and perform an alignment check). So in that case there would still be duplicate alignment checks.

In such cases, we need to make sure that either (a) only the first var handle in the dereference chain gets the proper alignment adaptation on the input segment or that (b) they all do, but then we omit the alignment checks that are generated as part of calling VH::get on the address layout (but that might be harder to do, as that logic is shared). I think (a) is probably simplest - just have a boolean in the path which says whether a path is nested into some other path using a dereference and, if so, omit the alignment adaptation?

JornVernee · 2023-06-15T11:49:27Z

only the first var handle in the dereference chain gets the proper alignment adaptation on the input segment

That would be easy enough to do, we just check for derefAdapters.length == 0. I'll add this too.

minor javadoc fix add test for misaligned deref

JornVernee · 2023-06-15T13:02:39Z

I've gone ahead and implemented this. I also added a test to make sure that alignment checks still happen when using deref path elements.

I also fixed a minor javadoc issue, in the doc for varHandle. It said that we checked the alignment of the accessed address, but we check the alignment of the base address (a term which is defined earlier).

openjdk · 2023-06-20T17:32:45Z

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

8310053: VarHandle and slice handle derived from layout are lacking alignment check

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

09174e0: 8310049: Refactor Charset tests to use JUnit
99d2a9a: 8310330: HttpClient: debugging interestOps/readyOps could cause exceptions and smaller cleanup
31b6fd7: 8309258: RISC-V: Add riscv_hwprobe syscall
4a9cc8a: 8309266: C2: assert(final_con == (jlong)final_int) failed: final value should be integer
4e4e586: 8310194: Generational ZGC: Lock-order asserts in JVMTI IterateThroughHeap
e1906e7: 8310027: Fix -Wconversion warnings in nmethod and compiledMethod related code
4ca548f: 8310326: Incorrect position of the synthetic unnamed class
a059576: 8310187: Improve Generational ZGC jtreg testing
9a68ec8: 8219357: G1: G1GCPhaseTimes::debug_phase uses unnecessary ResourceMark
0878872: 8310020: MacroAssembler::call_VM(_leaf) doesn't consistently check for conflict with C calling convention.
... and 59 more: https://git.openjdk.org/jdk/compare/57b8251241e2044d5039ce162bf4637a9b2e5466...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.

JornVernee · 2023-06-21T00:01:40Z

/integrate

openjdk · 2023-06-21T00:03:15Z

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

45eaf5e: 8298443: Remove expired flags in JDK 22
28415ad: 8310225: Reduce inclusion of oopMapCache.hpp and generateOopMap.hpp
4c3efb3: 8309034: NoClassDefFoundError when initializing Long$LongCache
1120106: 8310458: Fix build failure caused by JDK-8310049
09174e0: 8310049: Refactor Charset tests to use JUnit
99d2a9a: 8310330: HttpClient: debugging interestOps/readyOps could cause exceptions and smaller cleanup
31b6fd7: 8309258: RISC-V: Add riscv_hwprobe syscall
4a9cc8a: 8309266: C2: assert(final_con == (jlong)final_int) failed: final value should be integer
4e4e586: 8310194: Generational ZGC: Lock-order asserts in JVMTI IterateThroughHeap
e1906e7: 8310027: Fix -Wconversion warnings in nmethod and compiledMethod related code
... and 63 more: https://git.openjdk.org/jdk/compare/57b8251241e2044d5039ce162bf4637a9b2e5466...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-21T00:03:24Z

@JornVernee Pushed as commit e022e87.

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

JornVernee · 2023-06-21T00:04:14Z

/backport jdk21

openjdk · 2023-06-21T00:05:38Z

@JornVernee the backport was successfully created on the branch JornVernee-backport-e022e876 in my personal fork of openjdk/jdk21. To create a pull request with this backport targeting openjdk/jdk21:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit e022e876 from the openjdk/jdk repository.

The commit being backported was authored by Jorn Vernee on 21 Jun 2023 and was reviewed by Maurizio Cimadamore.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21:

$ git fetch https://github.com/openjdk-bots/jdk21.git JornVernee-backport-e022e876:JornVernee-backport-e022e876
$ git checkout JornVernee-backport-e022e876
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21.git JornVernee-backport-e022e876

Add root layout alignment check to varHandle and sliceHandle

f984574

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jun 14, 2023

JornVernee marked this pull request as ready for review June 14, 2023 21:57

JornVernee changed the title ~~8310053: VarHandle and slice handle derived from layout is lacking alignment check~~ 8310053: VarHandle and slice handle derived from layout are lacking alignment check Jun 14, 2023

openjdk bot added csr Pull request needs approved CSR before integration rfr Pull request is ready for review labels Jun 14, 2023

JornVernee added 2 commits June 15, 2023 00:14

add note to sliceHandle too

1abe758

formatting

668e9c3

mcimadamore reviewed Jun 14, 2023

View reviewed changes

mcimadamore approved these changes Jun 14, 2023

View reviewed changes

mcimadamore reviewed Jun 14, 2023

View reviewed changes

Drop alignment checks from accessed elements

9907626

Use expectThrows

mcimadamore approved these changes Jun 14, 2023

View reviewed changes

only check alignment of root for the first deref in a chain

b486d46

minor javadoc fix add test for misaligned deref

mcimadamore approved these changes Jun 15, 2023

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Jun 20, 2023

openjdk bot added the integrated Pull request has been integrated label Jun 21, 2023

openjdk bot closed this Jun 21, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 21, 2023

JornVernee deleted the RootLayoutCheck branch June 21, 2023 00:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8310053: VarHandle and slice handle derived from layout are lacking alignment check #14475

8310053: VarHandle and slice handle derived from layout are lacking alignment check #14475

JornVernee commented Jun 14, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 14, 2023

openjdk bot commented Jun 14, 2023

mlbridge bot commented Jun 14, 2023 •

edited

Loading

mcimadamore Jun 14, 2023

mcimadamore Jun 14, 2023

JornVernee Jun 14, 2023 •

edited

Loading

mcimadamore Jun 14, 2023

JornVernee commented Jun 14, 2023

mcimadamore left a comment

mcimadamore commented Jun 15, 2023 •

edited

Loading

JornVernee commented Jun 15, 2023

JornVernee commented Jun 15, 2023

openjdk bot commented Jun 20, 2023

JornVernee commented Jun 21, 2023

openjdk bot commented Jun 21, 2023

openjdk bot commented Jun 21, 2023

JornVernee commented Jun 21, 2023

openjdk bot commented Jun 21, 2023


		String expectedMessage = "Target offset incompatible with alignment constraints: " + struct.byteAlignment();

		try {

8310053: VarHandle and slice handle derived from layout are lacking alignment check #14475

8310053: VarHandle and slice handle derived from layout are lacking alignment check #14475

Conversation

JornVernee commented Jun 14, 2023 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jun 14, 2023

openjdk bot commented Jun 14, 2023

mlbridge bot commented Jun 14, 2023 • edited Loading

Webrevs

mcimadamore Jun 14, 2023

Choose a reason for hiding this comment

mcimadamore Jun 14, 2023

Choose a reason for hiding this comment

JornVernee Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

mcimadamore Jun 14, 2023

Choose a reason for hiding this comment

JornVernee commented Jun 14, 2023

mcimadamore left a comment

Choose a reason for hiding this comment

mcimadamore commented Jun 15, 2023 • edited Loading

JornVernee commented Jun 15, 2023

JornVernee commented Jun 15, 2023

openjdk bot commented Jun 20, 2023

JornVernee commented Jun 21, 2023

openjdk bot commented Jun 21, 2023

openjdk bot commented Jun 21, 2023

JornVernee commented Jun 21, 2023

openjdk bot commented Jun 21, 2023

JornVernee commented Jun 14, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 14, 2023 •

edited

Loading

JornVernee Jun 14, 2023 •

edited

Loading

mcimadamore commented Jun 15, 2023 •

edited

Loading