[lworld] RFR: 8351550: [lworld] Some bootstrap methods/lambda factories fail with -XX:+UseNullableValueFlattening [v2]

[John R Rose]

On Wed, 12 Mar 2025 19:36:36 GMT, Frederic Parain <fparain at openjdk.org> wrote:

>> Fix handling of offsets for boxing classes.
>> Thanks to Chen Liang and John Rose for their help identifying the root cause of the problem.
>> 
>> Fred
>
> Frederic Parain 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 two additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'upstream/lworld' into 8351550
>  - Fix handling of offsets for boxing classes

Good.  As it stands it needs some cleanup when Valhalla is integrated.
Put a comment to that effect, something like "when we exit preview,
this code must be simplified."

I suggested some name changes.

I also suggested a deeper cut, a more robust fix that would remember the offset separately for each `T_FOO` type.  I would prefer that, but I won't insist, since this code is going to be reworked again.

On the other hand, why not fix it in such a way, using a table indexed by `T_FOO`, that no further rework will be necessary?  It may be that with a full table-based approach, you won't need to look at the preview flag.

src/hotspot/share/classfile/javaClasses.cpp line 3990:

> 3988: }
> 3989: 
> 3990: int java_lang_boxing_object::_sub32bits_value_offset;

An existing term of art in our code for `sub32bits` is "subword".
The `_32` and `_64` are distracting; suggest simply `int` and `long`.
That combined with `subword` will make clear enough what is happening.
Suggestion: `s/sub32bits/subword/; s/32bits/int/; s/64bits/long/`

Also, are we sure the subwords will always be all the same, and the floats and non-floats will correspond?  This logic assumes that.  I suppose the adjusted asserts will catch any future problems.

A fuller fix would be to make a local array indexed by `T_FOO` code, as the VM has in many other places.   (The length is `T_VOID`.)  Then each offset could be stored in its own place, customized to each `T_FOO` type.  Why not move all the way there, and forget about these ad hoc classifications?

(But did the existing asserts catch this problem in the first place?  If not why not?)

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

PR Review: https://git.openjdk.org/valhalla/pull/1396#pullrequestreview-2680090553
PR Review Comment: https://git.openjdk.org/valhalla/pull/1396#discussion_r1992436070