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

[Tobias Hartmann]

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
>