RFR(XXS): 8176505: Wrong assertion 'should be an array copy/clone' in arraycopynode.cpp

[Tobias Hartmann]

Hi Volker,

On 10.03.2017 17:34, Volker Simonis wrote:
> Hi Roland, Tobias,
> 
> thanks for looking at my fix! You're both right, the assertion should
> be moved into the else branch instead of being removed. I've done that
> and also updated the assertion message because in the else branch the
> array copy node can only be a clone:
> 
> http://cr.openjdk.java.net/~simonis/webrevs/2017/8176505.v1/
> 
> I've also added my small test as regression test. But I'm currently at
> a loss with it. Without my fix, running it obviously crashes the VM,
> but JTreg still reports the test as passed:
> 
> #section:main
> ----------messages:(4/206)----------
> command: main compiler.arraycopy.TestObjectArrayCopy
> reason: User specified action: run main/othervm
> compiler.arraycopy.TestObjectArrayCopy
> Mode: othervm [/othervm specified]
> elapsed time (seconds): 1.731
> ----------configuration:(0/0)----------
> ----------System.out:(23/1256)----------
> # To suppress the following error report, specify this argument
> # after -XX: or in .hotspotrc:  SuppressErrorAt=/arraycopynode.cpp:228
> #
> # A fatal error has been detected by the Java Runtime Environment:
> #
> #  Internal Error
> (/usr/work/d046063/OpenJDK/jdk9-dev/hotspot/src/share/vm/opto/arraycopynode.cpp:228),
> pid=12359, tid=12441
> #  assert(ary_src != __null) failed: should be an array copy/clone
> ...
> #
> Current thread is 12441
> Dumping core ...
> ----------System.err:(1/15)----------
> STATUS:Passed.
> 
> Can you please check if you see the same behavior with your version of
> JTreg? Or am I doing something fundamentally wrong? I'm pretty sure
> that a crashing VM should lead to test failure and I'm also pretty
> sure that it worked that way in the past.

I can confirm the same behavior on my system (though it's very intermittent). I think it's because 20.000 loop iterations are just enough to trigger the C2 compilation but when the compilation fails, the test is done and jtreg is already shutting down without recognizing that the VM crashed.

I would suggest to either user -Xbatch or increase the number of loop iterations (40.000 works on my system).

Thanks,
Tobias

> On Fri, Mar 10, 2017 at 3:13 PM, Tobias Hartmann
> <tobias.hartmann at oracle.com> wrote:
>> Hi Volker,
>>
>> thanks for fixing this issue!
>>
>> I added this assert with JDK-8156760 which was about Object.clone(). I agree with Roland that the assert should be moved into the else branch which handles clonebasic.
>>
>> Could you add the regression test to your changeset?
>>
>> Thanks,
>> Tobias
>>
>> On 10.03.2017 14:58, Volker Simonis wrote:
>>> Hi,
>>>
>>> first of all I think this isn't really a high prio bug because the
>>> assertion is wrong and there's no impact for product builds. On the
>>> other hand I think a lot of test running on debug builds may fail
>>> because of this issue so probably better to fix this in 9 as well (I
>>> actually wonder that you haven't seen this issue until now :)
>>>
>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8176505/
>>> https://bugs.openjdk.java.net/browse/JDK-8176505
>>>
>>> Here are the gory details: the following assertion in
>>> arraycopynode.cpp is wrong because src_type can be a plain Object:
>>>
>>> const TypeAryPtr* ary_src = src_type->isa_aryptr();
>>> assert(ary_src != NULL, "should be an array copy/clone");
>>>
>>> This can be easily demonstrated with the following trivial test program:
>>>
>>> public class ArraySrcType {
>>>   public static boolean crash(Object src) {
>>>     String[] dst = new String[1];
>>>     System.arraycopy(src, 0, dst, 0, 1);
>>>     return dst[0] == null;
>>>   }
>>>   public static void main(String[] args) {
>>>     String[] sa = new String[1];
>>>     for (int i = 0; i < 20_000; i++) {
>>>       crash(sa);
>>>     }
>>>   }
>>> }
>>>
>>> Running it with a slow- or fastdebug build results in the following crash:
>>>
>>> # To suppress the following error report, specify this argument
>>> # after -XX: or in .hotspotrc: SuppressErrorAt=/arraycopynode.cpp:228
>>> #
>>> # A fatal error has been detected by the Java Runtime Environment:
>>> #
>>> # Internal Error
>>> (/usr/work/d046063/OpenJDK/jdk10-hs/hotspot/src/share/vm/opto/arraycopynode.cpp:228),
>>> pid=34335, tid=34410
>>> # assert(ary_src != __null) failed: should be an array copy/clone
>>> #
>>>
>>> Notice that the assertion is unnecessary anyway, because some lines
>>> later int he code we explicitely check for ary_src being NULL and bail
>>> out if that 's the case:
>>>
>>>     if (ary_src == NULL || ary_src->klass() == NULL ||
>>>         ary_dest == NULL || ary_dest->klass() == NULL) {
>>>       // We don't know if arguments are arrays
>>>       return false;
>>>     }
>>>
>>> Unfortunately, I still need a sponsor :(
>>>
>>> Thank you and best regards,
>>> Volker
>>>