Fwd: A bug in C1 intrinsic arraycopy

[Xiang Yuan]

Hi all,

         We find a bug in hotspot C1 intrinsic arraycopy, and it is
reported at https://bugs.openjdk.java.net/browse/JDK-8159431. This report
includes test case, test result and patch.

We have triggered it with AArch64 and x86 slowdebug & fastdebug JDK. We
didn't test it on Sparc, however from the codes, it may have the same issue.



*****Testcase Begin*****

public class TestArrayCopy {

     public static void main(String args[]) {

         System.out.println("TestArrayCopy");

         Object aArray[] = new Object[10];



         for (int i = 0; i < 10; i++) {

             aArray[i] = new Object();

         }



         new TestArrayCopy().test(aArray);

     }



     public void test(Object aArray[]) {

         Object a = new Object();



         try {

             System.arraycopy(aArray, 0, a, 0, 1);

             throw new RuntimeException ("Test dst is not array failed!");

         } catch (ArrayStoreException e) {}

     }

}

*****Testcase End*****



To trigger this bug easier, run it with a fastdebug or slowdebug JDK.



Command line: java -XX:TieredStopAtLevel=1 -XX:-UseCompressedClassPointers
-Xcomp -XX:CompileOnly=TestArrayCopy.test,System.arraycopy TestArrayCopy



Expected Result:

    TestArrayCopy



Actual Result:

 TestArrayCopy

 Exception in thread "main" java.lang.RuntimeException: Test dst is not
array failed!

         at TestArrayCopy.test(TestArrayCopy.java:18)

         at TestArrayCopy.main(TestArrayCopy.java:10)





         The cause of this bug is that in LIR_Assembler::emit_arraycopy,
when the type check of dst or src is needed, it uses the following steps to
do arraycopy:

1. check class of src and dst, if class_src is a sub-class of class_dst,
GOTO 4. Type check includes:

         (1) MacroAssembler::check_klass_subtype_fast_path

    (2) Runtime1::slow_subtype_check

                   2. check whether src and dst are both object arrays.

                   3. ......

                   4. do arraycopy.



         However, in the above test case, the src is an array of Object and
the dst is an Object. And Object array is a sub-class of Object, and the
check in Step 1 - (1) is passed. Then the arraycopy is done. This means the
first element in src is copied into the dst, which is a object of class
Object, not an object array.



         The option -XX:-UseCompressedClassPointers is necessary. With this
option, when a new object ( not a array ) is created,
C1_MacroAssembler::initialize_header will call store_klass_gap() to set 0
to the place where stores the length of an array.

         And before the above 4 steps in LIR_Assembler::emit_arraycopy, it
will check whether the length of src or dst is large enough. Therefore,
length 0 will cause this check fail, and arraycopy will not be done.



         Without this option, the memory location in an object (not array)
which stores the length in an array is uninitialized which causes the bug
is easier to be triggered in debug mode.







         The following patch can fix it in aarch64 ( which is also included
in the bug report), and other platforms can fix it with similar patches.

******** Patch Begin *********

diff -r c0b5ea3442e1 src/cpu/aarch64/vm/c1_LIRAssembler_aarch64.cpp

 --- a/src/cpu/aarch64/vm/c1_LIRAssembler_aarch64.cpp Fri Jun 03 17:45:03
2016 -0400

 +++ b/src/cpu/aarch64/vm/c1_LIRAssembler_aarch64.cpp Mon Jun 06 11:32:02
2016 +0800

 @@ -2315,6 +2315,14 @@

    __ load_klass(src, src);

    __ load_klass(dst, dst);



 + if (!(flags & LIR_OpArrayCopy::dst_objarray)) {

 + assert(SystemDictionary::Object_klass_loaded(), "check needed");

 + Klass * obj_klass =
SystemDictionary::well_known_klass(SystemDictionary::Object_klass_knum);

 + __ mov(tmp, (u_int64_t)obj_klass);

 + __ cmp(dst, tmp);

 + __ br(Assembler::EQ, slow);

 + }

 +

    __ check_klass_subtype_fast_path(src, dst, tmp, &cont, &slow, NULL);



    __ PUSH(src, dst);

******** Patch End  ********



         This patch checks whether dst's klass is Object. If so, goes to
check whether dst is an object array.



         This check in the patch is a little late, that is after the length
check of src and dst. This may make the check in the patch is not
necessarily correct.

         For without the argument -XX:-UseCompressedClassPointers, the ‘
length’ of an object ( not array ) is uninitialized.

Set 0 to the ‘length’ of an object causes an extra store in each new
operation. And we haven’t found a better solution.





         Thanks a lot!

         Best Wishes!



         Xiang Yuan

         2016.06.16
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20160616/20207b2c/attachment.html>