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

src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp

@@ -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");
 

src/hotspot/cpu/ppc/nativeInst_ppc.cpp

@@ -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");
 

src/hotspot/cpu/riscv/nativeInst_riscv.cpp

@@ -68,10 +68,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");
 

src/hotspot/cpu/s390/nativeInst_s390.cpp

@@ -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;

src/hotspot/cpu/x86/nativeInst_x86.cpp

@@ -218,7 +218,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");
 
@@ -281,7 +281,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.

src/hotspot/share/c1/c1_Runtime1.cpp

@@ -880,7 +880,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
@@ -1096,7 +1096,7 @@ JRT_ENTRY(void, Runtime1::patch_code(JavaThread* current, Runtime1::StubID stub_
  // 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
@@ -1261,12 +1261,8 @@ JRT_ENTRY(void, Runtime1::patch_code(JavaThread* current, Runtime1::StubID stub_
  }
  }
  }
- }
-
- // 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");
 

src/hotspot/share/code/nmethod.cpp

@@ -2052,7 +2052,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);

src/hotspot/share/runtime/mutexLocker.cpp

@@ -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;
@@ -231,7 +230,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
 

src/hotspot/share/runtime/mutexLocker.hpp

@@ -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;

test/hotspot/jtreg/compiler/c1/TestConcurrentPatching.java

@@ -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"));
+ }
+ }
+}