valhalla 8352647: [lworld] Remove larval InlineTypeNode in Unsafe intrinsics

Hi,

As a step of #1405, I would like to remove the usage of larval value objects in Unsafe intrinsics. This patch also tries to make sure that Unsafe::makePrivateBuffer and Unsafe::finishPrivateBuffer are always inlined, so that larval objects do not cross call boundaries except for invoking object constructors.

Please take a look and review this PR, thanks a lot.

Progress

[x] Change must not contain extraneous whitespace

Issue

JDK-8352647: [lworld] Remove larval InlineTypeNode in Unsafe intrinsics (Enhancement - P4)

Reviewers

Jatin Bhateja (@jatin-bhateja - Committer) ⚠️ Review applies to 3c3082ae

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1406

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

Using diff file

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

Using Webrev

Link to Webrev Comment

Mar 23 '25 17:03 merykitty

:wave: 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.

Mar 23 '25 17:03 bridgekeeper[bot]

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

8352647: [lworld] Remove larval InlineTypeNode in Unsafe intrinsics

Reviewed-by: jbhateja, thartmann

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

a85d0a25d10d1dd2158b0483e38f547f9564cd39: 8353752: [lworld] Remove handcoded offsets of field offsets for boxing classes
4f7decde6538902d4bedebd0497d8123d5fb9e27: Remove last use of the default value
293a9c406f784c1d8d165574487e58c475739074: 8353746: [lworld] Remove the default value and @ImplictlyConstructible annotation
a8ad27534166e0956c6e5cfd4dbd0578f408cff7: Merge jdk
a347ecdedc098bd23598ba6acf28d77db01be066: 8350905: Shenandoah: Releasing a WeakHandle's referent may extend its lifetime
5502ce733e77efa9f40116dd0e34d4d2333a48dc: 8351699: Problem list com/sun/jdi/JdbStopInNotificationThreadTest.java with ZGC
cdf7632f8a85611077a27c91ad928ed8ea116f95: 8351444: Shenandoah: Class Unloading may encounter recycled oops
930455b59608b547017c9649efeb6bd381340c34: 8351640: Print reason for making method not entrant
895f64a18d7c752332ef9255c0b118bf25bdbb90: 8351142: Add JFR monitor deflation and statistics events
db531bf7df517eb6a07080aceb2a88a3b90d5e94: 8351881: Tidy complains about missing "alt" attribute
... and 87 more: https://git.openjdk.org/valhalla/compare/eae2c5243f1adafd7678d2f73a503b628ae6f419...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jatin-bhateja, @TobiHartmann) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

Mar 23 '25 17:03 openjdk[bot]

Webrevs

Mar 23 '25 17:03 mlbridge[bot]

I just had a look at https://github.com/openjdk/valhalla/pull/1405, thanks for working on this @merykitty.

so that larval objects do not cross call boundaries except for invoking object constructors

But this will not prevent users from passing larval objects created via makePrivateBuffer to other methods, right? I.e., we still have the issues described in JDK-8239003 (and linked issues).

Mar 26 '25 09:03 TobiHartmann

@TobiHartmann

But this will not prevent users from passing larval objects created via makePrivateBuffer to other methods, right? I.e., we still have the issues described in JDK-8239003 (and linked issues).

My stance on this matter is that we should simply disallow passing larval objects except to Unsafe::putXXX and Unsafe::finishPrivateBuffer. What do you think?

Mar 26 '25 14:03 merykitty

My stance on this matter is that we should simply disallow passing larval objects except to Unsafe::putXXX and Unsafe::finishPrivateBuffer. What do you think?

Right, I agree, maybe we should put some safeguards in place for this at some point. Deoptimization is still an issue though (see JDK-8322547). Anyway, it's out-of-scope of this patch.

Mar 26 '25 14:03 TobiHartmann

Hi @merykitty , For attached test case I am seeing crash at state merge point b/w larval and non-larval value

unsafe_access.txt

