RFR: 8290850: C2: create_new_if_for_predicate() does not clone pinned phi input nodes resulting in a broken graph [v2]

[Christian Hagedorn]

On Tue, 6 Dec 2022 10:25:35 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> The test cases of this bug reveal the same problem with `PhaseIdealLoop::create_new_if_for_predicate()` as in [JDK-8271954](https://bugs.openjdk.org/browse/JDK-8271954) (reusing pinned data nodes for different UCT paths into the UCT phi - see PR description of https://github.com/openjdk/jdk/pull/5185). While JDK-8271954 only fixed the usage of `PhaseIdealLoop::create_new_if_for_predicate()` for one specific case in loop unswitching, we now need this fix for other usages of `PhaseIdealLoop::create_new_if_for_predicate()` as well. I've found failing cases for most of the usages but I think we should always do a proper cloning as originally added with JDK-8271954. This is what a propose with this patch.
>> 
>> To always do this cloning, I've replaced the `UnswitchingAction` by a `rewire_uncommon_proj_phi_inputs` bool that is false by default but can be set if we should only do a rewiring (we can still do the rewiring for the slow loop as previously done with `UnswitchingAction::SlowLoopRewiring`, also see https://github.com/openjdk/jdk/pull/5185 for more details). However, the current fix does not update ctrl for the slow loop nodes - I've fixed that.
>> 
>> I've reused the implementation of `clone_data_nodes_for_fast_loop()` but refactored and split that method into multiple methods to reuse some of them for the ctrl update of the slow loop nodes.
>> 
>> Thanks,
>> Christian
>
> Christian Hagedorn has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - Add ResourceMark
>  - Merge branch 'master' into JDK-8290850
>  - Fix whitespaces
>  - 8290850: C2: create_new_if_for_predicate() does not clone pinned phi input nodes resulting in a broken graph

I've run some testing again and noticed that we run into some assertion failures where `has_ctrl()` returns true for a CFG node which is wrong. I've had a closer look and noticed that the newly added `ResourceMarks` are a problem:

We call `set_ctrl()` for the newly cloned nodes which lets the `PhaseTransform::_nodes` `Node_List` grow:
https://github.com/openjdk/jdk/blob/dd7385d1e86afe8af79587e80c5046af5c84b5cd/src/hotspot/share/opto/node.cpp#L2774-L2780
We allocate a new array in the resource area because `PhaseTransform::_nodes` was allocated with `Thread::current()->resource_area()`:
https://github.com/openjdk/jdk/blob/389b8f4b788375821a8bb4b017e50f905abdad2d/src/hotspot/share/opto/phaseX.cpp#L573-L576

Once we get out of the scope of the `ResourceMark`, we release the newly allocated array for `_nodes` and we will end up reading this released memory later which results in undefined behavior.

I'm not sure if we have similar problems elsewhere as we have quite some places where we use `ResourceMark`. We could think about using a separate `Arena` for `PhaseTransform` for `_nodes` and `_types`. Then we could safely use `ResourceMark` and don't need to worry about whether the `_nodes` and `_types` array get reallocated in between.

I'll revert my `ResourceMark` commit and run some testing again.

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

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