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

[Marc Chevalier]

On Sun, 20 Jul 2025 07:45:44 GMT, Quan Anh Mai <qamai 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 ...
>
> src/hotspot/share/runtime/signature.hpp line 591:
> 
>> 589:     : _bt(bt), _offset(offset), _symbol(symbol), _null_marker(null_marker) {}
>> 590: 
>> 591: #if 0
> 
> Why do you wrap it with `#if 0` instead of removing this function?

It's a leftover that survived the clean up, it should have been removed. I was trying to remove some sorting by removing the order function, and see if it's used somewhere legitimate, but this way it was still easily accessible. Anyway, it should have been, and will be, removed.

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2218253200