Skip to content

Commit 9046d7a

Browse files
eme64eme64
committed
8335390: C2 MergeStores: wrong result with Unsafe
Reviewed-by: thartmann, chagedorn, kvn
1 parent 318d9ac commit 9046d7a

File tree

3 files changed

+141
-3
lines changed

3 files changed

+141
-3
lines changed

‎src/hotspot/share/opto/memnode.cpp‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2984,6 +2984,9 @@ StoreNode* MergePrimitiveArrayStores::run() {
29842984
type2aelembytes(bt) != _store->memory_size()) {
29852985
return nullptr;
29862986
}
2987+
if (_store->is_unsafe_access()) {
2988+
return nullptr;
2989+
}
29872990

29882991
// The _store must be the "last" store in a chain. If we find a use we could merge with
29892992
// then that use or a store further down is the "last" store.
@@ -3017,11 +3020,13 @@ bool MergePrimitiveArrayStores::is_compatible_store(const StoreNode* other_store
30173020
int opc = _store->Opcode();
30183021
assert(opc == Op_StoreB || opc == Op_StoreC || opc == Op_StoreI, "precondition");
30193022
assert(_store->adr_type()->isa_aryptr() != nullptr, "must be array store");
3023+
assert(!_store->is_unsafe_access(), "no unsafe accesses");
30203024

30213025
if (other_store == nullptr ||
30223026
_store->Opcode() != other_store->Opcode() ||
30233027
other_store->adr_type() == nullptr ||
3024-
other_store->adr_type()->isa_aryptr() == nullptr) {
3028+
other_store->adr_type()->isa_aryptr() == nullptr ||
3029+
other_store->is_unsafe_access()) {
30253030
return false;
30263031
}
30273032

‎test/hotspot/jtreg/compiler/c2/TestMergeStores.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -611,8 +611,9 @@ static Object[] test1e(byte[] a) {
611611
}
612612

613613
@Test
614-
@IR(counts = {IRNode.STORE_L_OF_CLASS, "byte\\\\[int:>=0] \\\\(java/lang/Cloneable,java/io/Serializable\\\\)", "1"},
615-
applyIf = {"UseUnalignedAccesses", "true"})
614+
// Disabled by JDK-8335390, to be enabled again by JDK-8335392.
615+
// @IR(counts = {IRNode.STORE_L_OF_CLASS, "byte\\\\[int:>=0] \\\\(java/lang/Cloneable,java/io/Serializable\\\\)", "1"},
616+
// applyIf = {"UseUnalignedAccesses", "true"})
616617
static Object[] test1f(byte[] a) {
617618
UNSAFE.putByte(a, UNSAFE.ARRAY_BYTE_BASE_OFFSET + 0, (byte)0xbe);
618619
UNSAFE.putByte(a, UNSAFE.ARRAY_BYTE_BASE_OFFSET + 1, (byte)0xba);

‎test/hotspot/jtreg/compiler/c2/TestMergeStoresUnsafeArrayPointer.java‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
/*
2+
* Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
3+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
4+
*
5+
* This code is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU General Public License version 2 only, as
7+
* published by the Free Software Foundation.
8+
*
9+
* This code is distributed in the hope that it will be useful, but WITHOUT
10+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
11+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
12+
* version 2 for more details (a copy is included in the LICENSE file that
13+
* accompanied this code).
14+
*
15+
* You should have received a copy of the GNU General Public License version
16+
* 2 along with this work; if not, write to the Free Software Foundation,
17+
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
18+
*
19+
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
20+
* or visit www.oracle.com if you need additional information or have any
21+
* questions.
22+
*/
23+
24+
/*
25+
* @test
26+
* @bug 8335390
27+
* @summary Test merge stores for some Unsafe store address patterns.
28+
* @modules java.base/jdk.internal.misc
29+
* @requires vm.bits == 64
30+
* @requires os.maxMemory > 8G
31+
* @run main/othervm -XX:CompileCommand=compileonly,compiler.c2.TestMergeStoresUnsafeArrayPointer::test*
32+
* -Xbatch
33+
* -Xmx8g
34+
* compiler.c2.TestMergeStoresUnsafeArrayPointer
35+
* @run main/othervm -Xmx8g
36+
* compiler.c2.TestMergeStoresUnsafeArrayPointer
37+
*/
38+
39+
package compiler.c2;
40+
import jdk.internal.misc.Unsafe;
41+
42+
public class TestMergeStoresUnsafeArrayPointer {
43+
static final Unsafe UNSAFE = Unsafe.getUnsafe();
44+
45+
// We allocate a big int array of length:
46+
static final int SIZE = (1 << 30) + 100;
47+
48+
// This gives us a memory region of 4x as many bytes:
49+
static final long BYTE_SIZE = 4L * SIZE; // = 1L << 32 + 400L
50+
51+
// We set an "anchor" in the middle of this memory region, in bytes:
52+
static final long ANCHOR = BYTE_SIZE / 2;
53+
54+
static int four = 4;
55+
56+
public static void main(String[] args) {
57+
System.out.println("Allocate big array of SIZE = " + SIZE);
58+
int[] big = new int[SIZE];
59+
60+
// Each test is executed a few times, so that we can see the difference between
61+
// interpreter and compiler.
62+
int errors = 0;
63+
64+
long val = 0;
65+
System.out.println("test1");
66+
for (int i = 0; i < 100_000; i++) {
67+
testClear(big);
68+
test1(big, ANCHOR);
69+
long sum = testSum(big);
70+
if (i == 0) {
71+
val = sum;
72+
} else {
73+
if (sum != val) {
74+
System.out.println("ERROR: test1 had wrong value: " + val + " != " + sum);
75+
errors++;
76+
break;
77+
}
78+
}
79+
}
80+
81+
val = 0;
82+
System.out.println("test2");
83+
for (int i = 0; i < 100_000; i++) {
84+
testClear(big);
85+
test2(big, ANCHOR);
86+
long sum = testSum(big);
87+
if (i == 0) {
88+
val = sum;
89+
} else {
90+
if (sum != val) {
91+
System.out.println("ERROR: test2 had wrong value: " + val + " != " + sum);
92+
errors++;
93+
break;
94+
}
95+
}
96+
}
97+
98+
if (errors > 0) {
99+
throw new RuntimeException("ERRORS: " + errors);
100+
}
101+
System.out.println("PASSED");
102+
}
103+
104+
// Only clear and sum over relevant parts of array to make the test fast.
105+
static void testClear(int[] a) {
106+
for (int j = 0 ; j < 100; j++) { a[j] = j; }
107+
for (int j = a.length/2 - 100; j < a.length/2 + 100; j++) { a[j] = j; }
108+
for (int j = a.length - 100; j < a.length + 0; j++) { a[j] = j; }
109+
}
110+
111+
static long testSum(int[] a) {
112+
long sum = 0;
113+
for (int j = 0 ; j < 100; j++) { sum += a[j]; }
114+
for (int j = a.length/2 - 100; j < a.length/2 + 100; j++) { sum += a[j]; }
115+
for (int j = a.length - 100; j < a.length + 0; j++) { sum += a[j]; }
116+
return sum;
117+
}
118+
119+
// Reference: expected to merge.
120+
static void test1(int[] a, long anchor) {
121+
long base = UNSAFE.ARRAY_INT_BASE_OFFSET + anchor;
122+
UNSAFE.putInt(a, base + 0, 0x42424242);
123+
UNSAFE.putInt(a, base + 4, 0x66666666);
124+
}
125+
126+
// Test: if MergeStores is applied this can lead to wrong results
127+
static void test2(int[] a, long anchor) {
128+
long base = UNSAFE.ARRAY_INT_BASE_OFFSET + ANCHOR;
129+
UNSAFE.putInt(a, base + 0 + (long)(four + Integer.MAX_VALUE), 0x42424242);
130+
UNSAFE.putInt(a, base + Integer.MAX_VALUE + (long)(four + 4 ), 0x66666666);
131+
}
132+
}

0 commit comments

Comments
 (0)