CPROMPT>javac --enable-preview -source 25 --add-exports=java.base/jdk.internal.misc=ALL-UNNAMED unsafe_access.java
Note: unsafe_access.java uses preview features of Java SE 25.
Note: Recompile with -Xlint:preview for details.
CPROMPT>
CPROMPT>java -XX:-PauseAtStartup --enable-preview --add-exports=java.base/jdk.internal.misc=ALL-UNNAMED -cp . unsafe_access
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (/home/jatinbha/sandboxes/lworld/src/hotspot/share/opto/parse1.cpp:1790), pid=2453805, tid=2453825
#  assert(gvn().type(n)->is_zero_type()) failed: Should have been scalarized
#
# JRE version: OpenJDK Runtime Environment (25.0) (slowdebug build 25-internal-adhoc.root.lworld)
# Java VM: OpenJDK 64-Bit Server VM (slowdebug 25-internal-adhoc.root.lworld, mixed mode, sharing, tiered, compressed oops, compressed class ptrs, g1 gc, linux-amd64)
# Problematic frame:
# V  [libjvm.so+0x178efa5]  Parse::merge_common(Parse::Block*, int)+0x49f
#
# Core dump will be written. Default location: Core dumps may be processed with "/usr/share/apport/apport -p%p -s%s -c%c -d%d -P%P -u%u -g%g -- %E" (or dumping to /home/jatinbha/code/java/valhalla/core.2453805)
#
# An error report file with more information is saved as:
# /home/jatinbha/code/java/valhalla/hs_err_pid2453805.log
10001399990
#
# Compiler replay data is saved as:
# /home/jatinbha/code/java/valhalla/replay_pid2453805.log
#
# If you would like to submit a bug report, please visit:
#   https://bugreport.java.com/bugreport/crash.jsp
#
Aborted (core dumped)

Mar 28 '25 08:03 jatin-bhateja

@jatin-bhateja Merging a larval object and a non-larval object is undefined behaviour, and crashing should be an acceptable outcome then. There are issues with merges of larval objects, though, for example:

MyValue v = UNSAFE.makePrivateBuffer(v0);
for (int i = 0; i < 100; i++) {}
return UNSAFE.finishPrivateBuffer(v);

But generally, the situation of merging larval objects are messy not just with larval objects created by Unsafe and I want to defer the fix to after #1405.

Mar 28 '25 16:03 merykitty

@jatin-bhateja Merging a larval object and a non-larval object is undefined behaviour, and crashing should be an acceptable outcome then. There are issues with merges of larval objects, though, for example:
MyValue v = UNSAFE.makePrivateBuffer(v0);
for (int i = 0; i < 100; i++) {}
return UNSAFE.finishPrivateBuffer(v);
But generally, the situation of merging larval objects are messy not just with larval objects created by Unsafe and I want to defer the fix to after #1405.

Thanks, @merykitty. I think MEET over larval and non-larval should result in a larval value, so InlineTypeNode, which is the other input of the Phi node, should be buffered and treated as a larval object too, until we hit upon a finishPrivate buffer call that brings this merged oop value out of the larval state and re-materialize an InlineTypeNode.

Kindly include this test with this patch. for now, we can add it to ProblemList.txt, which can be enabled after #1405 .

Mar 28 '25 19:03 jatin-bhateja

@jatin-bhateja A larval object created by Unsafe should adhere to all the rules that are put on a larval object created by the new bytecode. In the type verification system, each larval object has a distinct type being either uninitializedThis or uninitialized(Offset). Meeting of a larval object with another larval object or with a non-larval object, thus, will result in a non-typed reference. As a result, the program is ill-formed if the merge is used. Please refer to the JVMS, section 4.10.1.

Mar 28 '25 19:03 merykitty

@jatin-bhateja A larval object created by Unsafe should adhere to all the rules that are put on a larval object created by the new bytecode. In the type verification system, each larval object has a distinct type being either uninitializedThis or uninitialized(Offset). Meeting of a larval object with another larval object or with a non-larval object, thus, will result in a non-typed reference. As a result, the program is ill-formed if the merge is used. Please refer to the JVMS, section 4.10.1.

