RFR: 8328702: C2: Crash during parsing because sub type check is not folded

[Roland Westrelin]

On Wed, 27 Mar 2024 13:48:09 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> The test case shows a problem where data is folded during parsing while control is not. This leaves the graph in a broken state and we fail with an assertion.
> 
> We have the following (pseudo) code for some class `X`:
> 
> o = flag ? new Object[] : new byte[]; 
> if (o instanceof X) {
>    X x = (X)o; // checkcast
> }
> 
> For the `checkcast`, C2 knows that the type of `o` is some kind of array, i.e. type `[bottom`. But this cannot be a sub type of `X`. Therefore, the `CheckCastPP` node created for the `checkcast` result is replaced by top by the type system. However, the `SubTypeCheckNode` for the `checkcast` is not folded and the graph is broken.
> 
> The problem of not folding the `SubTypeCheckNode` can be traced back to `SubTypeCheckNode::sub` calling `static_subtype_check()` when transforming the node after it's creation. `static_subtype_check()` should detect that the sub type check is always wrong here:
> https://github.com/openjdk/jdk/blob/d0a265039a36292d87b249af0e8977982e5acc7b/src/hotspot/share/opto/compile.cpp#L4454-L4460
> 
> But it does not because these two checks return the following:
> 1. Check: is `o` a sub type of `X`? -> returns no, so far so good.
> 2. Check: _could_ `o` be a sub type of `X`? -> returns no which is wrong! `[bottom` is only a sub type of `Object` and can never be a subtype of `X`
> 
> In `maybe_java_subtype_of_helper_for_arr()`, we wrongly conclude that any array with a base element type `bottom` _could_ be a sub type of anything:
> https://github.com/openjdk/jdk/blob/d0a265039a36292d87b249af0e8977982e5acc7b/src/hotspot/share/opto/type.cpp#L6462-L6465
> But this is only true if the super class is also an array class - but not if `other` (super klass) is an instance klass as in this case.
> 
> The fix for this is to first check the immediately following check which handles the case of comparing an array klass to an instance klass: An array klass can only ever be a sub class of an instance klass if it's the `Object` class. But in our case, we have `X` and this would return false:
> 
> https://github.com/openjdk/jdk/blob/d0a265039a36292d87b249af0e8977982e5acc7b/src/hotspot/share/opto/type.cpp#L6466-L6468
> 
> The very same problem can also be triggered with `X` being an interface instead. There are tests for both these cases.
> 
> #### Additionally Required Fix
> When running with `-XX:+ExpandSubTypeCheckAtParseTime`, we eagerly expand the sub type check during parsing and therefore do not emit a `SubTypeCheckNode`. When additionally running with `-XX:+StressReflectiveCode`, th...

Looks good to me.

> When running with -XX:+ExpandSubTypeCheckAtParseTime

Do we want to retire `ExpandSubTypeCheckAtParseTime`? Is there any reason to keep it?

src/hotspot/share/opto/type.cpp line 6465:

> 6463:   }
> 6464:   if (this_one->is_instance_type(other)) {
> 6465:     return other->klass()->equals(ciEnv::current()->Object_klass()) && other->_interfaces->intersection_with(this_one->_interfaces)->eq(other->_interfaces);

`TypeInterfaces` has a `contains` method that does `intersection_with` + `eq`. Could we use it here? i.e. `this_one->_interfaces->contains(other->_interfaces)`

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

Marked as reviewed by roland (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18512#pullrequestreview-1963542605
PR Comment: https://git.openjdk.org/jdk/pull/18512#issuecomment-2022999693
PR Review Comment: https://git.openjdk.org/jdk/pull/18512#discussion_r1541245665