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

[Volker Simonis]

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.

Regards,
Volker


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
>>