[lworld] RFR: 8348607: [lworld] Calls to Unsafe.getValue() / Unsafe.putValue() must be updated [v2]

[Roger Riggs]

On Wed, 29 Jan 2025 21:11:16 GMT, Chen Liang <liach at openjdk.org> wrote:

>> Core libraries counterpart of #1336.
>> 
>> This requires MemberName to fetch more information in initialization. A new int high flag field is added, in anticipation to a merger with the regular int flag field in the future. Currently, directly migrating the flags field to long is too impactful to the whole codebase.
>> 
>> Also 8348680 is addressed as well; it's some minor updates to make the null checks specific to references VH.
>> 
>> Testing: test/jdk/java/lang/invoke
>
> Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:
> 
>  - Completely trust the value stored in fields (checked by verifier)
>  - Merge branch 'lworld' of https://github.com/openjdk/valhalla into fix/vh-layout
>  - Fix TestIntrinsics, safety remarks
>  - Fix DMH too
>  - Remove redundant checks for missing locations
>  - New field in MemberName for now
>  - Adapt layouts stage 1 (need to store in MemberName)

src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java line 118:

> 116:             } else {
> 117:                 long offset = MethodHandleNatives.objectFieldOffset(member);
> 118:                 int layout = member.isFlat() ? member.getLayout() : 0;

It would be cleaner to define layout == 0 as flat, so separate tests were not necessary.
The layout field should always have a value.

src/java.base/share/classes/java/lang/invoke/MemberName.java line 450:

> 448:     /**
> 449:      * VM-internal layout code for this field {@link jdk.internal.misc.Unsafe#fieldLayout(Field)}.
> 450:      * Only meaningful if the field {@link #isFlat()}, otherwise this may be garbage.

Can this be improved to have layout == 0 if !isFlat?
It would be cleaner and allow fewer tests in the code.
It might even be able to save a bit by testing the `highFlags & MN_HIGHT_LAYOUT_MASK == 0` instead of isFlat.
And Yes, I know this comes from the VM changes, it would need to be pushed back to the vm.

src/java.base/share/classes/java/lang/invoke/X-VarHandle.java.template line 52:

> 50:         final CheckedType checkedFieldType;
> 51: #end[Object]
> 52: #if[Value]

Can this parameter "Value" be changed to "FlatValue" to be consistent with the other method and variable names supporting flat values.

src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 246:

> 244:      * or layout control with this value, it is just an opaque token
> 245:      * used for performance reasons.  This value may be garbage if this field
> 246:      * is not {@link #isFlatField}.

1. Other code implicitly assumes this is only 3 bits. Somewhere there should be an assert checking that assumption.
2. We should request that the VM has a default value so it is never garbage.
3. The test to know whether the value is valid should be documented here.
4. The test for null seems redundant with the check in the VM's Unsafe_FieldLayout implementation.
5. Please fix the indentation on the `return` below.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestIntrinsics.java line 1625:

> 1623:         Field field = Test80Value1.class.getDeclaredField("v");
> 1624:         var flat = U.isFlatField(field);
> 1625:         Asserts.assertEQ(test80(v, flat, flat ? U.fieldLayout(field) : 0, U.objectFieldOffset(field)), v.v);

This is a good example showing why fieldLayout() should return a known value even if not flat.

-------------

PR Review Comment: https://git.openjdk.org/valhalla/pull/1340#discussion_r1935825887
PR Review Comment: https://git.openjdk.org/valhalla/pull/1340#discussion_r1935830967
PR Review Comment: https://git.openjdk.org/valhalla/pull/1340#discussion_r1935812096
PR Review Comment: https://git.openjdk.org/valhalla/pull/1340#discussion_r1935853016
PR Review Comment: https://git.openjdk.org/valhalla/pull/1340#discussion_r1936030768