RFR: 8279219: [REDO] C2 crash when allocating array of size too large

[Tobias Hartmann]

On Tue, 4 Jan 2022 08:51:30 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> The new fix is largely similar to the previous one. 3 bugs were filed
> because of the previous change but there are only really 2 issues:
> 
> - attaching the valid length condition at expansion time to the new
>   array runtime call causes issues. With Shenandoah, passes of loop
>   opts are executed after macro expansion but precedence edges are
>   ignored when assigning controls to nodes which causes loop opts to
>   incorrectly attempt to eliminate the node pointed to by the
>   precedence edge. A similar issue occurs when the runtime call ends
>   up in a subgraph that dies after macro expansion because the
>   precedence edge is not cleared by dead code elimination which causes
>   the runtime call to still be reachable. In the new patch, this is
>   fixed by appending an extra input to the runtime call instead of
>   using a precedence edge.
> 
> - In the previous patch, a top valid length input is used when there's
>   no valid length input that needs to be recorded. That can cause an
>   assert failure during CCP. If valid length initially has type top,
>   the CatchNode out of the AllocateArray then have type (control,
>   control). If next, the valid length input becomes constant 0, the
>   CatchNode has type (top, control). An assert catches that the type
>   of the CatchNode doesn't widen. This fixed by using 1 by default as
>   valid length input and tweaking CatchNode::Value.
> 
> The new patch includes test cases for both issues.

Looks good to me, just added some minor comments. I also executed testing and it all passed.

src/hotspot/share/opto/cfgnode.cpp line 2692:

> 2690:       } else if (call->is_AllocateArray()) {
> 2691:         Node* klass_node = call->in(AllocateNode::KlassNode);
> 2692:         Node *length = call->in(AllocateNode::ALength);

`Node *length` -> `Node* length`

src/hotspot/share/opto/cfgnode.cpp line 2700:

> 2698:           f[CatchProjNode::fall_through_index] = Type::TOP;
> 2699:         } else if (valid_length_test_t->is_int()->is_con(0)) {
> 2700:           f[CatchProjNode::fall_through_index] = Type::TOP;

Maybe merge the two ifs because the bodies are identical.

src/hotspot/share/opto/compile.cpp line 3803:

> 3801:         assert(call->is_CallStaticJava(), "static call expected");
> 3802:         assert(call->req() == call->jvms()->endoff() + 1, "missing extra input");
> 3803:         call->del_req(call->req()-1);

Please add a comment explaining that this removes the `valid_length_test`, it's not obvious.

test/hotspot/jtreg/compiler/allocation/TestFailedAllocationBadGraph.java line 2:

> 1: /*
> 2:  * Copyright (c) 2021, Red Hat, Inc. All rights reserved.

Copyright date should be fixed.

test/hotspot/jtreg/compiler/allocation/TestFailedAllocationBadGraph.java line 26:

> 24: /*
> 25:  * @test
> 26:  * bug 8278413

Bug number should be updated to 8279219.

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

Marked as reviewed by thartmann (Reviewer).

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