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

[Hohensee, Paul]

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}