[aarch64-port-dev ] Fwd: A bug in C1 intrinsic arraycopy

[Zoltán Majó]

Hi Xiang,


thank you for reporting the problem! I'll look into it.

Best regards,


Zoltan


On 06/16/2016 04:54 PM, Xiang Yuan wrote:
> 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