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

Thu Jul 17 12:15:48 UTC 2025

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 so well). Moreover `ciInstanceKlass::compute_nonstatic_fields` has to agree with deoptimization code. In Valhalla, the calling convention has to agree with the layout wrt field ordering. Basically, it makes a big chain of things that needs to agree on the order of fields.

- Valhalla: [JDK-8354366: [lworld] VirtualObjectDebugInfoTest fails after merging jdk-25+16](https://bugs.openjdk.org/browse/JDK-8354366) partially reverts [JDK-8350892: [JVMCI] Align ResolvedJavaType.getInstanceFields with Class.getDeclaredFields](https://bugs.openjdk.org/browse/JDK-8350892) by reintroducing sorting. This allowed to un-ProblemList `compiler/jvmci/jdk.vm.ci.code.test/src/jdk/vm/ci/code/test/VirtualObjectDebugInfoTest.java`, but made `compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java` fail because now, and that's where I start.

Overall, it's a clash in the order of fields. First question is naturally: does the order really has to agree, or do we put some interfaces, pipes and curtains to adapt between potentially different order? I think we should keep a consistent order, as much as possible. It makes everything simpler, possibly more efficient as we don't have to search if we want to work on two lists... Efforts were made in mainline to keep the order consistent, let's not break that.

Second question is which order. Here, same, I'll invoke mainline that decided to go with stream order. It has also the benefit not to require sorting: get the field stream, ready to use. Lighter, easier, more efficient...

Let's define more precisely the order in which the fields should show up. Given a class `Derived`, that extends the class `Base`.
1. superclass first: for any transitive field `b` of `Base`, and any proper field `d` of `Derived`, `b` should appear before `d`,
2. consistent ordering: the list of (transitive) fields of `Base` must be a subsequence of the list of (transitive) fields of `Derived`. In other words, the order of the fields of `Base` in the list of fields of `Base` is the same as the order of the fields of `Base` in the list of fields of `Derived`.
3. proper field ordering: the proper fields of `Derived`, must appear in the order provided by `JavaFieldStream` (which happens to be the declaration order, but is not guaranteed).

This is enough to define the list of declared fields. It implies in particular that the fields of the super class form a prefix of the whole list of declared fields. It makes possible to isolate the proper fields by removing this prefix (and it's done somewhere). It also is cheap to compute as we can just push fields in the order the stream gives, starting with a recursive call to traverse the supertype's fields.

If we are interested in the list of actual fields, as present in the layout, we simply insert the fields of each flatten field instead of them. The null marker of each field, if it exists, appears immediately after.

Which changes need to be made? Mostly removing sorting
- remove sorting in `ciInstanceKlass::compute_nonstatic_fields_impl`.
- remove member variable `float _sort_offset` from `SigEntry` but add `bool _null_marker` for printing in `nmethod::print_nmethod_labels`.
- remove sorting from `InlineKlass::collect_fields`, but also, change the iterator: use the new `TopDownHierarchicalFieldStreamBase` that will travese the super classes first.
- remove sorting from `get_reassigned_fields`, seems to be a merge accident? The code around comes from [JDK-8350892](https://bugs.openjdk.org/browse/JDK-8350892) without the sort, but [#1429](https://github.com/openjdk/valhalla/pull/1429) introduced sorting.
- revert [JDK-8354366: [lworld] VirtualObjectDebugInfoTest fails after merging jdk-25+16](https://bugs.openjdk.org/browse/JDK-8354366), bringing back the changes of [JDK-8350892: [JVMCI] Align ResolvedJavaType.getInstanceFields with Class.getDeclaredFields](https://bugs.openjdk.org/browse/JDK-8350892).

Bonus:
- Added a printer for `VirtualObjectDebugInfoTest$TestClass` because the test failure message being "this `TestClass` is not equal to this other `TestClass`" is not so helpful.
- Added a flag to print fields collected in `InlineKlass::initialize_calling_convention`. It seems it was deemed useful but missing, and not only by me right now! The flag is called `PrintInlineKlassFields` and takes a comma-separated list of class name patterns to print. Feel free to suggest a better name.
- I've tried to print fields in `ciInstanceKlass::compute_nonstatic_fields` to compare. It works, but this function is called way too many times for each class (not sure why, didn't look into that), so not great to leave a flag for that.

Wow, what a wall of text. But for once, it comes with non-trivial changes.

Thanks,
Marc

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

Commit messages:
 - Merge remote-tracking branch 'origin/lworld' into fix/field-ordering
 - cleanup
 - UnProblemList
 - For testing

Changes: https://git.openjdk.org/valhalla/pull/1511/files
  Webrev: https://webrevs.openjdk.org/?repo=valhalla&pr=1511&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8357785
  Stats: 239 lines in 13 files changed: 138 ins; 60 del; 41 mod
  Patch: https://git.openjdk.org/valhalla/pull/1511.diff
  Fetch: git fetch https://git.openjdk.org/valhalla.git pull/1511/head:pull/1511

PR: https://git.openjdk.org/valhalla/pull/1511