8353236: [lworld] Better documentation for Valhalla Unsafe intrinsics #1425
Conversation
|
👋 Welcome back qamai! A progress list of the required criteria for merging this PR into |
|
@merykitty 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 85 new commits pushed to the
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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. ➡️ To flag this PR as ready for integration with the above commit message, type |
Webrevs
|
| * with illegal arguments, undefined behavior may time travel. That is, if a | ||
| * control path may eventually reach an invocation of an {@code Unsafe} method | ||
| * with illegal arguments, the symptoms of undefined behavior may be present | ||
| * even before the invocation of the {@code Unsafe} method. |
There was a problem hiding this comment.
I think you can talk in a more JIT-oriented manner, that while Unsafe is mostly set up to be the hooks between core libraries and hotspot runtime, hotspot compiler may interpret the input values and perform aggressive optimizations with contracts that cannot be explicitly expressed by Java program semantics.
There was a problem hiding this comment.
Done, not only the JIT, but the runtime may exhibit unpredictable behaviors in the presence of UB, too.
| * By default, usage of all methods in this class exhibits undefined behavior, | ||
| * unless otherwise explicitly specified. |
There was a problem hiding this comment.
The "by default" and "unless otherwise specified" seems to be redundant.
| * By default, usage of all methods in this class exhibits undefined behavior, | |
| * unless otherwise explicitly specified. | |
| * Unless otherwise explicitly specified, all methods in this class can exhibit undefined behavior. | |
| * . |
There was a problem hiding this comment.
I decide to just remove it as each method specifies its behaviours already.
| * value of this method. The object must also be not assigned to another | ||
| * local variable. |
There was a problem hiding this comment.
Why this restriction? What assumption is violated if there is a second assignment?
There was a problem hiding this comment.
There is none, but it would be easier to specify the restriction on Unsafe::finishPrivateBuffer as the only variable referring to the larval object is the one passed into that method. Otherwise, we must say that all variables referring to the object must not be used. This, however, sounds like a runtime constraint while what we really mean is a compile-time constraint. As a result, I think this restriction is harmless but make our life easier.
| * {@code Unsafe::putXXX} or to {@link #finishPrivateBuffer(Object)}, any | ||
| * other usage, such as loading from or returning it, is illegal. The only | ||
| * exception is the implicit check cast inserted by the compiler on the | ||
| * return value of this method. Explicit check casts are not allowed. |
There was a problem hiding this comment.
"not allowed"? But there is no enforcement; unpredictable behavior yes.
There was a problem hiding this comment.
Not allowed here in the sense that failure to do so will result in undefined behavior.
| * exception is the implicit check cast inserted by the compiler on the | ||
| * return value of this method. Explicit check casts are not allowed. | ||
| * </ul> | ||
| * Illegal usage of this method exhibits undefined behavior even if the |
There was a problem hiding this comment.
may or can; but not always.
There was a problem hiding this comment.
Illegal usage ALWAYS exhibits undefined behavior, the uncertainty here is that whether this undefined behavior manifests itself as an unexpected operation.
| * Illegal usage of this method exhibits undefined behavior even if the | ||
| * illegal statements are never actually reached at runtime. |
There was a problem hiding this comment.
Another can/may exhibit case; improve the phrase everywhere.
| * {@link #makePrivateBuffer(Object)}. | ||
| * <li>After the invocation of this method, the variable that holds the | ||
| * argument passed into this method must not be used. | ||
| * </ul> |
There was a problem hiding this comment.
I would add the exhortation that every field of the value object must have a value.
There was a problem hiding this comment.
Unsafe::makePrivateBuffer takes a non-larval value object so all of its fields always have a value.
|
@merykitty 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 |
Hi,
This patch clarifies the meaning of undefined behavior when working with
Unsafeand specifies the requirements when working withUnsafe::makePrivateBufferandUnsafe::finishPrivateBuffer.Please take a look and leave your suggestions, thanks a lot.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1425/head:pull/1425$ git checkout pull/1425Update a local copy of the PR:
$ git checkout pull/1425$ git pull https://git.openjdk.org/valhalla.git pull/1425/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1425View PR using the GUI difftool:
$ git pr show -t 1425Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1425.diff
Using Webrev
Link to Webrev Comment