RFR: 8182036: Load from initializing arraycopy uses wrong memory state

[Yangfei (Felix)]

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