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.

Overall fix looks ok to me.

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

> 4472:           assert(prev->type()->is_int()->_lo == cur->type()->is_int()->_lo, "not same");
> 4473:           assert(prev->type()->is_int()->_hi == cur->type()->is_int()->_hi, "not same");
> 4474:         }

Is this really necessary? Have you seen cases with multiple identical CastII nodes? Or is it to be extra cautious?

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

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