RFR: 8275610: C2: Object field load floats above its null check resulting in a segfault
Christian Hagedorn
chagedorn at openjdk.java.net
Fri Dec 3 14:09:31 UTC 2021
In the test case, a field load of an object floats above the object null check due to a `CastPP` that gets separated from its null check `If` node. C2 then schedules the field load before the object null check which results in a segfault.
The problem can be traced back to the elimination of identical back-to-back ifs [(JDK-8143542)](https://bugs.openjdk.java.net/browse/JDK-8143542). This is done in split-if where we detect identical back-to-back ifs in `PhaseIdealLoop::identical_backtoback_ifs()`. We then replace the boolean node of the dominated `If` by a phi to use the split-if optimization to nicely fold away the dominated `If` later in IGVN. This, however, does not update any data nodes that were dependent on the dominated `If` projections.
In the test case, we have the following graph right before splitting `313 If` (`NC 2`) through `190 Region`:
`313 If` is dominated by the identical (= both share `308 Bool`) `309 If` (`NC 1`). The bool input for `313 If` is replaced by a phi and the split-if optimization is applied. However, the data nodes dependent on the out projections of the dominated `313 If` (`261 CastPP` in this case) are not processed separately and just end up at the newly inserted regions in split-if. In the test case, we get the following graph where `261 CastPP` ends up at the new `334 Region`:
Loopopts are applied and can remove the `325 CountedLoop` and we find that the `171 RangeCheck` (`RC 2`) is applied on a constant array index 1.
Now at IGVN, the order in which the nodes are processed is important in order to trigger the segfault:
1. `334 Region` with `332 If` and `333 If` are removed because of the special split-if setup we used to remove the identical `313 If`. The control input of `261 CastPP` is therefore updated to `172 IfTrue`.
2. Applying `RangeCheck::Ideal()` for `171 RangeCheck` finds that `305 RangeCheck` (`RC 1`) already covers it and we remove `171 RangeCheck`. In this process, we rewire `261 CastPP` to the dominating `305 RangeCheck` and we have the following graph:
`261 CastPP` - and also the field `263 LoadI` - have now `306 IfFalse` as early control. GCM is then scheduling `263 LoadI` before the null check `309 If` and we get a segfault.
An easy fix is not straight forward. What we actually would want to do is rewiring `261 CastPP` from `334 Region` to `311 IfTrue` in the second graph after split-if to not separate it from the null check. But that's not possible because we would create a bad graph: The early control `311 IfTrue` of `261 CastPP` does not dominate its late control further down after `334 Region` because of the not yet removed `334 Region`. We would need to already clean the regions up and then do the rewiring. But then the question arises why to use the split-if optimization in the first place when we do not want to rely on IGVN to clean it up.
I therefore suggest to go with an easy bailout fix for JDK 18 where we do not apply this identical back-to-back if removal optimization if there are data dependencies and rework this in an RFE for JDK 19. Roland already has some ideas how to do that.
I ran some standard benchmarks and did not see any performance regressions with this fix.
Thanks,
Christian
-------------
Commit messages:
- 8275610: C2: Object field load floats above its null check resulting in a segfault
Changes: https://git.openjdk.java.net/jdk/pull/6701/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6701&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8275610
Stats: 112 lines in 2 files changed: 112 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/6701.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/6701/head:pull/6701
PR: https://git.openjdk.java.net/jdk/pull/6701
More information about the hotspot-compiler-dev
mailing list