Hi @merykitty , Larval transitions are either implicit, i.e. when a new value type instance is created, it begins in larval state and then post initilization transition out of larval state. The second kind of larval transition is explicit when we use UNSAFE APIs.

While an object is in the larval state, it should not exist in scalarized form, thus, a larval object should be buffered; it still is a legitimate value instance, but because it is assigned a buffer location, we can say for now it has a pseudo-identity. The compiler cannot directly forward the field initialization of larvals to its user; a read of a field value of a larval object is similar to getfield, i.e., the compiler needs to explicitly load the field value at the corresponding field offset from the base.

Given these semantics, a MEET b/w a larval and non-larval value should result in a laval value when merging the states of two basic blocks. I don't fully follow your comment on non-typed reference, as larval and non-larval instances are of value type. I think your approach to buffer the value type instance within the makePrivate buffer looks right to me. Please include the test case that I shared with the patch and add it to ProblemList.txt for now.

Mar 29 '25 08:03 jatin-bhateja

@jatin-bhateja Note that for the verifier, uninitialized and MyValue are completely different types that have their closest common ancestor being reference (section 4.10.1.2).

When a new MyValue instruction is executed, it pushes onto the stack a value of type uninitialized(Offset), not a value of type MyValue (section 4.10.1.9.new). The only action that can be done with a value of this type is to pass it into a constructor, after which all instances of type uninitialized(Offset) on the stack are replaced with the a value of type MyValue (section 4.10.1.9.invokespecial).

Inside the constructor MyValue::<init>, the first parameter has its type being uninitializedThis (section 4.10.1.6). There is nothing that can be done with a value of this type except passing it to a constructor of the superclass or executing putfield instructions. This value will be replaced by a value of type MyValue after the execution of the constructor of the superclass.

Unsafe lets you bypass the restriction that final fields can only be set inside the constructor of its holder class. To achieve this, it lies to the bytecode verifier that you are having a value of type MyValue when in reality the returned value of Unsafe::makePrivateBuffer is a value of type uninitialized. However, you still must adhere to the rules that are put on the values of type uninitialized, such as:

You cannot pass a larval object to any method except to Unsafe::putXXX or Unsafe::finishPrivateBuffer, which are designed to allow working with larval objects.
You cannot load from a field of a larval object.
Meeting of an uninitialized with a MyValue results in a value of type reference, which you cannot possibly work with. As a result, you cannot assign to a non-larval local variable with a larval value.

Mar 29 '25 15:03 merykitty

@jatin-bhateja Note that for the verifier, uninitialized and MyValue are completely different types that have their closest common ancestor being reference (section 4.10.1.2).

When a new MyValue instruction is executed, it pushes onto the stack a value of type uninitialized(Offset), not a value of type MyValue (section 4.10.1.9.new). The only action that can be done with a value of this type is to pass it into a constructor, after which all instances of type uninitialized(Offset) on the stack are replaced with the a value of type MyValue (section 4.10.1.9.invokespecial).

Inside the constructor MyValue::<init>, the first parameter has its type being uninitializedThis (section 4.10.1.6). There is nothing that can be done with a value of this type except passing it to a constructor of the superclass or executing putfield instructions. This value will be replaced by a value of type MyValue after the execution of the constructor of the superclass.

Unsafe lets you bypass the restriction that final fields can only be set inside the constructor of its holder class. To achieve this, it lies to the bytecode verifier that you are having a value of type MyValue when in reality the returned value of Unsafe::makePrivateBuffer is a value of type uninitialized. However, you still must adhere to the rules that are put on the values of type uninitialized, such as:

You cannot pass a larval object to any method except to Unsafe::putXXX or Unsafe::finishPrivateBuffer, which are designed to allow working with larval objects.

You cannot load from a field of a larval object.

