jdk
jdk copied to clipboard
openjdk
8332003: Clarify javadoc for MemoryLayout::offsetHandle
Consider this layout:
MemoryLayout SEQ = MemoryLayout.sequenceLayout(5,
MemoryLayout.sequenceLayout(10, JAVA_INT));
And the corresponding offset handle:
MethodHandle offsetHandle = SEQ.offsetHandle(PathElement.sequenceLayout(), PathElement.sequenceLayout());
The resulting method handle takes two additional long indices. The implementation checks that the dynamically provided indices conform to the bound available at construction: that is, the first index must be < 5, while the second must be < 10. If this is not true, an IndexOutOfBoundException is thrown.
However, the javadoc for MemoryLayout::byteOffsetHandle doesn't claim anything in this direction. There are only some vague claims in the javadoc for PathElement::sequenceElement() and PathElement::sequenceElement(long, long, long) which make some claims on which indices are actually allowed, but the text seems more in the tone of a discussion, rather than actual normative text.
I've tweaked the javadoc for MemoryLayout::byteOffsetHandle to actually state that the indices will be checked against the "size" of the corresponding open path element (this is a new concept that I also have defined in the javadoc).
I also added a test for byteOffsetHandle as I don't think we had a test for that specifically (although this method is tested indirectly, via MemoryLayout::varHandle).
Progress
- [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [ ] Change requires CSR request JDK-8332143 to be approved
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
Issues
- JDK-8332003: Clarify javadoc for MemoryLayout::offsetHandle (Bug - P3)
- JDK-8332143: Clarify javadoc for MemoryLayout::offsetHandle (CSR)
Reviewers
- Paul Sandoz (@PaulSandoz - Reviewer)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19158/head:pull/19158
$ git checkout pull/19158
Update a local copy of the PR:
$ git checkout pull/19158
$ git pull https://git.openjdk.org/jdk.git pull/19158/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19158
View PR using the GUI difftool:
$ git pr show -t 19158
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19158.diff
Webrev
:wave: Welcome back mcimadamore! 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@mcimadamore 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:
8332003: Clarify javadoc for MemoryLayout::offsetHandle
Reviewed-by: psandoz
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 25 new commits pushed to the master branch:
- c4867c62c44b48e48845608fe4b29b58749767ad: 8329273: C2 SuperWord: Some basic MemorySegment IR tests
- 8032d640c0d34fe507392a1d4faa4ff2005c771d: 8332245: C2: missing record_for_ign() call in GraphKit::must_be_not_null()
- fa043aec425ae1e3086d09492b3fabcfbd3fa779: 8294880: Review running time of jdk/internal/shellsupport/doc/JavadocHelperTest.java
- a5005c87c4d5598eb54e9824105767d833f9660b: 8330814: Cleanups for KeepAliveCache tests
- 1a944478a26a766f5a573a1236b642d8e7b0685c: 8332111: [BACKOUT] A way to align already compiled methods with compiler directives
- 957eb611ce2531a3fcc764813ad1e0776887fdda: 8331429: [JVMCI] Cleanup JVMCIRuntime allocation routines
- 2f10a316ff0c5a4c124b94f6fabb38fb119d2c82: 8302850: Implement C1 clone intrinsic that reuses arraycopy code for primitive arrays
- c642f44bbe1e4cdbc23496a34ddaae30990ce7c0: 8329839: Cleanup ZPhysicalMemoryBacking trace logging
- d04ac14bdbab4187d0be98b8471f90be8a14f649: 8332236: javac crashes with module imports and implicitly declared class
- 4e77cf881d031e5b0320915b3eabd7702e560291: 8330795: C2: assert((uint)type <= T_CONFLICT && _zero_type[type] != nullptr) failed: bad type with -XX:-UseCompressedClassPointers
- ... and 15 more: https://git.openjdk.org/jdk/compare/1484153c1a092cefc20270b35aa1e508280843a4...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.
@mcimadamore has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@mcimadamore please create a CSR request for issue JDK-8332003 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.
@mcimadamore 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.
Webrevs
- 05: Full - Incremental (38a9924f)
- 04: Full - Incremental (629000d1)
- 03: Full - Incremental (d46387a7)
- 02: Full - Incremental (aa77995c)
- 01: Full - Incremental (aa644d3d)
- 00: Full (7cfd222b)
In an offline discussion @minborg noted that other methods in the MemoryLayout class do not specify in full the set exception thrown by the returned method handle/var handle:
- MemoryLayout::varHandle
- MemoryLayout::sliceHandle
- MemoryLayout::arrayElementVarHandle
The first two are missing details re. checks against the upper bound of the index parameter/coordinates. I believe this omission was because these methods already say that if the accessed offset is bigger than the segment size, then a IIOBE occurs. But there's a subtle problem with it. In the impl, even if you have a big enough memory segment to allow access at index 42, but the layout says that the max index is 41, access fails with IIOBE anyway. So I think the javadoc should be updated to contain the new text about the index checks.
The last method in the list actually doesn't have any text when it comes to the exceptions thrown by the returned handle. This is because we say that this method can be obtained by calling collectCoordinates on MemoryLayout::varHandle. Nevertheless, I think it's worth repeating what exceptions can come up (especially as doing so reveals more clearly that there's no extra checks for the very first long coordinate of the returned handle - so now the existing apiNote makes even more sense, I think).
Going to push as commit 30bb066b1982c5318d54bfe74115306c602e2974.
Since your change was applied there have been 25 commits pushed to the master branch:
- c4867c62c44b48e48845608fe4b29b58749767ad: 8329273: C2 SuperWord: Some basic MemorySegment IR tests
- 8032d640c0d34fe507392a1d4faa4ff2005c771d: 8332245: C2: missing record_for_ign() call in GraphKit::must_be_not_null()
- fa043aec425ae1e3086d09492b3fabcfbd3fa779: 8294880: Review running time of jdk/internal/shellsupport/doc/JavadocHelperTest.java
- a5005c87c4d5598eb54e9824105767d833f9660b: 8330814: Cleanups for KeepAliveCache tests
- 1a944478a26a766f5a573a1236b642d8e7b0685c: 8332111: [BACKOUT] A way to align already compiled methods with compiler directives
- 957eb611ce2531a3fcc764813ad1e0776887fdda: 8331429: [JVMCI] Cleanup JVMCIRuntime allocation routines
- 2f10a316ff0c5a4c124b94f6fabb38fb119d2c82: 8302850: Implement C1 clone intrinsic that reuses arraycopy code for primitive arrays
- c642f44bbe1e4cdbc23496a34ddaae30990ce7c0: 8329839: Cleanup ZPhysicalMemoryBacking trace logging
- d04ac14bdbab4187d0be98b8471f90be8a14f649: 8332236: javac crashes with module imports and implicitly declared class
- 4e77cf881d031e5b0320915b3eabd7702e560291: 8330795: C2: assert((uint)type <= T_CONFLICT && _zero_type[type] != nullptr) failed: bad type with -XX:-UseCompressedClassPointers
- ... and 15 more: https://git.openjdk.org/jdk/compare/1484153c1a092cefc20270b35aa1e508280843a4...master
Your commit was automatically rebased without conflicts.
@mcimadamore Pushed as commit 30bb066b1982c5318d54bfe74115306c602e2974.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.