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

Fri Jul 25 08:26:09 UTC 2025

On Thu, 24 Jul 2025 11:38:44 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:
> 
>   Second round of comments

Thanks for the thorough analysis Marc! I'm glad to see this code being fixed and cleaned up. Looks all good to me, I just found a few minor things.

> Nice, I have a pending issue of making this sort less a minefield but removing it completely is absolutely better.

@merykitty Should we close [JDK-8348547](https://bugs.openjdk.org/browse/JDK-8348547) as duplicate then? If so, I would suggest to integrate the regression tests attached to this bug with this PR. @marc-chevalier you might want to verify that they pass now.

src/hotspot/share/code/nmethod.cpp line 3846:

> 3844:       if (!did_name)
> 3845:         stream->print("%s", type2name(t));
> 3846:       // If the entry has a non-default sort_offset, it must be a null marker

Comment needs to be updated.

src/hotspot/share/oops/fieldStreams.hpp line 336:

> 334: /* Iterates on the fields of a class and its super-class top-down (java.lang.Object first)
> 335:  * Doesn't traverse interfaces for now, because it's not clear which order would make sense
> 336:  * Let's decide how  when/if the needs appear. Since we are not traversing interfaces, we

Suggestion:

 * Let's decide when or if the need arises. Since we are not traversing interfaces, we

src/hotspot/share/oops/inlineKlass.cpp line 447:

> 445:     if (*PrintInlineKlassFields != '\0') {
> 446:       const char* class_name_str = _name->as_C_string();
> 447:       if (StringUtils::class_list_match(PrintInlineKlassFields, class_name_str)) {

Nice! I already added similar printing multiple times when debugging issues in this area. Not sure why I never integrated that.

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

> 2867:             if (bt == T_OBJECT) {
> 2868:               // Nullable inline type argument, insert InlineTypeNode::NullMarker field right after T_METADATA delimiter
> 2869:               // Set the sort_offset so that the field is detected as null marker by nmethod::print_nmethod_labels.

This comment needs to be updated. It still mentions the `sort_offset`.

src/hotspot/share/runtime/signature.hpp line 582:

> 580:   BasicType _bt;      // Basic type of the argument
> 581:   int _offset;        // Offset of the field in its value class holder for scalarized arguments (-1 otherwise). Used for packing and unpacking.
> 582:   float _sort_offset; // Offset used for sorting

I'm glad this is gone now. Always felt hacky.

src/hotspot/share/runtime/signature.hpp line 583:

> 581:   int _offset;        // Offset of the field in its value class holder for scalarized arguments (-1 otherwise). Used for packing and unpacking.
> 582:   Symbol* _name;    // Symbol for printing
> 583:   bool _null_marker;   // Is it a null marker? For printing

Suggestion:

  Symbol* _name;      // Symbol for printing
  bool _null_marker;  // Is it a null marker? For printing

test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.code.test/src/jdk/vm/ci/code/test/VirtualObjectDebugInfoTest.java line 89:

> 87:                     .append("]: ");
> 88:             for (int i = 0; i < arrayField.length; ++i) {
> 89:                 if(i != 0) {

Suggestion:

                if (i != 0) {

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

Marked as reviewed by thartmann (Committer).

PR Review: https://git.openjdk.org/valhalla/pull/1511#pullrequestreview-3054493576
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2230485359
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2230466436
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2230488247
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2230483994
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2230481700
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2230458927
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2230440942