[lworld] RFR: 8357785: [lworld] TestResolvedJavaType fails due to unexpected getInstanceFields order [v5]

[Tobias Hartmann]

On Fri, 25 Jul 2025 12:09:13 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

>> It's all a matter of order. In short, mainline doesn't sort objects fields by offset, but just take them as they come from the stream. The order can be arbitrary, but in some places, it matters it's the same everywhere. Valhalla is sorting too much for mainline's taste, a merge caused clashes.
>> 
>> But maybe, a bit of archeology:
>> - Mainline [JDK-8350892: [JVMCI] Align ResolvedJavaType.getInstanceFields with Class.getDeclaredFields](https://bugs.openjdk.org/browse/JDK-8350892): it removed some sorting of fields, keeping it consistent between `reassign_fields_by_klass` and `ciInstanceKlass::compute_nonstatic_fields()`: they both had fields offset-sorted, now they are both stream-sorted. Most of HotSpot agrees with this new order. This change also add a new test checking the order is consistent: `compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java`.
>> 
>> - Valhalla: [#1429](https://github.com/openjdk/valhalla/pull/1429) brings [JDK-8350892](https://bugs.openjdk.org/browse/JDK-8350892) in Valhalla. This merge is a bit mysterious to me,  how some sorting reappeared in Valhalla-untouched code. For instance, [JDK-8350892](https://bugs.openjdk.org/browse/JDK-8350892) removed the function `
>> static int sort_field_by_offset(ciField** a, ciField** b)` and
>>   ```cpp
>>   // Now sort them by offset, 
>>   // (In principle, they could mix with superclass fields.)
>>   fields->sort(sort_field_by_offset);
>>   ```
>>   from `int ciInstanceKlass::compute_nonstatic_fields()` (in `ciInstanceKlass.cpp`), but the merge only removes the comment, but leave the `->sort()` and `sort_field_by_offset`. My guess is that it was a tentative fix for some failing tests after merge. Nevertheless `compiler/jvmci/jdk.vm.ci.code.test/src/jdk/vm/ci/code/test/VirtualObjectDebugInfoTest.java` is ProblemListed.
>> 
>> - Valhalla: After this merge, Valhalla continues to assume some ordering, as one can see in [JDK-8355076: [lworld] Incorrect declared_nonstatic_fields computation](https://bugs.openjdk.org/browse/JDK-8355076) that (among other things), makes a new occurrence of `->sort(sort_field_by_offset)`, to be consistent with `InlineKlass::collect_fields`. But now, everything is different! The new test introduced by [JDK-8350892](https://bugs.openjdk.org/browse/JDK-8350892) connects more things together, and enforce the same order in `ciInstanceKlass::compute_nonstatic_fields` and in `Class.getDeclaredFields` (reflexion related, I think. I don't know that ...
>
> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix get_non_flat_field_by_offset

> Unfortunately, I don't think this PR solves [JDK-8348547](https://bugs.openjdk.org/browse/JDK-8348547). It solves [JDK-8357186](https://bugs.openjdk.org/browse/JDK-8357186), though.

Ah, makes sense. Thanks for clarifying.

Looks good.

src/hotspot/share/runtime/sharedRuntime.cpp line 2868:

> 2866:             _sig_cc_ro->appendAll(vk->extended_sig());
> 2867:             if (bt == T_OBJECT) {
> 2868:               // Nullable inline type argument, insert InlineTypeNode::NullMarker field right after T_METADATA delimite

Suggestion:

              // Nullable inline type argument, insert InlineTypeNode::NullMarker field right after T_METADATA delimiter

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

Marked as reviewed by thartmann (Committer).

PR Review: https://git.openjdk.org/valhalla/pull/1511#pullrequestreview-3055277484
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2231003150