8378560: [lworld] Reflective construction of value object triggers assert in C2

[merykitty]

Hi,

Reflective construction of value object triggers assert in C2 because it does not follow the normal object construction pattern and is technically UB because we try to return a larval object from a method. I was told that this is required for the construction of hidden classes, but to me it seems like we put those restrictions on ourselves and shoot ourselves in the foot by using these Unsafe hacks.

This PR tries to fix this issue by letting the compiler know of these methods which can return or accept larval objects. Note that this is pretty fragile, and seemingly harmless changes to the code shape generated by the MethodHandle mechanism can break it, which is a usual symptom of undefined behaviour.

Please take a look and leave your review, thanks a lot.

---

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-8378560: [lworld] Reflective construction of value object triggers assert in C2 (Bug - P4)

Reviewers

• Christian Hagedorn (@chhagedorn - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2204

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2204.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back qamai! 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]

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

8378560: [lworld] Reflective construction of value object triggers assert in C2

Reviewed-by: chagedorn

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 110 new commits pushed to the lworld branch:

• 4e71493: 8378771: [lworld] JFR Cooperative Sampling support for needs_stack_repair methods and frames
• 12012cc: 8378862: [lworld] flatArrayKlass::copy_array is missing a memory barrier when copying from flat array to non-flat array
• 432870d: 8379184: [lworld] Cleanup fieldLayoutBuilder.cpp to remove get_size_and_alignment
• ... and 107 more: https://git.openjdk.org/valhalla/compare/a2f9166fc80b4708d3c854e1d86c069968aa2b7d...lworld

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 lworld branch, type /integrate in a new comment.

[mlbridge]

Webrevs

• 02: Full - Incremental (223c325d)
• 01: Full - Incremental (7659cd27)
• 00: Full (f9de2810)

[liach]

The recognition of "may be larval" seems sufficient. Is it okay if it recognizes non-larval calls like invokespecial on private methods as "may be larval"? Would that create potential regressions?

[merykitty]

@liach I think it is okay, values are scalarized when they are pushed onto the stack, so when we invoke a call, the arguments are scalarized already. In addition, linkToSpecial is a link method, that means what we inline/call is often the target of the link, not the link itself.

[liach]

As long as this won't create bugs, I think this should be fine. You can add comments on the DMH and Unsafe methods and linkToSpecial in this patch or I can add the comments as a later cleanup.

[merykitty]

I think it is better to leave it to a following PR, as it touches unrelated files, and the documentation should also clarify what is undefined behavior, this is my draft.

diff --git a/src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java b/src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java
index 28a4177e93f..d5dc92890c9 100644
--- a/src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java
+++ b/src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java
@@ -504,6 +504,13 @@ static Object constructorMethod(Object mh) {
     }
 
     /*non-public*/
