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

[Roland Westrelin]

On Sun, 30 May 2021 00:50:41 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 refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:
> 
>   8267904: C2 crash when compile negative Arrays.copyOf length after loop

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

> 4454:     // 1. Replace CastIINode with AllocateArrayNode's length.
> 4455:     // 2. Create CastIINode again in arraycopy_move_allocation_here after control flow adjustion.
> 4456:     Node* init_control = init->proj_out(TypeFunc::Control);

I would suggest rephrasing the comment to:
The CastIINode created in GraphKit::new_array (in AllocateArrayNode::make_ideal_length) must stay below the allocation (i.e. is only valid if the allocation succeeds): 1) replace CastIINode with AllocateArrayNode's length here 2) Create CastIINode again once allocation has moved (see below) at the end of this method

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

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