8375441: [lworld] C2: assert(is_instance()) failed: bad cast

[marc-chevalier]

Some code added by JDK-8372700 can compute the constant value of a field of a (flatten) element in a flat array. We get a crash when the element of the array is known to be null, and so the field doesn't exist.

So, let's just check in ciConstant ciFlatArray::field_value(int index, ciField* field) whether we get a null constant before interpreting it as a ciInstance and trying to retrieve a field from there. This should be enough since a ciObject is (directly) derived by ciNullObject, ciInstance and ciArray. Since we are looking up a value of a flat array, an element cannot be a ciArray (arrays have identities and can't be contained in a flat array). After looking up whether the flat array element is null, the obj->as_instance() cast acts as an assert, should we ever add another derived class from ciObject.

In case of a null array element, field_value simply returns an invalid ciConstant.

Tested with tier1,tier2,tier3,hs-precheckin-comp,hs-comp-stress,valhalla-comp-stress. Looks good.

Thanks,
Marc

---

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-8375441: [lworld] C2: assert(is_instance()) failed: bad cast (Bug - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1923

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mchevalier! 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]

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

[mlbridge]

Webrevs

• 00: Full (f051e943)

[merykitty]

This is incorrect, the value is not constant if the element of the array is null, not if the field we retrieved is null.

[marc-chevalier]

I think you're correct. While being sound, we might miss opportunities where the value is actually constant but we discard it. con.is_value() already is enough to make sure the array element is not null (and constant).

[merykitty]

I don't really agree with this fix, ciFlatArray::field_value should be dumber, it is the caller who knows that we do not fold the load if the element is null, the callee should just return the field as it is.

[marc-chevalier]

I fear I don't understand. Let's say, I have a flat array MyValue[] arr where MyValue is a value class with a single field f. Let's also assume arr[0] == null, arr.field_value(0, f) (assuming the ci... versions of it with matching names) tries to get the constant value of the field f of arr[0], and arr[0].f is not null, it's rather undefined. It's not about stability and folding. On the other hand, if arr[0] is not null, but arr[0].f is null, arr.field_value(0, f) already returns null (the ciConstant that means that).

Am I missing something?

[merykitty]

That is only true from the Java perspective. From the VM perspective, a flat array would be something like (C++ pseudocode):

class MyValuePayload {
  oop* f;
  bool null_marker;
};

MyValuePayload* arr = new MyValuePayload[n];

Then, it is clear that even if arr[0].null_marker == false, arr[0].f is still defined and has a value (which should be nullptr).

[marc-chevalier]

As discussed offline, I'm trying change that.