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

[Christian Hagedorn]

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...

Thanks Roland for your review!

> > When running with -XX:+ExpandSubTypeCheckAtParseTime
> 
> Do we want to retire `ExpandSubTypeCheckAtParseTime`? Is there any reason to keep it?

I'm not sure about how much benefit it gives us. A quick JBS search for "ExpandSubTypeCheckAtParseTime" revealed a few issues - but would need to double check how many of them really only triggered with that flag and were real bugs. So, apart from having it as a stress option, I don't see a real benefit for it - but that might be a good enough reason to keep it for now.

What do you think?

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

PR Review: https://git.openjdk.org/jdk/pull/18512#pullrequestreview-1963696677