8288534: Out of bound errors for memory segment access mentions wrong values #24
Conversation
👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into |
@mcimadamore The following label will be automatically applied to this pull request:
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
|
I would expect passing an non-constant exception mapper should work. The C2 intrinsic never accesses the exception mapper argument, and a range check failure results in a deoptimzation back to the interpreter from which the exception mapper is then operated on. However, i think its worth just double checking generated code. |
Both @iwanowww and @rwestrel have confirmed that that should be indeed the case. I did some test with PrintInlining with seemed to show the method as correctly inlined. For extra safety I will run all memory access benchmarks, and compare before/after. |
Benchmarks looks all good except UnrolledAccess::handle_loop_instance. A quick PrintInlining revealed a missed inlining for |
@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:
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 3 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit ff3db52.
Your commit was automatically rebased without conflicts. |
@mcimadamore Pushed as commit ff3db52. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
While playing with the API, I've realized that some of the out of bound error messgaes come out incorrectly.
This is because the bound check is performed as follows (to avoid overflow):
So, if out-of-bounds access is detected, the resulting exception mentions the values of the first and second parameter, respectively - but since the second parameter is the result of a subtraction, it doesn't make sense.
The solution is not to use
Objects.checkIndex
directly, but, instead, drop down one level, and pass our own "IOOB formatter":Note that, in order to recover the correct values, the formatter needs to know the segment size - so I made
AbstractMemorySegment
act as a formatter (so we don't need to allocate in such a hot path).The fix seems to bring back the desired messages - but I would like some confirmation that doing this won't disrupt intrinsification of the
checkIndex
method.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk19 pull/24/head:pull/24
$ git checkout pull/24
Update a local copy of the PR:
$ git checkout pull/24
$ git pull https://git.openjdk.org/jdk19 pull/24/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24
View PR using the GUI difftool:
$ git pr show -t 24
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk19/pull/24.diff