RFR: 8267904: C2 crash when compile negative Arrays.copyOf length after loop [v5]

[Roland Westrelin]

On Wed, 2 Jun 2021 23:13:02 GMT, Hui Shi <hshi at openjdk.org> wrote:

>> C2 crash when Arrays.copyOf has a negative length after a loop. This happens in release and debug build. Test and hs_err are in JBS.
>> 
>> Crash reason is:
>> - CastIINode is created in GraphKit::new_array (in AllocateArrayNode::make_ideal_length), Cast array lenght  to range [0, maxint-2]. This is safe when allocation is success and CastIINode 's input control is InitializeNode's proj control.
>> - In LibraryCallKit::inline_arraycopy, InitializeNode's proj control's use nodes' control is replaced with AllocateArrayNode's input control (in LibraryCallKit::arraycopy_move_allocation_here). This is necessary to move allocation after array copy checks. But this also includes CastIINode.
>> 
>>    C->gvn_replace_by(init->proj_out(TypeFunc::Control), alloc->in(0));
>> 
>> - CastIINode's control is also adjust to AllocateArrayNode's input control, which is illegal state in laster IGVN phase, casting a negative to [0, maxint-2].
>> - This cause control and nodes after loop become top and removed. The previous loop has no fall-through edge and crash.
>> 
>> Fix is:
>> - In LibraryCallKit::arraycopy_move_allocation_here
>>     - Before replacing init->proj_out(TypeFunc::Control) in, find and replace  CastIINode nodes with original array length.
>>     - After move allocation node, create CastIINode again if necessary.
>> 
>> Before fix:  node 250 is CastII which should be after InitializeNode.
>> ![image](https://user-images.githubusercontent.com/70356247/119938428-f7fa4e80-bfbe-11eb-925e-c239620c73f3.png)
>> 
>> After fix: all arry copy check is performed on original array length node 203
>> ![image](https://user-images.githubusercontent.com/70356247/119938532-2415cf80-bfbf-11eb-98c6-76e6b19b691f.png)
>> 
>> New test test/hotspot/jtreg/compiler/c2/TestNegativeArrayCopyAfterLoop.java is added and pass.
>> Tests performs on Linux X64 and no regression
>> - Tier1/2/3/hotspot_all_no_apps on release and fastdebug build.
>> - Tier1/2/3 with option "-XX:-TieredCompilation -Xbatch" on fastdebug build
>
> Hui Shi has updated the pull request incrementally with one additional commit since the last revision:
> 
>   remove unecessary gvn set_type_bottom and record_for_igvn when creating CastIINode after array allocation

Otherwise looks good to me.

src/hotspot/share/opto/library_call.cpp line 4468:

> 4466: #endif
> 4467:         C->gvn_replace_by(init_out, alloc_length);
> 4468:         record_for_igvn(init_out);

I don't think a call to record_for_igvn() is needed here.

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

Marked as reviewed by roland (Reviewer).

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