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

Sun Jul 20 07:50:57 UTC 2025

On Thu, 17 Jul 2025 12:07:56 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 so well). Moreover `ciInstance...

src/hotspot/share/ci/ciInstanceKlass.cpp line 474:

> 472: }
> 473: 
> 474: static int sort_field_by_offset(ciField** a, ciField** b) {

Could you add a comment at `ciInstanceKlass._declared_nonstatic_fields` and `_nonstatic_fields` to specify exactly the order of the fields there? It is a little bit hard to grasp at the first sight, especially with flattened fields.

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

> 194: };
> 195: 
> 196: template<typename FieldStreamType>

It would be good to say what this class does and what the template parameter is for.

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

> 323:  * Doesn't traverse interfaces for now (let's decide how when/if the needs appear).
> 324:  */
> 325: template<typename FieldStreamType>

By "doesn't traverse interfaces for now" I assume this traverses both static and non-static fields. Tbh I think traversing static fields seems questionable, or maybe I miss some cases, maybe this stream can discard those, too?

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

> 434:   }
> 435:   SigEntry::add_entry(sig, T_VOID, name(), offset);
> 436:   if (base_off == 0) {

Nice, I have a pending issue of making this sort less a minefield but removing it completely is absolutely better. The comment of this function says it sorts the flattened fields in the increasing offset order, please change it, too.

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?

test/hotspot/jtreg/ProblemList.txt line 135:

> 133: runtime/valhalla/inlinetypes/verifier/StrictInstanceFieldsTest.java CODETOOLS-7904031 generic-all
> 134: runtime/valhalla/inlinetypes/verifier/StrictStaticFieldsTest.java CODETOOLS-7904031 generic-all
> 135: runtime/cds/appcds/RewriteBytecodesInlineTest.java 8361082 generic-all

This issue is fixed with #1498 , so this seems like a merge conflict, right?

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2217682443
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2217682958
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2217683719
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2217684836
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2217685460
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2217686046