Skip to content

Commit

Permalink
8327743: JVM crash in hotspot/share/runtime/javaThread.cpp - failed: …
Browse files Browse the repository at this point in the history
…held monitor count should be equal to jni: 0 != 1

Co-authored-by: Fredrik Bredberg <fbredberg@openjdk.org>
Reviewed-by: rehn, fbredberg, pchilanomate, rrich
  • Loading branch information
David Holmes and Fredrik Bredberg committed Apr 16, 2024
1 parent 140f567 commit 274c805
Show file tree
Hide file tree
Showing 10 changed files with 491 additions and 44 deletions.
42 changes: 41 additions & 1 deletion src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1038,9 +1038,49 @@ static void continuation_enter_cleanup(MacroAssembler* masm) {
__ stop("incorrect sp1");
__ bind(OK);
#endif

__ ldr(rscratch1, Address(sp, ContinuationEntry::parent_cont_fastpath_offset()));
__ str(rscratch1, Address(rthread, JavaThread::cont_fastpath_offset()));

if (CheckJNICalls) {
// Check if this is a virtual thread continuation
Label L_skip_vthread_code;
__ ldrw(rscratch1, Address(sp, ContinuationEntry::flags_offset()));
__ cbzw(rscratch1, L_skip_vthread_code);

// If the held monitor count is > 0 and this vthread is terminating then
// it failed to release a JNI monitor. So we issue the same log message
// that JavaThread::exit does.
__ ldr(rscratch1, Address(rthread, JavaThread::jni_monitor_count_offset()));
__ cbz(rscratch1, L_skip_vthread_code);

// Save return value potentially containing the exception oop in callee-saved R19.
__ mov(r19, r0);
__ call_VM_leaf(CAST_FROM_FN_PTR(address, SharedRuntime::log_jni_monitor_still_held));
// Restore potential return value.
__ mov(r0, r19);

// For vthreads we have to explicitly zero the JNI monitor count of the carrier
// on termination. The held count is implicitly zeroed below when we restore from
// the parent held count (which has to be zero).
__ str(zr, Address(rthread, JavaThread::jni_monitor_count_offset()));

__ bind(L_skip_vthread_code);
}
#ifdef ASSERT
else {
// Check if this is a virtual thread continuation
Label L_skip_vthread_code;
__ ldrw(rscratch1, Address(sp, ContinuationEntry::flags_offset()));
__ cbzw(rscratch1, L_skip_vthread_code);

// See comment just above. If not checking JNI calls the JNI count is only
// needed for assertion checking.
__ str(zr, Address(rthread, JavaThread::jni_monitor_count_offset()));

__ bind(L_skip_vthread_code);
}
#endif

__ ldr(rscratch1, Address(sp, ContinuationEntry::parent_held_monitor_count_offset()));
__ str(rscratch1, Address(rthread, JavaThread::held_monitor_count_offset()));

Expand Down
55 changes: 51 additions & 4 deletions src/hotspot/cpu/ppc/sharedRuntime_ppc.cpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
* Copyright (c) 1997, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2012, 2023 SAP SE. All rights reserved.
* Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2012, 2024 SAP SE. 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
Expand Down Expand Up @@ -1637,7 +1637,7 @@ static void fill_continuation_entry(MacroAssembler* masm, Register reg_cont_obj,
// None.
//
// Kills:
// R8_ARG6, R9_ARG7, R10_ARG8
// R8_ARG6, R9_ARG7, R10_ARG8, R15_esp
//
static void continuation_enter_cleanup(MacroAssembler* masm) {
Register tmp1 = R8_ARG6;
Expand All @@ -1652,9 +1652,56 @@ static void continuation_enter_cleanup(MacroAssembler* masm) {
#endif

__ ld_ptr(tmp1, ContinuationEntry::parent_cont_fastpath_offset(), R1_SP);
__ st_ptr(tmp1, JavaThread::cont_fastpath_offset(), R16_thread);

if (CheckJNICalls) {
// Check if this is a virtual thread continuation
Label L_skip_vthread_code;
__ lwz(R0, in_bytes(ContinuationEntry::flags_offset()), R1_SP);
__ cmpwi(CCR0, R0, 0);
__ beq(CCR0, L_skip_vthread_code);

// If the held monitor count is > 0 and this vthread is terminating then
// it failed to release a JNI monitor. So we issue the same log message
// that JavaThread::exit does.
__ ld(R0, in_bytes(JavaThread::jni_monitor_count_offset()), R16_thread);
__ cmpdi(CCR0, R0, 0);
__ beq(CCR0, L_skip_vthread_code);

// Save return value potentially containing the exception oop
Register ex_oop = R15_esp; // nonvolatile register
__ mr(ex_oop, R3_RET);
__ call_VM_leaf(CAST_FROM_FN_PTR(address, SharedRuntime::log_jni_monitor_still_held));
// Restore potental return value
__ mr(R3_RET, ex_oop);

// For vthreads we have to explicitly zero the JNI monitor count of the carrier
// on termination. The held count is implicitly zeroed below when we restore from
// the parent held count (which has to be zero).
__ li(tmp1, 0);
__ std(tmp1, in_bytes(JavaThread::jni_monitor_count_offset()), R16_thread);

__ bind(L_skip_vthread_code);
}
#ifdef ASSERT
else {
// Check if this is a virtual thread continuation
Label L_skip_vthread_code;
__ lwz(R0, in_bytes(ContinuationEntry::flags_offset()), R1_SP);
__ cmpwi(CCR0, R0, 0);
__ beq(CCR0, L_skip_vthread_code);

// See comment just above. If not checking JNI calls the JNI count is only
// needed for assertion checking.
__ li(tmp1, 0);
__ std(tmp1, in_bytes(JavaThread::jni_monitor_count_offset()), R16_thread);

__ bind(L_skip_vthread_code);
}
#endif

__ ld(tmp2, in_bytes(ContinuationEntry::parent_held_monitor_count_offset()), R1_SP);
__ ld_ptr(tmp3, ContinuationEntry::parent_offset(), R1_SP);
__ st_ptr(tmp1, JavaThread::cont_fastpath_offset(), R16_thread);
__ std(tmp2, in_bytes(JavaThread::held_monitor_count_offset()), R16_thread);
__ st_ptr(tmp3, JavaThread::cont_entry_offset(), R16_thread);
DEBUG_ONLY(__ block_comment("} clean"));
Expand Down
43 changes: 42 additions & 1 deletion src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2003, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2003, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, 2020, Red Hat Inc. All rights reserved.
* Copyright (c) 2020, 2023, Huawei Technologies Co., Ltd. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
Expand Down Expand Up @@ -905,6 +905,47 @@ static void continuation_enter_cleanup(MacroAssembler* masm) {

__ ld(t0, Address(sp, ContinuationEntry::parent_cont_fastpath_offset()));
__ sd(t0, Address(xthread, JavaThread::cont_fastpath_offset()));

if (CheckJNICalls) {
// Check if this is a virtual thread continuation
Label L_skip_vthread_code;
__ lwu(t0, Address(sp, ContinuationEntry::flags_offset()));
__ beqz(t0, L_skip_vthread_code);

// If the held monitor count is > 0 and this vthread is terminating then
// it failed to release a JNI monitor. So we issue the same log message
// that JavaThread::exit does.
__ ld(t0, Address(xthread, JavaThread::jni_monitor_count_offset()));
__ beqz(t0, L_skip_vthread_code);

// Save return value potentially containing the exception oop in callee-saved x9
__ mv(x9, x10);
__ call_VM_leaf(CAST_FROM_FN_PTR(address, SharedRuntime::log_jni_monitor_still_held));
// Restore potential return value
__ mv(x10, x9);

// For vthreads we have to explicitly zero the JNI monitor count of the carrier
// on termination. The held count is implicitly zeroed below when we restore from
// the parent held count (which has to be zero).
__ sd(zr, Address(xthread, JavaThread::jni_monitor_count_offset()));

__ bind(L_skip_vthread_code);
}
#ifdef ASSERT
else {
// Check if this is a virtual thread continuation
Label L_skip_vthread_code;
__ lwu(t0, Address(sp, ContinuationEntry::flags_offset()));
__ beqz(t0, L_skip_vthread_code);

// See comment just above. If not checking JNI calls the JNI count is only
// needed for assertion checking.
__ sd(zr, Address(xthread, JavaThread::jni_monitor_count_offset()));

__ bind(L_skip_vthread_code);
}
#endif

__ ld(t0, Address(sp, ContinuationEntry::parent_held_monitor_count_offset()));
__ sd(t0, Address(xthread, JavaThread::held_monitor_count_offset()));

Expand Down
42 changes: 40 additions & 2 deletions src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1354,9 +1354,48 @@ void static continuation_enter_cleanup(MacroAssembler* masm) {
__ stop("Incorrect rsp at continuation_enter_cleanup");
__ bind(L_good_sp);
#endif

__ movptr(rbx, Address(rsp, ContinuationEntry::parent_cont_fastpath_offset()));
__ movptr(Address(r15_thread, JavaThread::cont_fastpath_offset()), rbx);

if (CheckJNICalls) {
// Check if this is a virtual thread continuation
Label L_skip_vthread_code;
__ cmpl(Address(rsp, ContinuationEntry::flags_offset()), 0);
__ jcc(Assembler::equal, L_skip_vthread_code);

// If the held monitor count is > 0 and this vthread is terminating then
// it failed to release a JNI monitor. So we issue the same log message
// that JavaThread::exit does.
__ cmpptr(Address(r15_thread, JavaThread::jni_monitor_count_offset()), 0);
__ jcc(Assembler::equal, L_skip_vthread_code);

// rax may hold an exception oop, save it before the call
__ push(rax);
__ call_VM_leaf(CAST_FROM_FN_PTR(address, SharedRuntime::log_jni_monitor_still_held));
__ pop(rax);

// For vthreads we have to explicitly zero the JNI monitor count of the carrier
// on termination. The held count is implicitly zeroed below when we restore from
// the parent held count (which has to be zero).
__ movq(Address(r15_thread, JavaThread::jni_monitor_count_offset()), 0);

__ bind(L_skip_vthread_code);
}
#ifdef ASSERT
else {
// Check if this is a virtual thread continuation
Label L_skip_vthread_code;
__ cmpl(Address(rsp, ContinuationEntry::flags_offset()), 0);
__ jcc(Assembler::equal, L_skip_vthread_code);

// See comment just above. If not checking JNI calls the JNI count is only
// needed for assertion checking.
__ movq(Address(r15_thread, JavaThread::jni_monitor_count_offset()), 0);

__ bind(L_skip_vthread_code);
}
#endif

__ movq(rbx, Address(rsp, ContinuationEntry::parent_held_monitor_count_offset()));
__ movq(Address(r15_thread, JavaThread::held_monitor_count_offset()), rbx);

Expand Down Expand Up @@ -3696,4 +3735,3 @@ void OptoRuntime::generate_exception_blob() {
_exception_blob = ExceptionBlob::create(&buffer, oop_maps, SimpleRuntimeFrame::framesize >> 1);
}
#endif // COMPILER2

47 changes: 35 additions & 12 deletions src/hotspot/share/runtime/javaThread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -910,19 +910,27 @@ void JavaThread::exit(bool destroy_vm, ExitType exit_type) {
"should not have a Java frame when detaching or exiting");
ObjectSynchronizer::release_monitors_owned_by_thread(this);
assert(!this->has_pending_exception(), "release_monitors should have cleared");
// Check for monitor counts being out of sync.
assert(held_monitor_count() == jni_monitor_count(),
"held monitor count should be equal to jni: " INTX_FORMAT " != " INTX_FORMAT,
held_monitor_count(), jni_monitor_count());
// All in-use monitors, including JNI-locked ones, should have been released above.
assert(held_monitor_count() == 0, "Failed to unlock " INTX_FORMAT " object monitors",
held_monitor_count());
} else {
// Check for monitor counts being out of sync.
assert(held_monitor_count() == jni_monitor_count(),
"held monitor count should be equal to jni: " INTX_FORMAT " != " INTX_FORMAT,
held_monitor_count(), jni_monitor_count());
// It is possible that a terminating thread failed to unlock monitors it locked
// via JNI so we don't assert the count is zero.
}

// Since above code may not release JNI monitors and if someone forgot to do an
// JNI monitorexit, held count should be equal jni count.
// Consider scan all object monitor for this owner if JNI count > 0 (at least on detach).
assert(held_monitor_count() == jni_monitor_count(),
"held monitor count should be equal to jni: " INTX_FORMAT " != " INTX_FORMAT,
held_monitor_count(), jni_monitor_count());
if (CheckJNICalls && jni_monitor_count() > 0) {
// We would like a fatal here, but due to we never checked this before there
// is a lot of tests which breaks, even with an error log.
log_debug(jni)("JavaThread %s (tid: " UINTX_FORMAT ") with Objects still locked by JNI MonitorEnter.",
exit_type == JavaThread::normal_exit ? "exiting" : "detaching", os::current_thread_id());
exit_type == JavaThread::normal_exit ? "exiting" : "detaching", os::current_thread_id());
}

// These things needs to be done while we are still a Java Thread. Make sure that thread
Expand Down Expand Up @@ -960,9 +968,12 @@ void JavaThread::exit(bool destroy_vm, ExitType exit_type) {
thread_name = os::strdup(name());
}

log_info(os, thread)("JavaThread %s (tid: " UINTX_FORMAT ").",
exit_type == JavaThread::normal_exit ? "exiting" : "detaching",
os::current_thread_id());
if (log_is_enabled(Info, os, thread)) {
ResourceMark rm(this);
log_info(os, thread)("JavaThread %s (name: \"%s\", tid: " UINTX_FORMAT ").",
exit_type == JavaThread::normal_exit ? "exiting" : "detaching",
name(), os::current_thread_id());
}

if (log_is_enabled(Debug, os, thread, timer)) {
_timer_exit_phase3.stop();
Expand Down Expand Up @@ -1990,17 +2001,23 @@ void JavaThread::trace_stack() {

#endif // PRODUCT

// Slow-path increment of the held monitor counts. JNI locking is always
// this slow-path.
void JavaThread::inc_held_monitor_count(intx i, bool jni) {
#ifdef SUPPORT_MONITOR_COUNT
assert(_held_monitor_count >= 0, "Must always be greater than 0: " INTX_FORMAT, _held_monitor_count);
assert(_held_monitor_count >= 0, "Must always be non-negative: " INTX_FORMAT, _held_monitor_count);
_held_monitor_count += i;
if (jni) {
assert(_jni_monitor_count >= 0, "Must always be greater than 0: " INTX_FORMAT, _jni_monitor_count);
assert(_jni_monitor_count >= 0, "Must always be non-negative: " INTX_FORMAT, _jni_monitor_count);
_jni_monitor_count += i;
}
assert(_held_monitor_count >= _jni_monitor_count, "Monitor count discrepancy detected - held count "
INTX_FORMAT " is less than JNI count " INTX_FORMAT, _held_monitor_count, _jni_monitor_count);
#endif
}

// Slow-path decrement of the held monitor counts. JNI unlocking is always
// this slow-path.
void JavaThread::dec_held_monitor_count(intx i, bool jni) {
#ifdef SUPPORT_MONITOR_COUNT
_held_monitor_count -= i;
Expand All @@ -2009,6 +2026,12 @@ void JavaThread::dec_held_monitor_count(intx i, bool jni) {
_jni_monitor_count -= i;
assert(_jni_monitor_count >= 0, "Must always be greater than 0: " INTX_FORMAT, _jni_monitor_count);
}
// When a thread is detaching with still owned JNI monitors, the logic that releases
// the monitors doesn't know to set the "jni" flag and so the counts can get out of sync.
// So we skip this assert if the thread is exiting. Once all monitors are unlocked the
// JNI count is directly set to zero.
assert(_held_monitor_count >= _jni_monitor_count || is_exiting(), "Monitor count discrepancy detected - held count "
INTX_FORMAT " is less than JNI count " INTX_FORMAT, _held_monitor_count, _jni_monitor_count);
#endif
}

Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/runtime/javaThread.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,7 @@ class JavaThread: public Thread {
static ByteSize cont_entry_offset() { return byte_offset_of(JavaThread, _cont_entry); }
static ByteSize cont_fastpath_offset() { return byte_offset_of(JavaThread, _cont_fastpath); }
static ByteSize held_monitor_count_offset() { return byte_offset_of(JavaThread, _held_monitor_count); }
static ByteSize jni_monitor_count_offset() { return byte_offset_of(JavaThread, _jni_monitor_count); }

#if INCLUDE_JVMTI
static ByteSize is_in_VTMS_transition_offset() { return byte_offset_of(JavaThread, _is_in_VTMS_transition); }
Expand Down
14 changes: 14 additions & 0 deletions src/hotspot/share/runtime/sharedRuntime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1931,6 +1931,20 @@ JRT_LEAF(void, SharedRuntime::complete_monitor_unlocking_C(oopDesc* obj, BasicLo
SharedRuntime::monitor_exit_helper(obj, lock, current);
JRT_END

// This is only called when CheckJNICalls is true, and only
// for virtual thread termination.
JRT_LEAF(void, SharedRuntime::log_jni_monitor_still_held())
assert(CheckJNICalls, "Only call this when checking JNI usage");
if (log_is_enabled(Debug, jni)) {
JavaThread* current = JavaThread::current();
int64_t vthread_id = java_lang_Thread::thread_id(current->vthread());
int64_t carrier_id = java_lang_Thread::thread_id(current->threadObj());
log_debug(jni)("VirtualThread (tid: " INT64_FORMAT ", carrier id: " INT64_FORMAT
") exiting with Objects still locked by JNI MonitorEnter.",
vthread_id, carrier_id);
}
JRT_END

#ifndef PRODUCT

void SharedRuntime::print_statistics() {
Expand Down
3 changes: 3 additions & 0 deletions src/hotspot/share/runtime/sharedRuntime.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,9 @@ class SharedRuntime: AllStatic {

static void monitor_exit_helper(oopDesc* obj, BasicLock* lock, JavaThread* current);

// Issue UL warning for unlocked JNI monitor on virtual thread termination
static void log_jni_monitor_still_held();

private:
static Handle find_callee_info(Bytecodes::Code& bc, CallInfo& callinfo, TRAPS);
static Handle find_callee_info_helper(vframeStream& vfst, Bytecodes::Code& bc, CallInfo& callinfo, TRAPS);
Expand Down
Loading

1 comment on commit 274c805

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.