8380902: [lworld] Type confusion in jvm.cpp functions by stefank · Pull Request #2261 · openjdk/valhalla

stefank · 2026-03-25T14:10:55Z

These functions cast oops to arrayOop without first checking that the obj argument is an array. It turns out that TestIntrinsics.java sends in non-array objects.

I found this when I worked on something else and changed obj->is_null_free_array() to obj->klass()->is_null_free_array_klass(), and the latter asserted because obj was a String and not an array.

Edit: I've now reworked the patch so that the Java APIs now only accept Object[] arrays and then put in asserts in the code that check that this is the case. I've also removed the parts of the tests that passes in Strings and int arrays.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

JDK-8380902: [lworld] Type confusion in jvm.cpp functions (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/2261/head:pull/2261
$ git checkout pull/2261

Update a local copy of the PR:
$ git checkout pull/2261
$ git pull https://git.openjdk.org/valhalla.git pull/2261/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2261

View PR using the GUI difftool:
$ git pr show -t 2261

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2261.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-03-25T14:13:03Z

👋 Welcome back stefank! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2026-03-25T14:13:06Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

mlbridge · 2026-03-25T14:16:23Z

Webrevs

01: Full - Incremental (8406165e)
00: Full (36ebcbbd)

dholmes-ora · 2026-03-25T23:58:28Z

JVM_* functions should be able to rely on being passed the correct arguments from the (trusted) Java caller. If a test doesn't do this then the bug is in the test IMO.

stefank · 2026-03-26T21:07:50Z

During an internal discussion we decided to change the code to take use Object[] as the array parameter. The test code was changed accordingly.

stefank · 2026-03-26T21:08:31Z

src/java.base/share/classes/java/lang/invoke/ArrayVarHandle.java

        Object[] array = (Object[]) handle.arrayType.cast(oarray);
        Class<?> arrayType = oarray.getClass();
-        if (ValueClass.isFlatArray(oarray)) {
+        if (ValueClass.isFlatArray(array)) {


I'd like to get extra scrutiny of this change.

What is your concern?

My concern is that I don't fully understand why the code the way it does, and it looks a little bit funny.

For example:

Object[] array = (Object[]) handle.arrayType.cast(oarray); Class<?> arrayType = oarray.getClass(); if (ValueClass.isFlatArray(array)) {

It performs a cast to get an array reference, but then it uses the oarray reference to fetch the arrayType (and the same for the isFlatArray call that I'm changing).

I would think that the could look like this:

Object[] array = (Object[]) handle.arrayType.cast(oarray); Class<?> arrayType = array.getClass(); if (ValueClass.isFlatArray(array)) {

but I'm not sure if that is missing some very subtle detail that is important for the code.

dholmes-ora

That was a lot of effort but I think the signatures make more sense now.

I have concerns about the main jvm.cpp changes though.

dholmes-ora · 2026-03-26T21:45:55Z

src/hotspot/share/prims/jvm.cpp

+JVM_ENTRY(jboolean, JVM_IsFlatArray(JNIEnv *env, jarray array))
+  oop o = JNIHandles::resolve_non_null(array);
+  Klass* klass = o->klass();
+
+  return klass->is_flatArray_klass();


Given the definition:

#ifdef _LP64 bool oopDesc::is_flatArray() const { markWord mrk = mark(); return (mrk.is_unlocked()) ? mrk.is_flat_array() : klass()->is_flatArray_klass(); }

I think this needs to retain the original form and not call is_flatArray_klass directly.

I don't think it does. The mark word is just another way to query the information that we get from by querying the class.

In fact, I have a prototype that completely removes these functions from the mark word and let all C++ code go through the Klass pointer. This cuts down on the complexity and some of the risks of those bits being unstable while inflating object monitors. Left are the JIT compilers using those bits, and having to navigate around the locking code.

If the markword logic is going away then sure. I have to query why we have/had two ways of querying this though? Optimization?

My understanding is that this was implemented as optimizations, but things get complicated because of object monitors and displaced mark words, and the optimizations bring conditional branches. I've looked through the C++ code and can't see anything where these optimizations seems to be necessary compared to the overhead of the surrounding code. C2 still uses these mark word bits for creating optimized code.

My wish is to make it so that the oopDesc::is_ functions call into obj->klass()->is_ functions. If we ever find a place in the C++ code where we get noticeably penalized for decoding the klass pointer, then we place a localized mark-word check optimization there. And then we evaluate if that optimization really gives us the sought after performance benefit that we hoped to get.

IIRC, the flat_array_bits in the markword is there to help minimizing the performance hit on code using good old Java arrays (non-flat). Because of array covariance, a runtime check is needed to test if the array is flat or not when the static type of the array is Object[], an array of interfaces, or an array of an abstract value class. In some situations, JITs are able to collect more information about the array real type, and can skip this check, but in many cases, this check cannot be elided. It was important to have a very fast check to minimize the impact, especially on legacy code not using value types.
The problem with having a bit in the markword, as explain by Stefan, is that the bit is not always present. The other ways the check can be performed are 1) to check the _kind field of the ArrayKlass by calling klass()->is_flatArray_klass() or 2) check the layout helper with layout_helper_is_flatArray(klass()->layout_helper()).

dholmes-ora · 2026-03-27T00:29:27Z

src/hotspot/share/prims/jvm.cpp

+JVM_ENTRY(jboolean, JVM_IsNullRestrictedArray(JNIEnv *env, jarray array))
+  oop o = JNIHandles::resolve_non_null(array);
+  Klass* klass = o->klass();
+
+  assert(klass->is_objArray_klass(), "Expects an object array");
+
+  return klass->is_null_free_array_klass();


Ditto - this needs to call oop->is_null_free_array.

dholmes-ora · 2026-03-27T00:30:34Z

src/hotspot/share/prims/jvm.cpp

  }
-  return false;
+
+  if (klass->is_flatArray_klass()) {


Again oop->is_flatArray.

8380902: [lworld] Type confusion in jvm.cpp functions

36ebcbb

openjdk bot added the rfr Pull request is ready for review label Mar 25, 2026

Change the functions to take an Object[] as the argument

8406165

stefank commented Mar 26, 2026

View reviewed changes

dholmes-ora suggested changes Mar 27, 2026

View reviewed changes

Conversation

stefank commented Mar 25, 2026 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Mar 25, 2026

Uh oh!

openjdk bot commented Mar 25, 2026

Uh oh!

mlbridge bot commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora commented Mar 25, 2026

Uh oh!

stefank commented Mar 26, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefank Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

stefank commented Mar 25, 2026 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 25, 2026 •

edited

Loading

stefank Mar 27, 2026 •

edited

Loading