+
+    /**
+     * This method returns an uninitialized instance. In general, this is undefined behavior, this
+     * method is treated specially by the JVM to allow this behavior. The returned value must be
+     * passed into a constructor using {@link MethodHandle#linkToSpecial}, otherwise, the behavior
+     * is undefined.
+     */
     static Object allocateInstance(Object mh) throws InstantiationException {
         Constructor dmh = (Constructor)mh;
         return UNSAFE.allocateInstance(dmh.instanceClass);
diff --git a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
index bb1bd95df30..7a3b5b74fa2 100644
--- a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
+++ b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
@@ -42,14 +42,55 @@
  * Although the class and all methods are public, use of this class is
  * limited because only trusted code can obtain instances of it.
  *
- * <em>Note:</em> It is the responsibility of the caller to make sure
- * arguments are checked before methods of this class are
- * called. While some rudimentary checks are performed on the input,
- * the checks are best effort and when performance is an overriding
- * priority, as when methods of this class are optimized by the
- * runtime compiler, some or all checks (if any) may be elided. Hence,
- * the caller must not rely on the checks and corresponding
- * exceptions!
+ * <h2><a id="undefined-behavior">Undefined Behavior</a></h2>
+ * For performance reasons, {@code Unsafe} is allowed to work outside the
+ * restrictions enforced by the JVM. As a result, it is the responsibility of
+ * the caller to ensure that an invocation of an {@code Unsafe} method is
+ * conformant, and failure to do so will result in undefined behavior. The
+ * runtime and the JIT compiler may assume that undefined behavior never
+ * happens, and operate accordingly. For example, the runtime assumes that each
+ * object has a header with a particular layout, and if the users use
+ * {@code Unsafe} to overwrite this header with invalid data, the behavior of
+ * the runtime becomes unpredictable. Another example is that the JIT compiler
+ * may assume that accesses on separate objects are unrelated, and schedule
+ * each of them without taking into consideration the others. If there is an
+ * {@code Unsafe} access that is out of bounds and points to object different
+ * from the declared base, the program may execute in a way that a variable
+ * seems to have multiple values at the same time. As a result, when a program
+ * exhibits undefined behavior, there is no restrictions on its behaviors. Such
+ * behaviors may include but not be limited to:
+ *
+ * <ul>
+ * <li>Working as expected.
+ * <li>Crashing the VM.
+ * <li>Corruption of the heap or JVM memory.
+ * <li>Nonsensical variable value. E.g. an {@code int} may appear to be
+ * simultaneously 0 and 1.
+ * <li>Impossible code execution. E.g. the branches of an {@code if} are
+ * both executed or both not executed.
+ * <li>Wiping out the hard drive.
+ * </ul>
+ *
+ * Undefined behavior, as described in this class, is analogous to the
+ * terminology with the same name in the C++ language.
+ * <p>
+ * Some methods (e.g. {@link #getInt}) exhibit undefined behavior if they
+ * are invoked at runtime with illegal arguments. This means that they will
+ * never exhibit undefined behavior if they are not actually reachable at
+ * runtime. On the other hands, other methods (e.g.
+ * {@link #allocateInstance(Class)}) exhibit undefined behavior if they are
+ * used incorrectly, even if the invocation may not be reachable at runtime.
+ * The analogous terminology in C++ is that such programs are ill-formed.
+ * <p>
+ * For methods exhibiting undefined behavior if they are invoked at runtime
+ * 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. This is because the
+ * JIT compiler may have certain assumptions about the inputs of an
+ * {@code Unsafe} invocation, these assumptions may propagate backward to
+ * previous statements, leading to wrong executions if the assumptions are
+ * invalid.
  *
  * @author John R. Rose
  * @see #getUnsafe
@@ -1596,6 +1637,11 @@ public native Class<?> defineClass0(String name, byte[] b, int off, int len,
     /**
      * Allocates an instance but does not run any constructor.
      * Initializes the class if it has not yet been.
+     * <p>
+     * This method returns an uninitialized instance. In general, this is undefined behavior, this
+     * method is treated specially by the JVM to allow this behavior. The returned value must be
+     * passed into a constructor using {@link java.lang.invoke.MethodHandle#linkToSpecial},
+     * otherwise, the behavior is undefined.
      */
     @IntrinsicCandidate
     public native Object allocateInstance(Class<?> cls)

[chhagedorn]

Otherwise, looks good, thanks!

[chhagedorn]

Can you also give a description about the exceptions here? Maybe you can also move it to the definition to easier follow the cases outlined.

[merykitty]

Thanks a lot for your review, I have added the description to state the examples at the definition sites.

[chhagedorn]

Thanks a lot, that is very helpful!

[merykitty]

Thanks a lot for your review.

/integrate

[openjdk]

Going to push as commit 3c48db9.
Since your change was applied there have been 117 commits pushed to the lworld branch:

• db5c187: 8379791: [lworld] C2: Blackhole wrongly treated as subclass of MemBar in NodeClasses enum
• 8aada8b: 8379906: [lworld] CLONE - Suppressed warnings reported when implicit compilation is combined with annotation processing
• 80c3047: 8379820: [lworld] tools/javac/launcher/SourceLauncherTest.java fails with error: release version 21 not supported
• ... and 114 more: https://git.openjdk.org/valhalla/compare/a2f9166fc80b4708d3c854e1d86c069968aa2b7d...lworld

Your commit was automatically rebased without conflicts.

[openjdk]

@merykitty Pushed as commit 3c48db9.

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