[jdk18] RFR: 8278413: C2 crash when allocating array of size too large

[Nils Eliasson]

On Wed, 15 Dec 2021 12:36:17 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> On the fallthrough path from an AllocateArray, the length of the
> allocated array is casted (with a CastII) to [0, max_size] with
> max_size some number that depends on the array type and can be less
> than max_jint.
> 
> Allocating an array of a length that's not in [0, max_size] causes the
> CastII to become top. The fallthrough path must be killed as well in
> that case otherwise this can lead to a broken graph. Currently c2 has
> logic to protect against an allocation of array of negative size in
> AllocateArrayNode::Ideal(). That call replaces the fallthrough path
> with an Halt node. But if the size is too big, then the fallthrough
> path is left as is.
> 
> This patch fixes that issues. It also reworks the length negative
> case. I added a Bool/CmpU input to the AllocateArray that tests for a
> valid length. If that input becomes false, CatchNode::Value() kills
> the fallthrough path. That logic is similar to that for a virtual call
> with a null receiver. I also removed AllocateArrayNode::Ideal() now
> that CatchNode::Value() takes care of the same corner case. The code
> in AllocateArrayNode::Ideal() was added by Vladimir and he told me he
> tried extending CatchNode::Value() at the time but that caused test
> failures. I had no issues in my testing so I assume doing it that way
> is ok now.
> 
> The new input to AllocateArray is moved to the CallStaticJava runtime
> call for array allocation on macro expansion as a precedence edge. The
> reason for that is that final graph reshape needs a way to tell
> whether the missing path out of the allocation is legal or not. final
> graph reshape then removes the then useless precedence edge.

I general I really like the change. I simplifies things nicely.

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

> 2698:           Node* valid_length_test = call->in(AllocateNode::ValidLengthTest);
> 2699:           const Type* valid_length_test_t = phase->type(valid_length_test);
> 2700:           if (valid_length_test_t->isa_int() && valid_length_test_t->is_int()->is_con(0)) {

Here you do: 

Node* valid_length_test = call->in(AllocateNode::ValidLengthTest);
const Type* valid_length_test_t = phase->type(valid_length_test);
if (valid_length_test_t->isa_int() && valid_length_test_t->is_int()->is_con(0)) {


But in compile.cpp:3766 you do:

Node* valid_length_test = call->in(call->req());
call->rm_prec(call->req());
if (valid_length_test->find_int_con(1) == 0) {


Why "call->req()" and not "call->in(AllocateNode::ValidLengthTest)"?

And why not "if (valid_length_test->find_int_con(1) == 0) {" in both places?

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

Changes requested by neliasso (Reviewer).

PR: https://git.openjdk.java.net/jdk18/pull/30