Meeting of an uninitialized with a MyValue results in a value of type reference, which you cannot possibly work with. As a result, you cannot assign to a non-larval local variable with a larval value.

Hi @merykitty ,

Meeting of an uninitialized with a MyValue results in a value of type reference, "

Yes, I agree, this should be seen on the lines of a primitive (value) meeting an identity object (boxes),

https://openjdk.org/projects/valhalla/design-notes/state-of-valhalla/01-background#:~:text=Java%E2%80%99s%20eight%20built%2Din%20primitive%20types%20are%20not%20objects%2C%20but%20they%20have%20boxes.%20When%20primitives%20want%20to%20interact%20in%20the%20world%20of%20objects%2C%20we%20transparently%20convert%20them%20to%20and%20from%20their%20corresponding%20box

https://openjdk.org/projects/valhalla/design-notes/state-of-valhalla/01-background#:~:text=Objects%20have%20identity%2C%20whereas%20primitives%20do%20not%3B%20boxing%20is%20not%20able%20to%20fully%20paper%20over%20this%20gap.%20Each%20time%20we%20convert%20from%20Integer%20to%20int%20the%20identity%20is%20lost%2C%20and%20each%20time%20we%20convert%20from%20int%20to%20an%20Integer%2C%20a%20fresh%20(but%20accidental)%20identity%20is%20created

Laraval value carries an identity while value (MyValue) does not, larval value still adheres to semantic restrictions of value type, i.e should not be used for synchronization, non-tearable etc.

Thus, here at merge point, MyValue should be buffered, since this is MEETing a larval value so new oop (Phi) should be larval, similar to following

"Merging a larval object and a non-larval object is undefined behaviour, and crashing should be an acceptable outcome then. "

Do you think we should get the semantics documented in the specification first or let the spec evolve from the implementation you choose?

Mar 29 '25 22:03 jatin-bhateja

Yes, I agree, this should be seen on the lines of a primitive (value) meeting an identity object (boxes),

I think you are misunderstanding, the type reference here refers to a verification type that represents the most general reference type, it is even higher than java.lang.Object. From the JVMS:

Verification type hierarchy:

                             top
                 ____________/\____________
                /                          \
               /                            \
            oneWord                       twoWord
           /   |   \                     /       \
          /    |    \                   /         \
        int  float  reference        long        double             <--- meeting of a larval value and a non-larval value results in the `reference` type here
                     /     \
                    /       \_____________
                   /                      \
                  /                        \
           uninitialized                    +------------------+    <--- larval objects have their verification type being `uninitialized`
            /         \                     |  Java reference  |    <--- `java.lang.Object` and its subclasses are here
           /           \                    |  type hierarchy  |
uninitializedThis  uninitialized(Offset)    +------------------+  
                                                     |
                                                     |
                                                    null

Thus, here at merge point, MyValue should be buffered, since this is MEETing a larval value so new oop (Phi) should be larval, similar to following

Note that this implicit boxing and unboxing only happens at the language level, javac will automatically translate ret = genInt(val) into ret = Integer.valueOf(genInt(val)). So, at the VM level, we do not ever encounter a meet of a java.lang.Integer and an int. As a result, C2 does not have to take auto boxing and unboxing into consideration.

Mar 30 '25 02:03 merykitty

To be clear, your second example is irrelevant also because a java.lang.Integer is not a larval int, it is a fully constructed int and its identity comes from the lack of value classes, which will eventually be removed once value classes land. A more direct translation of your first example to non-unsafe would be something like:

p = new Point;
p.<init>(val, val + 10);
if (val > 5000) {
    q = new Point;
    q.f1 = p.f1;
    q.f2 = p.f2;
    p = q;
    p.f1 = val + 10;
}
last_hash = p.hashCode();
if (val > 5000) {
    p.<init>();
}
return p;

And I am sure the verifier would reject a bytecode sequence like this.

Mar 30 '25 05:03 merykitty

Yes, I agree, this should be seen on the lines of a primitive (value) meeting an identity object (boxes),

