RFR: 8283408: Fix a C2 crash when filling arrays with unsafe [v2]

[Tobias Hartmann]

On Tue, 22 Mar 2022 03:43:08 GMT, Pengfei Li <pli at openjdk.org> wrote:

>> We recently found a segmentation fault issue in C2 compiler with some
>> code that uses Java Unsafe API to initialize an array in a loop. It can
>> be reproduced by below code snippet compiled by C2 on AArch64. It's also
>> reproducible on x86 with an additional VM option "-XX:+OptimizeFill".
>> 
>>   byte[] arr = new byte[size];
>>   int offset = unsafe.arrayBaseOffset(byte[].class);
>>   for (int i = offset; i < offset + size; i++) {
>>     unsafe.putByte(arr, i, val);
>>   }
>> 
>> This issue is caused by a NULL pointer in a C2 loop optimization phase
>> called intrinsify_fill. In this phase, array filling loop patterns are
>> recognized and replaced by some intrinsics. But filling operations with
>> Unsafe API call are not handled very well. From C2 mid-end's point of
>> view, a difference between an Unsafe call and a normal array access like
>> `arr[i] = val` is element addressing. For normal array accesses, C2 uses
>> two AddP nodes for computing an element's address - one for adding array
>> header size and another for adding the element's relative offset from
>> the header. But Unsafe calls may have only one AddP node for adding an
>> absolute offset of an element from the array base. In current code, the
>> intrinsify_fill phase creates an AddP node but with NULL input for above
>> case and eventually causes a segmentation fault.
>> 
>> In this patch, we add a check to allow one AddP node in array filling
>> patterns to be optional. After this fix, the case above can be optimized
>> by intrinsify_fill as well. We know that the Unsafe call is rarely used
>> in Java application code and developers should use it at their own risk.
>> But we still propose this fix because C2 crashes even Unsafe is used in
>> a correct way.
>> 
>> Jtreg hotspot::hotspot_all_no_apps, jdk::tier1~3 and langtools::tier1
>> are tested and no issue is found. We also create a new jtreg case within
>> this patch.
>
> Pengfei Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add a few comments

Looks good to me too.

src/hotspot/share/opto/loopTransform.cpp line 4050:

> 4048:   // AddP node adding an absolute offset, so we do a NULL check here.
> 4049:   if (offset != NULL) {
> 4050:     from = new AddPNode(base, from, offset);

Is it worth adding an `C->has_unsafe_access()` assert here?

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

Marked as reviewed by thartmann (Reviewer).

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