RFR: 8182036: Load from initializing arraycopy uses wrong memory state
Yangfei (Felix)
felix.yang at huawei.com
Mon Apr 19 13:16:36 UTC 2021
Hi,
Thanks for looking at this.
Could I have another review from some C2 experts please?
Felix
> -----Original Message-----
> From: Hohensee, Paul [mailto:hohensee at amazon.com]
> Sent: Saturday, April 10, 2021 7:17 AM
> To: Yangfei (Felix) <felix.yang at huawei.com>; jdk8u-dev <jdk8u-
> dev at openjdk.java.net>
> Subject: RE: RFR: 8182036: Load from initializing arraycopy uses wrong
> memory state
>
> Looks reasonable to me, but I'd be happier if a C2 engineer weighed in.
> Roland?
>
> Thanks,
> Paul
>
> -----Original Message-----
> From: jdk8u-dev <jdk8u-dev-retn at openjdk.java.net> on behalf of "Yangfei
> (Felix)" <felix.yang at huawei.com>
> Date: Sunday, March 28, 2021 at 8:13 PM
> To: jdk8u-dev <jdk8u-dev at openjdk.java.net>
> Subject: RFR: 8182036: Load from initializing arraycopy uses wrong memory
> state
>
> Hi,
>
> Please review this 8u backport fixing an user-visible C2 bug.
>
> Original bug: https://bugs.openjdk.java.net/browse/JDK-8182036
> Original patch:
> http://hg.openjdk.java.net/jdk10/jdk10/hotspot/rev/fe7fdd0fc266
> Original review thread: https://mail.openjdk.java.net/pipermail/hotspot-
> compiler-dev/2017-June/026448.html
>
> Webrev for 8u: http://cr.openjdk.java.net/~fyang/8182036-8u/webrev.00
>
> This bug can always reproduced by running the provided test case with 8u on
> x86_64 or aarch64 linux.
> The AArch64 C2 JIT code snippet with & without the fix is shown here [1] [2].
>
> I think we only needs this part for 8u (quotation from the original review
> thread):
> "
> Initialization of dst is performed by the arraycopy.
> PhaseMacroExpand::generate_block_arraycopy() generates a load/store
> pair to copy the element at offset 1 so it can bulk copy the elements starting
> at offset 2. Both the load and store are on the same slice which for an
> initializing arraycopy is raw memory. So the src[1] load and the src[1] = 0x42
> store are not on the same slice and the load can happen before the store
> resulting in an incorrect execution.
>
> The fix I propose is for the load to be from the src's slice.
> "
>
> The rest of the original fix is not needed for 8u as it's related to the following
> two issues which is not there in 8u:
> 8076188: Optimize arraycopy out for non escaping destination
> 8150933: System::arraycopy intrinsic doesn't mark mismatched loads
>
> Performed full jtreg test on both x86_64 and aarch64 Linux.
> Newly added jtreg test fails without the patch and passes otherwise.
> OK to backport?
>
> Thanks,
> Felix
>
> [1]. AArch64 C2 JIT code without patch:
>
> 284 ;; B5: # N1 <- B9 B4 Freq: 1
> 285
> 286 ;; 0x42
> 287 0x0000ffff8d01e13c: mov w11, #0x42 // #66
> 288 0x0000ffff8d01e140: str xzr, [x19, #16] ;*invokestatic arraycopy
> 289 ; - TestInitializingACLoadWithBadMem::test1 at 25
> (line 14)
> 290
> 291 0x0000ffff8d01e144: str w11, [x14, #20] ;*iastore
> 292 ; - TestInitializingACLoadWithBadMem::test1 at 18
> (line 11)
> 293
> 294 0x0000ffff8d01e148: str wzr, [x19, #20] <================
> store wrong value 0
> 295 ;; 0x4
> 296 0x0000ffff8d01e14c: orr x2, xzr, #0x4
> 297 0x0000ffff8d01e150: add x1, x19, #0x18
> 298 0x0000ffff8d01e154: add x0, x14, #0x18
> 299 0x0000ffff8d01e158: bl Stub::arrayof_jlong_disjoint_arraycopy
> 300 ;*invokestatic arraycopy
> 301 ; - TestInitializingACLoadWithBadMem::test1 at 25
> (line 14)
> 302 ; {runtime_call}
>
> [2]. AArch64 C2 JIT code with patch:
>
> 284 ;; B5: # N1 <- B9 B4 Freq: 1
> 285
> 286 ;; 0x42
> 287 0x0000ffff9d01e03c: mov w11, #0x42 // #66
> 288 0x0000ffff9d01e040: str xzr, [x19, #16] ;*invokestatic arraycopy
> 289 ; - TestInitializingACLoadWithBadMem::test1 at 25
> (line 14)
> 290
> 291 0x0000ffff9d01e044: str w11, [x14, #20] ;*iastore
> 292 ; - TestInitializingACLoadWithBadMem::test1 at 18
> (line 11)
> 293
> 294 0x0000ffff9d01e048: str w11, [x19, #20] <================
> store correct value 0x42
> 295 ;; 0x4
> 296 0x0000ffff9d01e04c: orr x2, xzr, #0x4
> 297 0x0000ffff9d01e050: add x1, x19, #0x18
> 298 0x0000ffff9d01e054: add x0, x14, #0x18
> 299 0x0000ffff9d01e058: bl Stub::arrayof_jlong_disjoint_arraycopy
> 300 ;*invokestatic arraycopy
> 301 ; - TestInitializingACLoadWithBadMem::test1 at 25
> (line 14)
> 302 ; {runtime_call}
>
>
More information about the jdk8u-dev
mailing list