I think you are misunderstanding, the type reference here refers to a verification type that represents the most general reference type, it is even higher than java.lang.Object. From the JVMS:
Verification type hierarchy:

                             top
                 ____________/\____________
                /                          \
               /                            \
            oneWord                       twoWord
           /   |   \                     /       \
          /    |    \                   /         \
        int  float  reference        long        double             <--- meeting of a larval value and a non-larval value results in the `reference` type here
                     /     \
                    /       \_____________
                   /                      \
                  /                        \
           uninitialized                    +------------------+    <--- larval objects have their verification type being `uninitialized`
            /         \                     |  Java reference  |    <--- `java.lang.Object` and its subclasses are here
           /           \                    |  type hierarchy  |
uninitializedThis  uninitialized(Offset)    +------------------+  
                                                     |
                                                     |
                                                    null
Thus, here at merge point, MyValue should be buffered, since this is MEETing a larval value so new oop (Phi) should be larval, similar to following

Note that this implicit boxing and unboxing only happens at the language level, javac will automatically translate ret = genInt(val) into ret = Integer.valueOf(genInt(val)). So, at the VM level, we do not ever encounter a meet of a java.lang.Integer and an int. As a result, C2 does not have to take auto boxing and unboxing into consideration.

Exactly, MEET b/w two reference type results in a type which is LCA of both, and in the worst cast it could be j.l.o

Here, we are dealing with larval and non-larval values, which are different representations of same value type instance.

Our discussion context is around the VM model as language semantics are clear on make/finishPrivate buffer semantics.

In the above example, I drew an analogy b/w boxes as larval values VS primitives as value type instances.

  public static Point micro(int val) throws Exception {
       Point p = new Point(val, val + 10);
       if (val > 50000) {
           p = UNSAFE.makePrivateBuffer(p);
           UNSAFE.putInt(p, UNSAFE.objectFieldOffset(Point.class.getDeclaredField("f1")), val + 10);
       }
       last_hash = p.hashCode();
       if (val > 50000) {
           p = UNSAFE.finishPrivateBuffer(p);
       }
       return p;
   }

"Merging a larval object and a non-larval object is undefined behaviour, and crashing should be an acceptable outcome then. "

Please mention your strategy to handle the above case and what you mean by crash as an acceptable outcome.

We can think through this and address this as a larger fix in PR 1405, for now I am ok with you fix and if can checkin the test and add it to ProblemList.txt

Mar 30 '25 07:03 jatin-bhateja

Exactly, MEET b/w two reference type results in a type which is LCA of both, and in the worst case it could be j.l.o

No, it is wrong, from the verification type system, the highest reference type in the type hierarchy is reference, which comprises java.lang.Object and larval objects (uninitialized). Larval objects have their types being subtypes of uninitialized and non-larval objects have their types being subtypes of java.lang.Object. These 2 are separate under the bytecode verifier.

Please mention your strategy to handle the above case and what you mean by crash as an acceptable outcome.

Crashing is an acceptable outcome when you are dealing with Unsafe. Using Unsafe means you are lying to the VM about the actual thing you are doing and have to rely on your own verification to make sure the program is valid. If the program using Unsafe is invalid (e.g. writing arbitrary value to a reference field), there is nothing the VM can do.

Mar 30 '25 07:03 merykitty

Exactly, MEET b/w two reference type results in a type which is LCA of both, and in the worst case it could be j.l.o

No, it is wrong, from the verification type system, the highest reference type in the type hierarchy is reference, which comprises java.lang.Object and larval objects (uninitialized). Larval objects have their types being subtypes of uninitialized and non-larval objects have their types being subtypes of java.lang.Object. These 2 are separate under the bytecode verifier.

Bytecode verification should be same for larval and non-larval objects, you are mixing explicit and implicit larval here which is where the confusion arises.

MEET operation semantics are in accordance with semantics defined in following logic https://github.com/openjdk/valhalla/blob/lworld/src/hotspot/share/opto/type.cpp#L4734

