[9] RFR(S): 8179678: ArrayCopy with same src and dst can cause incorrect execution or compiler crash

[Tobias Hartmann]

Hi Roland,

with your fix, compiler/arraycopy/TestEliminatedArrayCopyDeopt fails on all platforms with:

----------messages:(4/454)----------
command: main -XX:-BackgroundCompilation -XX:-UseOnStackReplacement -XX:+IgnoreUnrecognizedVMOptions -XX:-ReduceInitialCardMarks compiler.arraycopy.TestEliminatedArrayCopyDeopt
reason: User specified action: run main/othervm -XX:-BackgroundCompilation -XX:-UseOnStackReplacement -XX:+IgnoreUnrecognizedVMOptions -XX:-ReduceInitialCardMarks compiler.arraycopy.TestEliminatedArrayCopyDeopt 
Mode: othervm [/othervm specified]
elapsed time (seconds): 0.538
----------configuration:(0/0)----------
----------System.out:(1/10)----------
m1 failed
----------System.err:(13/825)----------
java.lang.RuntimeException: Test failed
	at compiler.arraycopy.TestEliminatedArrayCopyDeopt.main(TestEliminatedArrayCopyDeopt.java:206)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:563)
	at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:115)
	at java.base/java.lang.Thread.run(Thread.java:844)

Best regards,
Tobias

On 22.05.2017 21:41, Vladimir Kozlov wrote:
> Okay, this seems fine.
> 
> I will run testing on this change and sponsor if it pass.
> 
> Thanks,
> Vladimir
> 
> On 5/22/17 9:26 AM, Roland Westrelin wrote:
>>
>>> Call nodes have complicated graph of following control
>>> projections. You don't check for them. Why?  I think that was the
>>> reason that we scan memory edges instead of control when searching for
>>> calls.
>>
>> Catch and CatchProj?
>>
>> The logic I changed handles an array copy, once it is expanded, whose
>> destination is a non escaping array.
>>
>> Escape analysis only considers the destination of "validated" array
>> copies as non escaping:
>>
>> PointsToNode::EscapeState es = PointsToNode::ArgEscape;
>> if (call->is_ArrayCopy()) {
>>   ArrayCopyNode* ac = call->as_ArrayCopy();
>>   if (ac->is_clonebasic() ||
>>       ac->is_arraycopy_validated() ||
>>       ac->is_copyof_validated() ||
>>       ac->is_copyofrange_validated()) {
>>     es = PointsToNode::NoEscape;
>>   }
>> }
>>
>> An array copy is "validated" when LibraryCallKit::inline_arraycopy()
>> inserted guards to validate that this arraycopy is a "good" arraycopy.
>>
>> In PhaseMacroExpand::generate_arraycopy() the stub calls that are
>> inserted are all connected through a single ProjNode to the result
>> Region except:
>>
>> 1- the slow arraycopy call which has Catch and CatchProj nodes. But with
>> validated arraycopies, there shouldn't be a slow arraycopy alone but at
>> least one stub call on one of the control path.
>>
>> 2- a checkcast arraycopy for which the return of the call needs to be
>> checked: checkcast arraycopy are explicitly skipped for validated
>> arraycopies.
>>
>> 3- if the copy size is null, PhaseMacroExpand::generate_arraycopy()
>> would generate no call but that special case should be handled by
>> ArrayCopyNode::Ideal.
>>
>> 4- A generic arraycopy (if C2 can't tell whether inputs to arraycopy are
>> arrays).
>>
>> I found that 4 can happen eventhough given the arraycopy is validated,
>> it could be avoided. So here is a new webrev which sets src_elem =
>> dest_elem in PhaseMacroExpand::expand_arraycopy_node() when dest_elem is
>> known and the arraycopy is validated.
>>
>> http://cr.openjdk.java.net/~roland/8179678/webrev.03/
>>
>> Roland.
>>