Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.
/ jdk23u Public archive

Commit c644570

Browse files
committed
8340313: Crash due to invalid oop in nmethod after C1 patching
Reviewed-by: chagedorn Backport-of: 58d39c3
1 parent 73b2341 commit c644570

File tree

10 files changed

+159
-21
lines changed

10 files changed

+159
-21
lines changed

src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ address NativeCall::destination() const {
7878
//
7979
// Used in the runtime linkage of calls; see class CompiledIC.
8080
void NativeCall::set_destination_mt_safe(address dest) {
81-
assert((Patching_lock->is_locked() || SafepointSynchronize::is_at_safepoint()) ||
81+
assert((CodeCache_lock->is_locked() || SafepointSynchronize::is_at_safepoint()) ||
8282
CompiledICLocker::is_safe(addr_at(0)),
8383
"concurrent code patching");
8484

src/hotspot/cpu/ppc/nativeInst_ppc.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -92,10 +92,10 @@ address NativeCall::destination() const {
9292
// Used in the runtime linkage of calls; see class CompiledIC.
9393
//
9494
// Add parameter assert_lock to switch off assertion
95-
// during code generation, where no patching lock is needed.
95+
// during code generation, where no lock is needed.
9696
void NativeCall::set_destination_mt_safe(address dest, bool assert_lock) {
9797
assert(!assert_lock ||
98-
(Patching_lock->is_locked() || SafepointSynchronize::is_at_safepoint()) ||
98+
(CodeCache_lock->is_locked() || SafepointSynchronize::is_at_safepoint()) ||
9999
CompiledICLocker::is_safe(addr_at(0)),
100100
"concurrent code patching");
101101

src/hotspot/cpu/riscv/nativeInst_riscv.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -68,10 +68,10 @@ address NativeCall::destination() const {
6868
// Used in the runtime linkage of calls; see class CompiledIC.
6969
//
7070
// Add parameter assert_lock to switch off assertion
71-
// during code generation, where no patching lock is needed.
71+
// during code generation, where no lock is needed.
7272
void NativeCall::set_destination_mt_safe(address dest, bool assert_lock) {
7373
assert(!assert_lock ||
74-
(Patching_lock->is_locked() || SafepointSynchronize::is_at_safepoint()) ||
74+
(CodeCache_lock->is_locked() || SafepointSynchronize::is_at_safepoint()) ||
7575
CompiledICLocker::is_safe(addr_at(0)),
7676
"concurrent code patching");
7777

src/hotspot/cpu/s390/nativeInst_s390.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -658,8 +658,8 @@ void NativeGeneralJump::insert_unconditional(address code_pos, address entry) {
658658

659659
void NativeGeneralJump::replace_mt_safe(address instr_addr, address code_buffer) {
660660
assert(((intptr_t)instr_addr & (BytesPerWord-1)) == 0, "requirement for mt safe patching");
661-
// Bytes_after_jump cannot change, because we own the Patching_lock.
662-
assert(Patching_lock->owned_by_self(), "must hold lock to patch instruction");
661+
// Bytes_after_jump cannot change, because we own the CodeCache_lock.
662+
assert(CodeCache_lock->owned_by_self(), "must hold lock to patch instruction");
663663
intptr_t bytes_after_jump = (*(intptr_t*)instr_addr) & 0x000000000000ffffL; // 2 bytes after jump.
664664
intptr_t load_const_bytes = (*(intptr_t*)code_buffer) & 0xffffffffffff0000L;
665665
*(intptr_t*)instr_addr = load_const_bytes | bytes_after_jump;

src/hotspot/cpu/x86/nativeInst_x86.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ void NativeCall::insert(address code_pos, address entry) {
218218
// (spinlock). Then patches the last byte, and then atomically replaces
219219
// the jmp's with the first 4 byte of the new instruction.
220220
void NativeCall::replace_mt_safe(address instr_addr, address code_buffer) {
221-
assert(Patching_lock->is_locked() ||
221+
assert(CodeCache_lock->is_locked() ||
222222
SafepointSynchronize::is_at_safepoint(), "concurrent code patching");
223223
assert (instr_addr != nullptr, "illegal address for code patching");
224224

@@ -281,7 +281,7 @@ void NativeCall::set_destination_mt_safe(address dest) {
281281
debug_only(verify());
282282
// Make sure patching code is locked. No two threads can patch at the same
283283
// time but one may be executing this code.
284-
assert(Patching_lock->is_locked() || SafepointSynchronize::is_at_safepoint() ||
284+
assert(CodeCache_lock->is_locked() || SafepointSynchronize::is_at_safepoint() ||
285285
CompiledICLocker::is_safe(instruction_address()), "concurrent code patching");
286286
// Both C1 and C2 should now be generating code which aligns the patched address
287287
// to be within a single cache line.

src/hotspot/share/c1/c1_Runtime1.cpp

+4-8
Original file line numberDiff line numberDiff line change
@@ -880,7 +880,7 @@ static Klass* resolve_field_return_klass(const methodHandle& caller, int bci, TR
880880
// movl reg, [reg1 + <const>] (for field offsets)
881881
// jmp continue
882882
// <being_init offset> <bytes to copy> <bytes to skip>
883-
// patch_stub: jmp Runtim1::patch_code (through a runtime stub)
883+
// patch_stub: jmp Runtime1::patch_code (through a runtime stub)
884884
// jmp patch_site
885885
//
886886
// If the class is being initialized the patch body is rewritten and
@@ -1096,7 +1096,7 @@ JRT_ENTRY(void, Runtime1::patch_code(JavaThread* current, Runtime1::StubID stub_
10961096
// Now copy code back
10971097

10981098
{
1099-
MutexLocker ml_patch (current, Patching_lock, Mutex::_no_safepoint_check_flag);
1099+
MutexLocker ml_code (current, CodeCache_lock, Mutex::_no_safepoint_check_flag);
11001100
//
11011101
// Deoptimization may have happened while we waited for the lock.
11021102
// In that case we don't bother to do any patching we just return
@@ -1261,12 +1261,8 @@ JRT_ENTRY(void, Runtime1::patch_code(JavaThread* current, Runtime1::StubID stub_
12611261
}
12621262
}
12631263
}
1264-
}
1265-
1266-
// If we are patching in a non-perm oop, make sure the nmethod
1267-
// is on the right list.
1268-
{
1269-
MutexLocker ml_code (current, CodeCache_lock, Mutex::_no_safepoint_check_flag);
1264+
// If we are patching in a non-perm oop, make sure the nmethod
1265+
// is on the right list.
12701266
nmethod* nm = CodeCache::find_nmethod(caller_frame.pc());
12711267
guarantee(nm != nullptr, "only nmethods can contain non-perm oops");
12721268

src/hotspot/share/code/nmethod.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -2052,7 +2052,7 @@ bool nmethod::make_not_entrant() {
20522052
} // leave critical region under NMethodState_lock
20532053

20542054
#if INCLUDE_JVMCI
2055-
// Invalidate can't occur while holding the Patching lock
2055+
// Invalidate can't occur while holding the NMethodState_lock
20562056
JVMCINMethodData* nmethod_data = jvmci_nmethod_data();
20572057
if (nmethod_data != nullptr) {
20582058
nmethod_data->invalidate_nmethod_mirror(this);

src/hotspot/share/runtime/mutexLocker.cpp

-2
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@
3636

3737
// Mutexes used in the VM (see comment in mutexLocker.hpp):
3838

39-
Mutex* Patching_lock = nullptr;
4039
Mutex* NMethodState_lock = nullptr;
4140
Monitor* SystemDictionary_lock = nullptr;
4241
Mutex* InvokeMethodTypeTable_lock = nullptr;
@@ -231,7 +230,6 @@ void mutex_init() {
231230
MUTEX_DEFN(Metaspace_lock , PaddedMutex , nosafepoint-3);
232231
MUTEX_DEFN(MetaspaceCritical_lock , PaddedMonitor, nosafepoint-1);
233232

234-
MUTEX_DEFN(Patching_lock , PaddedMutex , nosafepoint); // used for safepointing and code patching.
235233
MUTEX_DEFN(MonitorDeflation_lock , PaddedMonitor, nosafepoint); // used for monitor deflation thread operations
236234
MUTEX_DEFN(Service_lock , PaddedMonitor, service); // used for service thread operations
237235

src/hotspot/share/runtime/mutexLocker.hpp

-1
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@
3131

3232
// Mutexes used in the VM.
3333

34-
extern Mutex* Patching_lock; // a lock used to guard code patching of compiled code
3534
extern Mutex* NMethodState_lock; // a lock used to guard a compiled method state
3635
extern Monitor* SystemDictionary_lock; // a lock on the system dictionary
3736
extern Mutex* InvokeMethodTypeTable_lock;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
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+
import java.lang.reflect.Method;
25+
import java.net.URL;
26+
import java.net.URLClassLoader;
27+
import java.util.ArrayList;
28+
29+
/**
30+
* @test
31+
* @bug 8340313
32+
* @summary Test that concurrent patching of oop immediates is thread-safe in C1.
33+
* @run main/othervm/timeout=480 -Xcomp -XX:CompileCommand=compileonly,TestConcurrentPatching::* -XX:TieredStopAtLevel=1 TestConcurrentPatching
34+
*/
35+
36+
class MyClass { }
37+
38+
class Holder {
39+
public static final MyClass OBJ1 = null;
40+
public static final MyClass OBJ2 = null;
41+
public static final MyClass OBJ3 = null;
42+
public static final MyClass OBJ4 = null;
43+
public static final MyClass OBJ5 = null;
44+
public static final MyClass OBJ6 = null;
45+
public static final MyClass OBJ7 = null;
46+
public static final MyClass OBJ8 = null;
47+
public static final MyClass OBJ9 = null;
48+
public static final MyClass OBJ10 = null;
49+
public static final MyClass OBJ11 = null;
50+
public static final MyClass OBJ12 = null;
51+
public static final MyClass OBJ13 = null;
52+
public static final MyClass OBJ14 = null;
53+
public static final MyClass OBJ15 = null;
54+
public static final MyClass OBJ16 = null;
55+
public static final MyClass OBJ17 = null;
56+
public static final MyClass OBJ18 = null;
57+
public static final MyClass OBJ19 = null;
58+
public static final MyClass OBJ20 = null;
59+
}
60+
61+
public class TestConcurrentPatching {
62+
// Increase to 100_000 for a good chance of reproducing the issue with a single run
63+
static final int ITERATIONS = 1000;
64+
65+
static Object field;
66+
67+
// 'Holder' class is unloaded on first execution and therefore field
68+
// accesses require patching when the method is C1 compiled (with -Xcomp).
69+
public static void test() {
70+
field = Holder.OBJ1;
71+
field = Holder.OBJ2;
72+
field = Holder.OBJ3;
73+
field = Holder.OBJ4;
74+
field = Holder.OBJ5;
75+
field = Holder.OBJ6;
76+
field = Holder.OBJ7;
77+
field = Holder.OBJ8;
78+
field = Holder.OBJ9;
79+
field = Holder.OBJ10;
80+
field = Holder.OBJ11;
81+
field = Holder.OBJ12;
82+
field = Holder.OBJ13;
83+
field = Holder.OBJ14;
84+
field = Holder.OBJ15;
85+
field = Holder.OBJ16;
86+
field = Holder.OBJ17;
87+
field = Holder.OBJ18;
88+
field = Holder.OBJ19;
89+
field = Holder.OBJ20;
90+
}
91+
92+
// Appendix of invokedynamic call sites is unloaded on first execution and
93+
// therefore requires patching when the method is C1 compiled (with -Xcomp).
94+
public static void testIndy() throws Throwable {
95+
field = (Runnable) () -> { };
96+
field = (Runnable) () -> { };
97+
field = (Runnable) () -> { };
98+
field = (Runnable) () -> { };
99+
field = (Runnable) () -> { };
100+
field = (Runnable) () -> { };
101+
field = (Runnable) () -> { };
102+
field = (Runnable) () -> { };
103+
field = (Runnable) () -> { };
104+
field = (Runnable) () -> { };
105+
}
106+
107+
// Run 'test' by multiple threads to trigger concurrent patching of field accesses
108+
static void runWithThreads(Method method) {
109+
ArrayList<Thread> threads = new ArrayList<>();
110+
for (int threadIdx = 0; threadIdx < 10; threadIdx++) {
111+
threads.add(new Thread(() -> {
112+
try {
113+
method.invoke(null);
114+
} catch (Exception e) {
115+
throw new IllegalStateException(e);
116+
}
117+
}));
118+
}
119+
threads.forEach(Thread::start);
120+
threads.forEach(t -> {
121+
try {
122+
t.join();
123+
} catch (Throwable e) {
124+
throw new IllegalStateException(e);
125+
}
126+
});
127+
}
128+
129+
public static void main(String[] args) throws Exception {
130+
Class<?> thisClass = TestConcurrentPatching.class;
131+
ClassLoader defaultLoader = thisClass.getClassLoader();
132+
URL classesDir = thisClass.getProtectionDomain().getCodeSource().getLocation();
133+
134+
// Load the test class multiple times with a separate class loader to make sure
135+
// that the 'Holder' class is unloaded for each compilation of method 'test'
136+
// and that the appendix of the invokedynamic call site is unloaded for each
137+
// compilation of method 'testIndy'.
138+
for (int i = 0; i < ITERATIONS; ++i) {
139+
URLClassLoader myLoader = URLClassLoader.newInstance(new URL[] {classesDir}, defaultLoader.getParent());
140+
Class<?> testClass = Class.forName(thisClass.getCanonicalName(), true, myLoader);
141+
runWithThreads(testClass.getDeclaredMethod("test"));
142+
runWithThreads(testClass.getDeclaredMethod("testIndy"));
143+
}
144+
}
145+
}

0 commit comments

Comments
 (0)