RFR(S): 8159611: C2: ArrayCopy elimination skips required parameter checks

[Vladimir Kozlov]

Not generating exception is definitely bug.

First, about test case. It would be nice if it also verifies other 
IndexOutOfBoundsException cases.

Actually additional dynamic check will help in case of negative length 
is know during compilation. The allocation code will be eliminated very 
early instead of waiting macro expansion:

       int length = alloc->in(AllocateNode::ALength)->find_int_con(-1);
       if (length < 0) {
         NOT_PRODUCT(fail_eliminate = "Array's size is not constant";)
         can_eliminate = false;
       }

About additional length check in your new test. I think it may be 
collapsed with preceding check since it is generated after other checks.
So I would suggest to move it after offset + length check.

Thanks,
Vladimir

On 8/16/16 7:57 AM, Volker Simonis wrote:
> On Tue, Aug 16, 2016 at 7:24 AM, Tobias Hartmann
> <tobias.hartmann at oracle.com> wrote:
>> Hi Volker,
>>
>> thanks for taking care of this issue!
>>
>> Did you check what happens if the allocation is not eliminated and macro expansion phase emits another negative guard? Are the checks merged?
>>
>
> It depends. I just saw that in some cases the regression test worked
> before, because the length check was done in
> SharedRuntime::slow_arraycopy_C(). So in that case there's obviously
> nothing that can be merged. But the test case is obviously a
> degenerated example anyway, so I don't think that's a problem.
>
> If I do a more real-world example like this where the arracopy can not
> be eliminated because one of its arguments escapes:
>
>   public static boolean do_test2(int length, Object[] dest) {
>     try {
>       System.arraycopy(new Object[10], 1, dest, 1, length);
>       return false;
>     } catch (IndexOutOfBoundsException e) {
>       return true;
>     }
>   }
>
> and call it with:
>
> do_test2(8, new Object[10])
>
> the generated code for do_test2() unfortunately contains one more
> check now with my change (the 'length' field is in [rsp + #0]):
>
> 0a2   B4: #    B18 B5 <- B3  Freq: 0,999999
> 0a2       movl    R9, [rsp + #0]    # spill
> 0a6       testl   R9, R9
> 0a9       jl     B18  P=0,000001 C=-1,000000
> 0a9
> 0af   B5: #    B18 B6 <- B4  Freq: 0,999998
> 0af       movl    RBX, R9    # spill
> 0b2       incl    RBX    # int
> 0b4       cmpl    RBX, #10    # unsigned
> 0b7       jnbe,u  B18  P=0,000001 C=-1,000000
>
> The generated code before my change looked like this (againthe
> 'length' field is in [rsp + #0]):
>
> 0a1   B4: #    B17 B5 <- B3  Freq: 0,999999
> 0a1       movl    R11, [rsp + #8]    # spill
> 0a6       incl    R11    # int
> 0a9       cmpl    R11, #10    # unsigned
> 0ad       jnbe,u  B17  P=0,000001 C=-1,000000
>
> It seems that the 'length' check has been completely eliminated before.
>
> So I need to do some more tests to understand why the new check isn't
> eliminated.
>
> Do you think the new check results in a performance regression? Have
> you run some benchmarks?
>
>> I would prefer brackets around the if body but you don't need to send another webrev:
>>  if (EliminateAllocations) {
>>    generate_negative_guard(length, slow_region);
>>  }
>
> Yes, I agree.
>
>>
>> Best regards,
>> Tobias
>>
>> On 12.08.2016 21:13, Volker Simonis wrote:
>>> Hi,
>>>
>>> can I please have a review and sponsor for the following fix:
>>>
>>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8159611
>>> https://bugs.openjdk.java.net/browse/JDK-8159611
>>>
>>>
>>> We are inserting several checks for the arguments of
>>> System.arraycopy() in LibraryCallKit::inline_arraycopy() before
>>> intensifying the call in LibraryCallKit::inline_arraycopy. However the
>>> check for the 'length' argument of arracopy is postponed to the macro
>>> expansion phase in PhaseMacroExpand::generate_arraycopy().
>>>
>>> But if we are running with EscapeAnalysis and EliminateAllocations,
>>> the array allocations inside a call to System.arraycopy() may get
>>> eliminated and thus the complete call to System.arraycopy() will be
>>> removed (see PhaseMacroExpand::process_users_of_allocation). In this
>>> case the extra 'length' check won't be added by
>>> PhaseMacroExpand::generate_arraycopy() any more because macro
>>> expansion happens after the elimination of macro nodes.
>>>
>>> In such a case it may happen that System.arraycopy() will silently
>>> accept an invalid (i.e. negative) 'length' parameter, although it
>>> should actually throw an ArrayOutOfBounds exception.
>>>
>>> The fix is simple: also insert a check for the length field in
>>> LibraryCallKit::inline_arraycopy() if we are running with
>>> EliminateAllocations.
>>>
>>> Regards,
>>> Volker
>>>