diff --git a/src/hotspot/share/prims/jni.cpp b/src/hotspot/share/prims/jni.cpp index d6a435d34d1..318f40374a6 100644 --- a/src/hotspot/share/prims/jni.cpp +++ b/src/hotspot/share/prims/jni.cpp @@ -2835,6 +2835,10 @@ JNI_ENTRY(void*, jni_GetPrimitiveArrayCritical(JNIEnv *env, jarray array, jboole Handle a(thread, JNIHandles::resolve_non_null(array)); assert(a->is_typeArray(), "just checking"); + // We must defer JVM TI suspension while we have access to a Java object + // as it could surprise the debugger if we mutate it concurrently whilst + // logically suspended. + thread->enter_jni_deferred_suspension(); // Pin object Universe::heap()->pin_object(thread, a()); @@ -2852,6 +2856,7 @@ JNI_ENTRY(void, jni_ReleasePrimitiveArrayCritical(JNIEnv *env, jarray array, voi HOTSPOT_JNI_RELEASEPRIMITIVEARRAYCRITICAL_ENTRY(env, array, carray, mode); // Unpin object Universe::heap()->unpin_object(thread, JNIHandles::resolve_non_null(array)); + thread->exit_jni_deferred_suspension(); HOTSPOT_JNI_RELEASEPRIMITIVEARRAYCRITICAL_RETURN(); JNI_END @@ -2859,6 +2864,13 @@ JNI_END JNI_ENTRY(const jchar*, jni_GetStringCritical(JNIEnv *env, jstring string, jboolean *isCopy)) HOTSPOT_JNI_GETSTRINGCRITICAL_ENTRY(env, string, (uintptr_t *) isCopy); oop s = JNIHandles::resolve_non_null(string); + + // We must defer JVM TI suspension while we have access to a Java object. + // Even if we are taking a private copy we must not be considered + // suspended as the debugger could be mutating the string we are about + // to copy. + thread->enter_jni_deferred_suspension(); + jchar* ret; if (!java_lang_String::is_latin1(s)) { typeArrayHandle s_value(thread, java_lang_String::value(s)); @@ -2879,6 +2891,10 @@ JNI_ENTRY(const jchar*, jni_GetStringCritical(JNIEnv *env, jstring string, jbool ret[i] = ((jchar) s_value->byte_at(i)) & 0xff; } ret[s_len] = 0; + } else { + // If we return null there should not be a paired release operation + // so we have to cancel suspension deferral here. + thread->exit_jni_deferred_suspension(); } if (isCopy != nullptr) *isCopy = JNI_TRUE; } @@ -2904,6 +2920,7 @@ JNI_ENTRY(void, jni_ReleaseStringCritical(JNIEnv *env, jstring str, const jchar // Unpin value array Universe::heap()->unpin_object(thread, value); } + thread->exit_jni_deferred_suspension(); HOTSPOT_JNI_RELEASESTRINGCRITICAL_RETURN(); JNI_END diff --git a/src/hotspot/share/runtime/handshake.cpp b/src/hotspot/share/runtime/handshake.cpp index b54068d65d6..885f46e530d 100644 --- a/src/hotspot/share/runtime/handshake.cpp +++ b/src/hotspot/share/runtime/handshake.cpp @@ -83,8 +83,10 @@ class HandshakeOperation : public CHeapObj { int32_t pending_threads() { return AtomicAccess::load(&_pending_threads); } const char* name() { return _handshake_cl->name(); } bool is_async() { return _handshake_cl->is_async(); } - bool is_suspend() { return _handshake_cl->is_suspend(); } + bool is_self_suspend() { return _handshake_cl->is_self_suspend(); } + bool is_suspend_request() { return _handshake_cl->is_suspend_request(); } bool is_async_exception() { return _handshake_cl->is_async_exception(); } + bool is_enabled() { return _handshake_cl->is_enabled(_target); } }; class AsyncHandshakeOperation : public HandshakeOperation { @@ -472,18 +474,24 @@ void Handshake::execute(AsyncHandshakeClosure* hs_cl, JavaThread* target) { } // Filters + +// op is enabled and can be executed by the current thread rather than the target. static bool non_self_executable_filter(HandshakeOperation* op) { - return !op->is_async(); + return !op->is_async() && op->is_enabled(); } +// op is not an async-exception op static bool no_async_exception_filter(HandshakeOperation* op) { return !op->is_async_exception(); } +// op is an async-exception op static bool async_exception_filter(HandshakeOperation* op) { return op->is_async_exception(); } +// op is not any kind of suspend op, nor an async-exception op static bool no_suspend_no_async_exception_filter(HandshakeOperation* op) { - return !op->is_suspend() && !op->is_async_exception(); + return !op->is_self_suspend() && !op->is_suspend_request() && !op->is_async_exception(); } +// All ops static bool all_ops_filter(HandshakeOperation* op) { return true; } @@ -521,8 +529,12 @@ HandshakeOperation* HandshakeState::get_op_for_self(bool allow_suspend, bool che assert(_lock.owned_by_self(), "Lock must be held"); assert(allow_suspend || !check_async_exception, "invalid case"); #if INCLUDE_JVMTI - if (allow_suspend && (_handshakee->is_disable_suspend() || _handshakee->is_vthread_transition_disabler())) { - // filter out suspend operations while JavaThread can not be suspended + // Filter out suspend operations while JavaThread can not be suspended. + // Potentially this could be folded into the `is_enabled` state of the operation + // and filtered directly through _queue.peek, but the incoming `allow_suspend` + // complicates that so we just maintain the explicit checks for now. + if (allow_suspend && (_handshakee->is_disable_suspend() || _handshakee->is_vthread_transition_disabler() || + _handshakee->jni_deferred_suspension())) { allow_suspend = false; } #endif @@ -565,12 +577,16 @@ void HandshakeState::clean_async_exception_operation() { } } +// Returns true if there is an enabled op that the current thread can execute +// on behalf of the handshakee. bool HandshakeState::have_non_self_executable_operation() { assert(_handshakee != Thread::current(), "Must not be called by self"); assert(_lock.owned_by_self(), "Lock must be held"); return _queue.contains(non_self_executable_filter); } +// Returns an enabled op that the current thread can execute +// on behalf of the handshakee. HandshakeOperation* HandshakeState::get_op() { assert(_handshakee != Thread::current(), "Must not be called by self"); assert(_lock.owned_by_self(), "Lock must be held"); @@ -683,7 +699,7 @@ HandshakeState::ProcessResult HandshakeState::try_process(HandshakeOperation* ma return HandshakeState::_not_safe; } - // Claim the mutex if there still an operation to be executed. + // Claim the mutex if there still an enabled operation to be executed. if (!claim_handshake()) { return HandshakeState::_claim_failed; } @@ -699,8 +715,13 @@ HandshakeState::ProcessResult HandshakeState::try_process(HandshakeOperation* ma Thread* current_thread = Thread::current(); HandshakeOperation* op = get_op(); + // It is possible that since we claimed the handshake the op has + // transitioned to a disabled state and so won't be returned by get_op. + if (op == nullptr) { + return HandshakeState::_no_operation; + } - assert(op != nullptr, "Must have an op"); + assert(op->is_enabled(), "Should not reach here with a disabled op"); assert(SafepointMechanism::local_poll_armed(_handshakee), "Must be"); assert(op->_target == nullptr || _handshakee == op->_target, "Wrong thread"); diff --git a/src/hotspot/share/runtime/handshake.hpp b/src/hotspot/share/runtime/handshake.hpp index c764bbcfcd2..cfdaf2e82df 100644 --- a/src/hotspot/share/runtime/handshake.hpp +++ b/src/hotspot/share/runtime/handshake.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017, 2025, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2017, 2026, 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 @@ -50,8 +50,10 @@ class HandshakeClosure : public ThreadClosure, public CHeapObj { virtual ~HandshakeClosure() {} const char* name() const { return _name; } virtual bool is_async() { return false; } - virtual bool is_suspend() { return false; } + virtual bool is_self_suspend() { return false; } + virtual bool is_suspend_request() { return false; } virtual bool is_async_exception() { return false; } + virtual bool is_enabled(Thread* target) { return true; } virtual void do_thread(Thread* thread) = 0; }; @@ -99,7 +101,6 @@ class HandshakeState { Monitor _lock; // Set to the thread executing the handshake operation. Thread* volatile _active_handshaker; - bool claim_handshake(); bool possibly_can_process_handshake(); bool can_process_handshake(); diff --git a/src/hotspot/share/runtime/javaThread.cpp b/src/hotspot/share/runtime/javaThread.cpp index 5a29a668a54..ec1c8f64619 100644 --- a/src/hotspot/share/runtime/javaThread.cpp +++ b/src/hotspot/share/runtime/javaThread.cpp @@ -469,6 +469,7 @@ JavaThread::JavaThread(MemTag mem_tag) : _exception_handler_pc(nullptr), _jni_active_critical(0), + _jni_deferred_suspension_count(0), _pending_jni_exception_check_fn(nullptr), _depth_first_number(0), diff --git a/src/hotspot/share/runtime/javaThread.hpp b/src/hotspot/share/runtime/javaThread.hpp index bc6c0a3a4fd..5dd84fcc01d 100644 --- a/src/hotspot/share/runtime/javaThread.hpp +++ b/src/hotspot/share/runtime/javaThread.hpp @@ -458,9 +458,12 @@ class JavaThread: public Thread { volatile address _exception_handler_pc; // PC for handler of exception private: - // support for JNI critical regions + // support for JNI critical regions interaction with GC jint _jni_active_critical; // count of entries into JNI critical region + // support for JNI critical regions deferral of JVM TI suspension + jint _jni_deferred_suspension_count; + // Checked JNI: function name requires exception check char* _pending_jni_exception_check_fn; @@ -980,10 +983,19 @@ public: _jni_active_critical--; assert(_jni_active_critical >= 0, "JNI critical nesting problem?"); } - // Atomic version; invoked by a thread other than the owning thread. bool in_critical_atomic() { return AtomicAccess::load(&_jni_active_critical) > 0; } + bool jni_deferred_suspension() { return AtomicAccess::load(&_jni_deferred_suspension_count); } + inline void enter_jni_deferred_suspension(); + void exit_jni_deferred_suspension() { + precond(Thread::current() == this); + int sc = AtomicAccess::load(&_jni_deferred_suspension_count); + AtomicAccess::store(&_jni_deferred_suspension_count, sc - 1); + assert(_jni_deferred_suspension_count >= 0, + "JNI deferred suspension nesting problem?"); + } + // Checked JNI: is the programmer required to check for exceptions, if so specify // which function name. Returning to a Java frame should implicitly clear the // pending check, this is done for Native->Java transitions (i.e. user JNI code). diff --git a/src/hotspot/share/runtime/javaThread.inline.hpp b/src/hotspot/share/runtime/javaThread.inline.hpp index 2c2ed2da2fa..5b4556b5666 100644 --- a/src/hotspot/share/runtime/javaThread.inline.hpp +++ b/src/hotspot/share/runtime/javaThread.inline.hpp @@ -196,6 +196,14 @@ void JavaThread::enter_critical() { _jni_active_critical++; } +void JavaThread::enter_jni_deferred_suspension() { + precond(JavaThread::current() == this); + assert(_thread_state != _thread_in_native && _thread_state != _thread_blocked, + "Must not defer suspension when handshake-safe"); + int sc = AtomicAccess::load(&_jni_deferred_suspension_count); + AtomicAccess::store(&_jni_deferred_suspension_count, sc + 1); +} + inline void JavaThread::set_done_attaching_via_jni() { _jni_attach_state = _attached_via_jni; OrderAccess::fence(); diff --git a/src/hotspot/share/runtime/suspendResumeManager.cpp b/src/hotspot/share/runtime/suspendResumeManager.cpp index 3408d763e57..3181c0f78aa 100644 --- a/src/hotspot/share/runtime/suspendResumeManager.cpp +++ b/src/hotspot/share/runtime/suspendResumeManager.cpp @@ -48,7 +48,7 @@ public: current->set_thread_state(jts); current->suspend_resume_manager()->set_async_suspend_handshake(false); } - virtual bool is_suspend() { return true; } + virtual bool is_self_suspend() { return true; } }; // This is the closure that synchronously honors the suspend request. @@ -64,6 +64,16 @@ public: _did_suspend = target->suspend_resume_manager()->suspend_with_handshake(_register_vthread_SR); } bool did_suspend() { return _did_suspend; } + virtual bool is_suspend_request() { return true; } + + // If the target is in a JNI deferred suspension region, then we cannot + // process this operation. This must be checked with the HandshakeState lock + // held, which together with the fact the target only enters a deferred + // region from a handshake-unsafe state, means we cannot race with the + // target entering that region. + virtual bool is_enabled(Thread* target) { + return !JavaThread::cast(target)->jni_deferred_suspension(); + } }; void SuspendResumeManager::set_suspended(bool is_suspend, bool register_vthread_SR) { diff --git a/test/hotspot/jtreg/runtime/jni/critical/SuspendInCritical.java b/test/hotspot/jtreg/runtime/jni/critical/SuspendInCritical.java new file mode 100644 index 00000000000..47ece0f1857 --- /dev/null +++ b/test/hotspot/jtreg/runtime/jni/critical/SuspendInCritical.java @@ -0,0 +1,212 @@ +/* + * Copyright (c) 2026, 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. + */ + +/** + * @test + * @bug 8373839 + * @requires vm.jvmti + * @library /test/lib + * @run main/othervm/native -agentlib:SuspendInCritical SuspendInCritical 1 + * @run main/othervm/native -agentlib:SuspendInCritical SuspendInCritical 2 + * @run main/othervm/native -agentlib:SuspendInCritical SuspendInCritical 3 + */ +import jdk.test.lib.Asserts; + +import java.util.concurrent.CountDownLatch; + +public class SuspendInCritical { + + static { + System.loadLibrary("SuspendInCritical"); + } + + static native void suspendThread(Thread t); + static native void resumeThread(Thread t); + static native boolean isSuspended(Thread t); + + static native void doCritical(byte[] b, String str); + static native void leaveCriticalNative(); + static native long getNativeCounter(); + + static volatile boolean suspendStarted = false; + static volatile boolean suspendCompleted = false; + static volatile boolean canTerminate = false; + + /* + * Incoming arg sets the mode: + * - 1: GetPrimitiveArrayCritical only + * - 2: GetStringCritical only + * - 3: Do both so we have a nested critical region + */ + public static void main(String[] args) throws Throwable { + int mode = Integer.parseInt(args[0]); + final byte[] bytes = (mode == 1 || mode == 3) ? new byte[16] : null; + final String str = (mode == 2 || mode == 3) ? "A String" : null; + + final Thread t = new Thread() { + public void run() { + System.out.println("CriticalThread calling native ..."); + doCritical(bytes, str); + System.out.println("CriticalThread return from native ..."); + // delay termination so we can check the suspend state + // from another thread + while (!canTerminate) { + delay(10); + } + System.out.println("CriticalThread terminating"); + } + }; + t.setName("CriticalThread"); + + System.out.println("main thread starting CriticalThread"); + + // Start the target thread. It will enter a JNI critical region + // and stay executing native code until released. + t.start(); + + // Check that the counter is progressing + checkNativeCounter(); + + System.out.println("main thread saw CriticalThread executing and is starting SuspenderThread"); + + // Now start the suspender thread. + Thread s = new Thread() { + public void run() { + suspendStarted = true; + System.out.println("SuspenderThread calling suspend ..."); + // This will block until t is out of the critical region + suspendThread(t); + suspendCompleted = true; + + System.out.println("SuspenderThread checking suspend ..."); + // Verify t is suspended + Asserts.assertTrue(isSuspended(t), "not suspended"); + + System.out.println("SuspenderThread calling resume ..."); + // Resume t + resumeThread(t); + + System.out.println("SuspenderThread checking not suspended ..."); + Asserts.assertFalse(isSuspended(t), "suspended"); + + System.out.println("SuspenderThread allowing target to terminate ..."); + // Allow t to terminate + canTerminate = true; + System.out.println("SuspenderThread terminatng"); + } + }; + s.setName("SuspenderThread"); + s.start(); + + while (!suspendStarted) { + delay(2); + } + + // Check suspender is blocked + checkSuspenderIsBlocked(); + + System.out.println("main thread confirms SuspenderThread is blocked in suspend()"); + + // Check target is still progressing + checkNativeCounter(); + + System.out.println("main thread saw CriticalThread still executing and has enabled the upcall"); + + // Allow target to proceed to Java upcall + leaveCriticalNative(); + + // Wait till target is in Java upcall + waitForUpcall(); + + System.out.println("main thread saw CriticalThread in upcall"); + + // Check suspender is blocked + checkSuspenderIsBlocked(); + + System.out.println("main thread confirms SuspenderThread is still blocked in suspend()"); + + // Check target is still executing + checkUpcallCount(); + + System.out.println("main thread confirms CriticalThread is still executing and will let it return and be suspended"); + + // Check suspender is still blocked + checkSuspenderIsBlocked(); + + // Allow target to return from Java and exit critical + upcallDone = true; + + // The suspension can now proceed, then both threads will terminate + t.join(); + s.join(); + System.out.println("main thread terminating"); + } + + static void checkNativeCounter() { + long counter = getNativeCounter(); + // If the counter never progresses then the test will timeout + while (counter == getNativeCounter()) { + delay(5); + } + } + + static void checkSuspenderIsBlocked() { + delay(200); + Asserts.assertTrue(!suspendCompleted,"Unexpected suspend completion"); + } + + static volatile boolean upcallDone = false; + static volatile long upcallCounter = 0; + static CountDownLatch inUpcall = new CountDownLatch(1); + + static void waitForUpcall() throws InterruptedException { + inUpcall.await(); + } + + static void upcall() { + inUpcall.countDown(); + System.out.println("CriticalThread is executing upcall"); + while (!upcallDone) { + upcallCounter++; + delay(1); + } + } + + static void checkUpcallCount() { + long count = upcallCounter; + // If the counter never progresses then the test will timeout + while (count == upcallCounter) { + delay(5); + } + } + + static void delay(long ms) { + try { + Thread.sleep(ms); + } + catch (InterruptedException ex) { + throw new Error("interrupted!"); + } + } + +} diff --git a/test/hotspot/jtreg/runtime/jni/critical/libSuspendInCritical.cpp b/test/hotspot/jtreg/runtime/jni/critical/libSuspendInCritical.cpp new file mode 100644 index 00000000000..0c5e95de611 --- /dev/null +++ b/test/hotspot/jtreg/runtime/jni/critical/libSuspendInCritical.cpp @@ -0,0 +1,146 @@ +/* + * Copyright (c) 2026, 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. + */ + +#include +#include +#include +#include +#include + +#include "jvmti.h" +#include "jvmti_common.hpp" + +static jvmtiEnv* jvmti = nullptr; + +static std::atomic stay_in_critical_native{JNI_TRUE}; +static std::atomic native_counter{0}; + +extern "C" { + +JNIEXPORT void JNICALL +Java_SuspendInCritical_suspendThread(JNIEnv* jni, jclass cls, jthread t) { + suspend_thread(jvmti, jni, t); +} + +JNIEXPORT void JNICALL +Java_SuspendInCritical_resumeThread(JNIEnv* jni, jclass cls, jthread t) { + resume_thread(jvmti, jni, t); +} + +JNIEXPORT jboolean JNICALL +Java_SuspendInCritical_isSuspended(JNIEnv* jni, jclass cls, jthread t) { + jint state = get_thread_state(jvmti, jni, t); + return (state & JVMTI_THREAD_STATE_SUSPENDED) != 0; +} + +JNIEXPORT void JNICALL +Java_SuspendInCritical_leaveCriticalNative(JNIEnv* jni, jclass cls) { + stay_in_critical_native = JNI_FALSE; +} + +JNIEXPORT jlong JNICALL +Java_SuspendInCritical_getNativeCounter(JNIEnv* jni, jclass cls) { + return native_counter; +} + +JNIEXPORT void JNICALL + Java_SuspendInCritical_doCritical(JNIEnv* jni, jclass cls, jbyteArray bytes, jstring str) { + jboolean is_copy = JNI_FALSE; + jbyte* b; + if (bytes != nullptr) { + LOG("CriticalThread doing GetPrimitiveArrayCritical\n"); + b = (jbyte*) jni->GetPrimitiveArrayCritical(bytes, &is_copy); + if (b == nullptr) { + jni->FatalError("GetPrimitiveArrayCritical returned null!"); + } + } + + const jchar* c; + if (str != nullptr) { + LOG("CriticalThread doing GetStringCritical\n"); + c = jni->GetStringCritical(str, &is_copy); + if (c == nullptr) { + jni->FatalError("GetStringCritical returned null!"); + } + } + + LOG("CriticalThread should now be in deferred suspension\n"); + // Do some visible work + while (stay_in_critical_native) { + native_counter++; + sleep_ms(1); + } + + LOG("CriticalThread released for Java upcall\n"); + // Now perform the Java upcall + jmethodID upcall_mid = jni->GetStaticMethodID(cls, "upcall", "()V"); + if (upcall_mid == nullptr) { + // Unexpected exception - let it propagate + if (bytes != nullptr) { + jni->ReleasePrimitiveArrayCritical(bytes, b, 0); + } + if (str != nullptr) { + jni->ReleaseStringCritical(str, c); + } + return; + } + jni->CallStaticVoidMethod(cls, upcall_mid); + + if (bytes != nullptr) { + jni->ReleasePrimitiveArrayCritical(bytes, b, 0); + } + if (str != nullptr) { + jni->ReleaseStringCritical(str, c); + } + + // Now we should suspend as we return to Java + LOG("CriticalThread returning to Java\n"); +} + +/** Agent library initialization. */ + +JNIEXPORT jint JNICALL +Agent_OnLoad(JavaVM *jvm, char *options, void *reserved) { + LOG("\nAgent_OnLoad started\n"); + + // create JVMTI environment + if (jvm->GetEnv((void **) (&jvmti), JVMTI_VERSION_1_1) != JNI_OK) { + LOG("Wrong result of a valid call to GetEnv!\n"); + return JNI_ERR; + } + + // add specific capabilities for suspending thread + jvmtiCapabilities suspendCaps; + memset(&suspendCaps, 0, sizeof(suspendCaps)); + suspendCaps.can_suspend = 1; + + jvmtiError err = jvmti->AddCapabilities(&suspendCaps); + if (err != JVMTI_ERROR_NONE) { + LOG("(AddCapabilities) unexpected error: %s (%d)\n", TranslateError(err), err); + return JNI_ERR; + } + LOG("Agent_OnLoad finished\n"); + return JNI_OK; +} + +} // extern C