stan/jdk-24
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

8340313: Crash due to invalid oop in nmethod after C1 patching

Browse Source 
Reviewed-by: tschatzl, kvn, dlong
This commit is contained in:
Tobias Hartmann 2024-10-17 05:03:09 +00:00
parent 3da6890081
commit 58d39c317e
10 changed files with 160 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp
View File

@ -78,7 +78,7 @@ address NativeCall::destination() const {
//
// Used in the runtime linkage of calls; see class CompiledIC.
void NativeCall::set_destination_mt_safe(address dest) {
assert((Patching_lock->is_locked() || SafepointSynchronize::is_at_safepoint()) ||
assert((CodeCache_lock->is_locked() || SafepointSynchronize::is_at_safepoint()) ||
CompiledICLocker::is_safe(addr_at(0)),
"concurrent code patching");

4
src/hotspot/cpu/ppc/nativeInst_ppc.cpp
View File

@ -92,10 +92,10 @@ address NativeCall::destination() const {
// Used in the runtime linkage of calls; see class CompiledIC.
//
// Add parameter assert_lock to switch off assertion
// during code generation, where no patching lock is needed.
// during code generation, where no lock is needed.
void NativeCall::set_destination_mt_safe(address dest, bool assert_lock) {
assert(!assert_lock ||
(Patching_lock->is_locked() || SafepointSynchronize::is_at_safepoint()) ||
(CodeCache_lock->is_locked() || SafepointSynchronize::is_at_safepoint()) ||
CompiledICLocker::is_safe(addr_at(0)),
"concurrent code patching");

6
src/hotspot/cpu/riscv/nativeInst_riscv.cpp
View File

@ -215,10 +215,10 @@ void NativeShortCall::print() {
// Used in the runtime linkage of calls; see class CompiledIC.
//
// Add parameter assert_lock to switch off assertion
// during code generation, where no patching lock is needed.
// during code generation, where no lock is needed.
bool NativeShortCall::set_destination_mt_safe(address dest, bool assert_lock) {
assert(!assert_lock ||
(Patching_lock->is_locked() || SafepointSynchronize::is_at_safepoint()) ||
(CodeCache_lock->is_locked() || SafepointSynchronize::is_at_safepoint()) ||
CompiledICLocker::is_safe(instruction_address()),
"concurrent code patching");
@ -386,7 +386,7 @@ void NativeFarCall::print() {
bool NativeFarCall::set_destination_mt_safe(address dest, bool assert_lock) {
assert(NativeFarCall::is_at(addr_at(0)), "unexpected code at call site");
assert(!assert_lock ||
(Patching_lock->is_locked() || SafepointSynchronize::is_at_safepoint()) ||
(CodeCache_lock->is_locked() || SafepointSynchronize::is_at_safepoint()) ||
CompiledICLocker::is_safe(addr_at(0)),
"concurrent code patching");

4
src/hotspot/cpu/s390/nativeInst_s390.cpp
View File

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

4
src/hotspot/cpu/x86/nativeInst_x86.cpp
View File

@ -81,7 +81,7 @@ void NativeCall::insert(address code_pos, address entry) {
// (spinlock). Then patches the last byte, and then atomically replaces
// the jmp's with the first 4 byte of the new instruction.
void NativeCall::replace_mt_safe(address instr_addr, address code_buffer) {
assert(Patching_lock->is_locked() ||
assert(CodeCache_lock->is_locked() ||
SafepointSynchronize::is_at_safepoint(), "concurrent code patching");
assert (instr_addr != nullptr, "illegal address for code patching");
@ -144,7 +144,7 @@ void NativeCall::set_destination_mt_safe(address dest) {
debug_only(verify());
// Make sure patching code is locked. No two threads can patch at the same
// time but one may be executing this code.
assert(Patching_lock->is_locked() || SafepointSynchronize::is_at_safepoint() ||
assert(CodeCache_lock->is_locked() || SafepointSynchronize::is_at_safepoint() ||
CompiledICLocker::is_safe(instruction_address()), "concurrent code patching");
// Both C1 and C2 should now be generating code which aligns the patched address
// to be within a single cache line.

12
src/hotspot/share/c1/c1_Runtime1.cpp
View File

@ -887,7 +887,7 @@ static Klass* resolve_field_return_klass(const methodHandle& caller, int bci, TR
// movl reg, [reg1 + <const>] (for field offsets)
// jmp continue
// <being_init offset> <bytes to copy> <bytes to skip>
// patch_stub: jmp Runtim1::patch_code (through a runtime stub)
// patch_stub: jmp Runtime1::patch_code (through a runtime stub)
// jmp patch_site
//
// If the class is being initialized the patch body is rewritten and
@ -1103,7 +1103,7 @@ JRT_ENTRY(void, Runtime1::patch_code(JavaThread* current, C1StubId stub_id ))
// Now copy code back
{
MutexLocker ml_patch (current, Patching_lock, Mutex::_no_safepoint_check_flag);
MutexLocker ml_code (current, CodeCache_lock, Mutex::_no_safepoint_check_flag);
//
// Deoptimization may have happened while we waited for the lock.
// In that case we don't bother to do any patching we just return
@ -1268,12 +1268,8 @@ JRT_ENTRY(void, Runtime1::patch_code(JavaThread* current, C1StubId stub_id ))
}
}
}
}
// If we are patching in a non-perm oop, make sure the nmethod
// is on the right list.
{
MutexLocker ml_code (current, CodeCache_lock, Mutex::_no_safepoint_check_flag);
// If we are patching in a non-perm oop, make sure the nmethod
// is on the right list.
nmethod* nm = CodeCache::find_nmethod(caller_frame.pc());
guarantee(nm != nullptr, "only nmethods can contain non-perm oops");

2
src/hotspot/share/code/nmethod.cpp
View File

@ -2048,7 +2048,7 @@ bool nmethod::make_not_entrant() {
} // leave critical region under NMethodState_lock
#if INCLUDE_JVMCI
// Invalidate can't occur while holding the Patching lock
// Invalidate can't occur while holding the NMethodState_lock
JVMCINMethodData* nmethod_data = jvmci_nmethod_data();
if (nmethod_data != nullptr) {
nmethod_data->invalidate_nmethod_mirror(this);

2
src/hotspot/share/runtime/mutexLocker.cpp
View File

@ -36,7 +36,6 @@
// Mutexes used in the VM (see comment in mutexLocker.hpp):
Mutex* Patching_lock = nullptr;
Mutex* NMethodState_lock = nullptr;
Monitor* SystemDictionary_lock = nullptr;
Mutex* InvokeMethodTypeTable_lock = nullptr;
@ -233,7 +232,6 @@ void mutex_init() {
MUTEX_DEFN(Metaspace_lock , PaddedMutex , nosafepoint-3);
MUTEX_DEFN(MetaspaceCritical_lock , PaddedMonitor, nosafepoint-1);
MUTEX_DEFN(Patching_lock , PaddedMutex , nosafepoint); // used for safepointing and code patching.
MUTEX_DEFN(MonitorDeflation_lock , PaddedMonitor, nosafepoint); // used for monitor deflation thread operations
MUTEX_DEFN(Service_lock , PaddedMonitor, service); // used for service thread operations
MUTEX_DEFN(Notification_lock , PaddedMonitor, service); // used for notification thread operations

1
src/hotspot/share/runtime/mutexLocker.hpp
View File

@ -31,7 +31,6 @@
// Mutexes used in the VM.
extern Mutex* Patching_lock; // a lock used to guard code patching of compiled code
extern Mutex* NMethodState_lock; // a lock used to guard a compiled method state
extern Monitor* SystemDictionary_lock; // a lock on the system dictionary
extern Mutex* InvokeMethodTypeTable_lock;

145
test/hotspot/jtreg/compiler/c1/TestConcurrentPatching.java Normal file
View File

@ -0,0 +1,145 @@
/*
* Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
import java.lang.reflect.Method;
import java.net.URL;
import java.net.URLClassLoader;
import java.util.ArrayList;
/**
* @test
* @bug 8340313
* @summary Test that concurrent patching of oop immediates is thread-safe in C1.
* @run main/othervm/timeout=480 -Xcomp -XX:CompileCommand=compileonly,TestConcurrentPatching::* -XX:TieredStopAtLevel=1 TestConcurrentPatching
*/
class MyClass { }
class Holder {
public static final MyClass OBJ1 = null;
public static final MyClass OBJ2 = null;
public static final MyClass OBJ3 = null;
public static final MyClass OBJ4 = null;
public static final MyClass OBJ5 = null;
public static final MyClass OBJ6 = null;
public static final MyClass OBJ7 = null;
public static final MyClass OBJ8 = null;
public static final MyClass OBJ9 = null;
public static final MyClass OBJ10 = null;
public static final MyClass OBJ11 = null;
public static final MyClass OBJ12 = null;
public static final MyClass OBJ13 = null;
public static final MyClass OBJ14 = null;
public static final MyClass OBJ15 = null;
public static final MyClass OBJ16 = null;
public static final MyClass OBJ17 = null;
public static final MyClass OBJ18 = null;
public static final MyClass OBJ19 = null;
public static final MyClass OBJ20 = null;
}
public class TestConcurrentPatching {
// Increase to 100_000 for a good chance of reproducing the issue with a single run
static final int ITERATIONS = 1000;
static Object field;
// 'Holder' class is unloaded on first execution and therefore field
// accesses require patching when the method is C1 compiled (with -Xcomp).
public static void test() {
field = Holder.OBJ1;
field = Holder.OBJ2;
field = Holder.OBJ3;
field = Holder.OBJ4;
field = Holder.OBJ5;
field = Holder.OBJ6;
field = Holder.OBJ7;
field = Holder.OBJ8;
field = Holder.OBJ9;
field = Holder.OBJ10;
field = Holder.OBJ11;
field = Holder.OBJ12;
field = Holder.OBJ13;
field = Holder.OBJ14;
field = Holder.OBJ15;
field = Holder.OBJ16;
field = Holder.OBJ17;
field = Holder.OBJ18;
field = Holder.OBJ19;
field = Holder.OBJ20;
}
// Appendix of invokedynamic call sites is unloaded on first execution and
// therefore requires patching when the method is C1 compiled (with -Xcomp).
public static void testIndy() throws Throwable {
field = (Runnable) () -> { };
field = (Runnable) () -> { };
field = (Runnable) () -> { };
field = (Runnable) () -> { };
field = (Runnable) () -> { };
field = (Runnable) () -> { };
field = (Runnable) () -> { };
field = (Runnable) () -> { };
field = (Runnable) () -> { };
field = (Runnable) () -> { };
}
// Run 'test' by multiple threads to trigger concurrent patching of field accesses
static void runWithThreads(Method method) {
ArrayList<Thread> threads = new ArrayList<>();
for (int threadIdx = 0; threadIdx < 10; threadIdx++) {
threads.add(new Thread(() -> {
try {
method.invoke(null);
} catch (Exception e) {
throw new IllegalStateException(e);
}
}));
}
threads.forEach(Thread::start);
threads.forEach(t -> {
try {
t.join();
} catch (Throwable e) {
throw new IllegalStateException(e);
}
});
}
public static void main(String[] args) throws Exception {
Class<?> thisClass = TestConcurrentPatching.class;
ClassLoader defaultLoader = thisClass.getClassLoader();
URL classesDir = thisClass.getProtectionDomain().getCodeSource().getLocation();
// Load the test class multiple times with a separate class loader to make sure
// that the 'Holder' class is unloaded for each compilation of method 'test'
// and that the appendix of the invokedynamic call site is unloaded for each
// compilation of method 'testIndy'.
for (int i = 0; i < ITERATIONS; ++i) {
URLClassLoader myLoader = URLClassLoader.newInstance(new URL[] {classesDir}, defaultLoader.getParent());
Class<?> testClass = Class.forName(thisClass.getCanonicalName(), true, myLoader);
runWithThreads(testClass.getDeclaredMethod("test"));
runWithThreads(testClass.getDeclaredMethod("testIndy"));
}
}
}