Integrated: 8324517: C2: crash in compiled code because of dependency on removed range check CastIIs

[Roland Westrelin]

On Tue, 19 Mar 2024 13:21:49 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> Range check `CastII` nodes are removed once loop opts are over. The
> test case for this change includes 3 cases where elimination of a
> range check `CastII` causes a crash in compiled code because either a
> out of bounds array load or a division by zero happen.
> 
> In `test1`:
> 
> - the range checks for the `array[otherArray.length]` loads constant
>   fold: `otherArray.length` is a `CastII` of i at the `otherArray`
>   allocation. `i` is less than 9. The `CastII` at the allocation
>   narrows the type down further to `[0-9]`.
>   
> - the `array[otherArray.length]` loads are control dependent on the
> unrelated:
> 
> 
> if (flag == 0) {
> 
> 
> test. There's an identical dominating test which replaces that one. As
> a consequence, the `array[otherArray.length]` loads become control
> dependent on the dominating test.
> 
> - The `CastII` nodes at the `otherArray` allocations are replaced by a
>   dominating range check `CastII` nodes for:
>   
> 
> newArray[i] = 42;
> 
> 
> - After loop opts, the range check `CastII` nodes are removed and the
>   2 `array[otherArray.length]` loads common at the first:
>   
> 
> if (flag == 0) {
> 
> 
> test before the:
> 
> 
> float[] otherArray = new float[i];
> 
> 
> and
> 
> 
> newArray[i] = 42;
> 
> 
> that guarantee `i` is positive.
> 
> - `test1` is called with `i = -1`, the array load proceeds with an out
>   of bounds index and the crash occurs.
>   
>   
> `test2` and `test3` are mostly identical except for the check that's
> eliminated (a null divisor check) and the instruction that causes a
> fault (an integer division).
>   
> The fix I propose is to not eliminate range check `CastII` nodes after
> loop opts. When range check`CastII` nodes were introduced, performance
> was observed to regress. Removing them after loop opts was found to
> preserve both correctness and performance. Today, the performance
> regression still exists when `CastII` nodes are left in. So I propose
> we keep them until the end of optimizations (so the 2 array loads
> above don't lose a dependency and wrongly common) but remove them at
> the end of all optimizations.
> 
> In the case of the array loads, they are dependent on a range check
> for another array through a range check `CastII` and we must not lose
> that dependency otherwise the array loads could float above the range
> check at gcm time. I propose we deal with that problem the way it's
> handled for `CastPP` nodes: add the dependency to the load (or
> division)nodes as a precedence edge when the cast is removed.
> 
> @TobiHartmann ran performance testing for that patch (Thanks!) and reported
> no regression.

This pull request has now been integrated.

Changeset: ab8d7b0c
Author:    Roland Westrelin <roland at openjdk.org>
URL:       https://git.openjdk.org/jdk/commit/ab8d7b0cedfaae124262325cd1d4b59cef996d85
Stats:     562 lines in 6 files changed: 536 ins; 23 del; 3 mod

8324517: C2: crash in compiled code because of dependency on removed range check CastIIs

Reviewed-by: epeter, thartmann

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

PR: https://git.openjdk.org/jdk/pull/18377