RFR: 8277529: SIGSEGV in C2 CompilerThread Node::rematerialize() compiling Packet::readUnsignedTrint [v2]

[Christian Hagedorn]

On Fri, 3 Dec 2021 08:59:47 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> This bug was found in internal testing for 17.0.2 after the backport of [JDK-8272574](https://bugs.openjdk.java.net/browse/JDK-8272574). The fix rewires a control input of a split load node through a phi to the same control input as the corresponding memory input into the phi node:
>> https://github.com/openjdk/jdk/blob/e002bfec8cb815b551c9b0f851a8a5b288e8360d/src/hotspot/share/opto/memnode.cpp#L1629-L1637
>> 
>> This can lead to an illegal control input for the new load if `mem->in(i)` is a projection. This situation occurs in the test case: `mem->in(i)` is a projection of an `ArrayCopy` node. Having an `ArrayCopy` node as control input is unexpected and we later fail with an assertion (in product we crash with a segfault).
>> 
>> My initial thought was to just not apply the improved rewiring for memory projections and fall back to the else case on L1636. However, this also does not work as we could then wrongly move a range check out of a loop and creating a cyclic dependency (case described in the [review of JDK-8272574](https://github.com/openjdk/jdk/pull/5142)) and reproduced with `test2()` where we hit:
>> https://github.com/openjdk/jdk/blob/e002bfec8cb815b551c9b0f851a8a5b288e8360d/src/hotspot/share/opto/loopPredicate.cpp#L777-L778
>> 
>> During this analysis I came across the [fix for JDK-8146792](http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/2748d975045f) which should actually prevent loop predication if we have a data dependency on the projection into a loop. But this does not seem to work correctly as shown in the test case found for JDK-8272574. In the review of JDK-8272574, it was missed that the actual problem could have been traced back to JDK-8146792 and instead we went for an improvement for loads split through phis to not end up with cases supposed to be fixed by JDK-8146972 (which now also causes other problems). But the root problem is still there to be fixed.
>> 
>> I therefore propose to completely undo the fix for JDK-8272574 for now and go for a fix on top of JDK-8146972 to fix JDK-8272574 and also prevent this bug. The assertion code added by JDK-8272574 is still good to have. I suggest to revisit the improved rewiring for loads done with JDK-8272574 in an RFE. I think it should still be beneficial but requires some more careful checking (to avoid the problems reported in this bug).
>> 
>> 
>> Explanation of JDK-8272574 with regard to JDK-8146972 (covered by `test2-5()`):
>> 
>> When deciding if we can apply loop predication to a check inside the loop, we call `IdealLoopTree::is_range_check()`. This method calls `PhaseIdealLoop::is_scaled_iv_plus_offset()` which can create new nodes for the offset:
>> https://github.com/openjdk/jdk/blob/b79554bb5cef14590d427543a40efbcc60c66548/src/hotspot/share/opto/loopTransform.cpp#L2586-L2588
>> `get_ctrl()` of these new nodes could also be the incoming projection into a loop. But these nodes are not marked as non-loop-invariant as done for the other nodes in `Invariant::Invariant()`:
>> https://github.com/openjdk/jdk/blob/b79554bb5cef14590d427543a40efbcc60c66548/src/hotspot/share/opto/loopPredicate.cpp#L663-L667
>> 
>> The proposed fix keeps track when above code was applied (`data_dependency_on` is set) and does an additional checking for `get_ctrl()` accordingly if a new node was created for the offset in `PhaseIdealLoop::is_scaled_iv_plus_offset()`. This should be fine as we are not using the newly created `offset` node anymore after doing that check.
>> 
>> In discussions with Roland, we think that we should revisit this fix and the fix of JDK-8146972 and clean them up to directly do the invariant checks in `compute_invariance()/visit()` without special handling in the constructor. But given the deadline of 17.0.2 and the fork soon coming up for 18, we think it's the most safe way to go with the proposed fix - if others agree.
>> 
>> Thanks,
>> Christian
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   use direct bailout

Thanks Roland for your review and the offline discussion about it. I will file an RFE once it is integrated to clean it up in JDK 19.

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

PR: https://git.openjdk.java.net/jdk/pull/6670