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

[Chen Liang]

On Thu, 30 Jan 2025 15:37:53 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> 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/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.

Yep, removed the flat bit, and as a result removed the high flags in MemberName. Flat is now derived from the 3 layout bits all being 0.

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

Done.

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

Done, check added in VM code that fills MemberName. The only known-to-lib info now is that 0 is non-flat, other values are random flat values specific to VM.

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1340#discussion_r1936464164
PR Review Comment: https://git.openjdk.org/valhalla/pull/1340#discussion_r1936464217
PR Review Comment: https://git.openjdk.org/valhalla/pull/1340#discussion_r1936463015