With UNSAFE we explicit transition the value to a mutable state and thus need to handle them at merge point if it meets a non-larval value.

Let's think through this and address it as your larger fix for https://github.com/openjdk/valhalla/pull/1405

Crashing is an acceptable outcome when you are dealing with Unsafe. Using Unsafe means you are lying to the VM about the >> actual thing you are doing and have to rely on your own verification to make sure the program is valid. If the program using >> Unsafe is invalid (e.g. writing arbitrary value to a reference field), there is nothing the VM can do.

Crash occurs at the state merge point. If you run following test with C1 compiler it works fine. So we need to address this crash in C2 compiler. https://github.com/user-attachments/files/19501609/unsafe_access.txt

CPROMPT>java --enable-preview --add-exports=java.base/jdk.internal.misc=ALL-UNNAMED -XX:TieredStopAtLevel=3 -cp . unsafe_access 10001399990

For now I am ok with your fix, please also emit the IR to set larval bit in mark word during allocation expansion.

Mar 30 '25 07:03 jatin-bhateja

Bytecode verification should be same for larval and non-larval objects, you are mixing explicit and implicit larval here which is where the confusion arises.

I have addressed this point above: "Unsafe lets you bypass the restriction that final fields can only be set inside the constructor of its holder class. To achieve this, it lies to the bytecode verifier that you are having a value of type MyValue when in reality the returned value of Unsafe::makePrivateBuffer is a value of type uninitialized. However, you still must adhere to the rules that are put on the values of type uninitialized". Bytecode verification being the same for Unsafe::makePrivateBuffer and non-larval objects is due to the fact that you are lying to the bytecode verifier when using Unsafe, not because they are fundamentally the same. Explicit and implicit larval objects should be fundamentally the same.

Crash occurs at the state merge point. If you run following test with C1 compiler it works fine. So we need to address this crash in C2 compiler.

Undefined behaviour means that any behaviour is an acceptable behaviour. C1 can execute just fine and it is an acceptable outcome, C2 crashes and it is also an acceptable outcome.

For now I am ok with your fix, please also emit the IR to set larval bit in mark word during allocation expansion.

I believe setting AllocateNode._larval to true will make the expansion include setting the larval bit.

Mar 30 '25 08:03 merykitty

Undefined behaviour means that any behaviour is an acceptable behaviour. C1 can execute just fine and it is an acceptable outcome, C2 crashes and it is also an acceptable outcome.

From user standpoint be it C1 or C2 compiler, JVM needs to be consistent w.r.t to undefined behavior and crash is not acceptable.

I believe setting AllocateNode._larval to true will make the expansion include setting the larval bit.

Yes, AllocateNode::make_ideal_mark handles this.

No, it is wrong, from the verification type system, the highest reference type in the type hierarchy is reference, which comprises java.lang.Object and larval objects (uninitialized). Larval objects have their types being subtypes of uninitialized and non-larval objects have their types being subtypes of java.lang.Object. These 2 are separate under the bytecode verifier.

I see your point , but the contention here is that value types have a default value see https://github.com/openjdk/valhalla/blob/lworld/src/hotspot/share/oops/instanceKlass.cpp#L1459

So, technically they don't qualify as uninitialized objects.

Mar 30 '25 13:03 jatin-bhateja

Not asking to address in this patch, but what if we return a larval value to caller ? i.e. value returned by makePrivateBuffer is returned from a method since the return type will still be a concrete value type; hence, the caller will expect to receive the arguments in scalarized form. https://github.com/openjdk/valhalla/blob/lworld/src/hotspot/share/opto/graphKit.cpp#L2009

Mar 30 '25 13:03 jatin-bhateja

@jatin-bhateja FYI the default value of a value class is created in a previous round of experiment when we have to construct a value object with aconst_init and withfield. With the introduction of larval objects the notion of default value is no longer necessary and is in progress of removal (#1389). Additionally, all objects value or not are created at the bytecode new with their default value and they are still uninitialized.

Not asking to address in this patch, but what if we return a larval value to caller?

uninitialized is not a valid operand for areturn so a normal larval object cannot be returned from a method. As a result, returning an object created by Unsafe::makePrivateBuffer should also not allowed to be returned without invoking Unsafe::finishPrivateBuffer first.

Mar 30 '25 15:03 merykitty

From user standpoint be it C1 or C2 compiler, JVM needs to be consistent w.r.t to undefined behavior and crash is not acceptable.

Using Unsafe is inherently unsafe and we cannot have any promise regarding the behaviour of a program. For example, this will crash the VM:

UNSAFE.getInt(null, 0);

Worse, consider this program, given v0 == v1 and v0.v == 0:

static class Int {
    int v;
}

void test(int[] array, Int v0, int v1) {
    int x = v0.v;
    Unsafe.putInt(array, someOffset, 1);
    int y = v1.v;
    int z = v0.v;
}

Now if array + someOffset somehow points to v0.v, we will see that 3 consecutive loads from the same memory location return impossible results with x == z == 0 and y == 1.

Mar 30 '25 15:03 merykitty

@jatin-bhateja FYI the default value of a value class is created in a previous round of experiment when we have to construct a value object with aconst_init and withfield. With the introduction of larval objects the notion of default value is no longer necessary and is in progress of removal (#1389). Additionally, all objects value or not are created at the bytecode new with their default value and they are still uninitialized.

With the new scheme, we no longer use the factory constructor, and construction is aligned with the identity instance. @ImplicitlyConstructible annotations can still be used to enforces default value initialization

Refer to the following comment in instanceKlass.hpp

// Query if this class can be implicitly constructed, meaning the VM is allowed // to create instances without calling a constructor // Applies to inline classes and their super types

https://github.com/openjdk/valhalla/blob/lworld/src/hotspot/share/oops/instanceKlass.hpp#L423

Just noticed that https://github.com/openjdk/valhalla/pull/1389 is in draft state.

Not asking to address in this patch, but what if we return a larval value to the caller?

uninitialized is not a valid operand for areturn so a normal larval object cannot be returned from a method. As a result, returning an object created by Unsafe::makePrivateBuffer should also not allowed to be returned without invoking Unsafe::finishPrivateBuffer first.

We need to enforce these semantics in C2, I feel that a better place to document these is https://bugs.openjdk.org/browse/JDK-8046159

All execution engines must then adhere to clearly spelled semantics.

Similar to "areturn" , comparing uninitialized values using "acmp" is also undefined.

Mar 30 '25 22:03 jatin-bhateja

We need to enforce these semantics in C2

Sure, I think adding best-effort mechanisms to catch undefined behaviour would be appreciated. We would also need to spell out the restrictions more clearly in the javadoc of Unsafe Valhalla intrinsics. For this PR, do you have any other concern? Thanks a lot.

Mar 31 '25 03:03 merykitty

I filed https://bugs.openjdk.org/browse/JDK-8353236 for improving the javadoc and I will file another issue later to try our best catching the illegal uses in C2.

Mar 31 '25 04:03 merykitty

Hi @merykitty , Thanks for clearly spelling out the UNSAFE API semantics it chops out ambiguities about context in which larval values can be used.

The approach looks correct to me and aligns with larval handling in lworld+vector.

I see you have not addressed some minor comments around comments reformatting, Should we include the crash test in this PR and add it to ProblemList.txt for now?

https://github.com/user-attachments/files/19501609/unsafe_access.txt

Best Regards, Jatin

Apr 01 '25 05:04 jatin-bhateja

I have added a test that uses Unsafe in a proper way and crashes.

Apr 01 '25 17:04 merykitty

Thank @jatin-bhateja for the reviews and profound discussion.

@TobiHartmann Do I need to wait for you to run some tests?

Apr 02 '25 06:04 merykitty

Thanks for asking, @merykitty. I'll run some testing and will report back.

Apr 02 '25 13:04 TobiHartmann