Skip to content

Commit

Permalink
8316746: Top of lock-stack does not match the unlocked object
Browse files Browse the repository at this point in the history
Reviewed-by: rrich, lucy
  • Loading branch information
TheRealMDoerr committed Nov 9, 2023
1 parent dd9eab1 commit 7d8adfa
Show file tree
Hide file tree
Showing 7 changed files with 116 additions and 84 deletions.
1 change: 0 additions & 1 deletion src/hotspot/cpu/arm/frame_arm.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,6 @@ BasicObjectLock* frame::interpreter_frame_monitor_begin() const {
return (BasicObjectLock*) addr_at(interpreter_frame_monitor_block_bottom_offset);
}

// Pointer beyond the "oldest/deepest" BasicObjectLock on stack.
BasicObjectLock* frame::interpreter_frame_monitor_end() const {
BasicObjectLock* result = (BasicObjectLock*) *addr_at(interpreter_frame_monitor_block_top_offset);
// make sure the pointer points inside the frame
Expand Down
1 change: 0 additions & 1 deletion src/hotspot/cpu/ppc/frame_ppc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,6 @@ intptr_t *frame::initial_deoptimization_info() {
frame::frame(void* sp, void* fp, void* pc) : frame((intptr_t*)sp, (address)pc) {}
#endif

// Pointer beyond the "oldest/deepest" BasicObjectLock on stack.
BasicObjectLock* frame::interpreter_frame_monitor_end() const {
BasicObjectLock* result = (BasicObjectLock*) at_relative(ijava_idx(monitors));
// make sure the pointer points inside the frame
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1978,7 +1978,7 @@ void InterpreterMacroAssembler::profile_parameters_type(Register tmp1, Register
}
}

// Add a InterpMonitorElem to stack (see frame_sparc.hpp).
// Add a monitor (see frame_ppc.hpp).
void InterpreterMacroAssembler::add_monitor_to_stack(bool stack_is_empty, Register Rtemp1, Register Rtemp2) {

// Very-local scratch registers.
Expand Down
138 changes: 59 additions & 79 deletions src/hotspot/cpu/ppc/templateTable_ppc_64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4159,79 +4159,67 @@ void TemplateTable::athrow() {
// at next monitor exit.
void TemplateTable::monitorenter() {
transition(atos, vtos);

__ verify_oop(R17_tos);

Register Rcurrent_monitor = R11_scratch1,
Rcurrent_obj = R12_scratch2,
Register Rcurrent_monitor = R3_ARG1,
Rcurrent_obj = R4_ARG2,
Robj_to_lock = R17_tos,
Rscratch1 = R3_ARG1,
Rscratch2 = R4_ARG2,
Rscratch3 = R5_ARG3,
Rcurrent_obj_addr = R6_ARG4;
Rscratch1 = R11_scratch1,
Rscratch2 = R12_scratch2,
Rbot = R5_ARG3,
Rfree_slot = R6_ARG4;

Label Lfound, Lallocate_new;

__ ld(Rscratch1, _abi0(callers_sp), R1_SP); // load FP
__ li(Rfree_slot, 0); // Points to free slot or null.

// Set up search loop - start with topmost monitor.
__ mr(Rcurrent_monitor, R26_monitor);
__ addi(Rbot, Rscratch1, -frame::ijava_state_size);

// ------------------------------------------------------------------------------
// Null pointer exception.
__ null_check_throw(Robj_to_lock, -1, R11_scratch1);
__ null_check_throw(Robj_to_lock, -1, Rscratch1);

// Try to acquire a lock on the object.
// Repeat until succeeded (i.e., until monitorenter returns true).
// Check if any slot is present => short cut to allocation if not.
__ cmpld(CCR0, Rcurrent_monitor, Rbot);
__ beq(CCR0, Lallocate_new);

// ------------------------------------------------------------------------------
// Find a free slot in the monitor block.
Label Lfound, Lexit, Lallocate_new;
ConditionRegister found_free_slot = CCR0,
found_same_obj = CCR1,
reached_limit = CCR6;
// Note: The order of the monitors is important for C2 OSR which derives the
// unlock order from it (see comments for interpreter_frame_monitor_*).
{
Label Lloop;
Register Rlimit = Rcurrent_monitor;

// Set up search loop - start with topmost monitor.
__ addi(Rcurrent_obj_addr, R26_monitor, in_bytes(BasicObjectLock::obj_offset()));
Label Lloop, LnotFree, Lexit;

__ ld(Rlimit, 0, R1_SP);
__ addi(Rlimit, Rlimit, - (frame::ijava_state_size + frame::interpreter_frame_monitor_size_in_bytes() - in_bytes(BasicObjectLock::obj_offset()))); // Monitor base
__ bind(Lloop);
__ ld(Rcurrent_obj, in_bytes(BasicObjectLock::obj_offset()), Rcurrent_monitor);
// Exit if current entry is for same object; this guarantees, that new monitor
// used for recursive lock is above the older one.
__ cmpd(CCR0, Rcurrent_obj, Robj_to_lock);
__ beq(CCR0, Lexit); // recursive locking

// Check if any slot is present => short cut to allocation if not.
__ cmpld(reached_limit, Rcurrent_obj_addr, Rlimit);
__ bgt(reached_limit, Lallocate_new);
__ cmpdi(CCR0, Rcurrent_obj, 0);
__ bne(CCR0, LnotFree);
__ mr(Rfree_slot, Rcurrent_monitor); // remember free slot closest to the bottom
__ bind(LnotFree);

// Pre-load topmost slot.
__ ld(Rcurrent_obj, 0, Rcurrent_obj_addr);
__ addi(Rcurrent_obj_addr, Rcurrent_obj_addr, frame::interpreter_frame_monitor_size_in_bytes());
// The search loop.
__ bind(Lloop);
// Found free slot?
__ cmpdi(found_free_slot, Rcurrent_obj, 0);
// Is this entry for same obj? If so, stop the search and take the found
// free slot or allocate a new one to enable recursive locking.
__ cmpd(found_same_obj, Rcurrent_obj, Robj_to_lock);
__ cmpld(reached_limit, Rcurrent_obj_addr, Rlimit);
__ beq(found_free_slot, Lexit);
__ beq(found_same_obj, Lallocate_new);
__ bgt(reached_limit, Lallocate_new);
// Check if last allocated BasicLockObj reached.
__ ld(Rcurrent_obj, 0, Rcurrent_obj_addr);
__ addi(Rcurrent_obj_addr, Rcurrent_obj_addr, frame::interpreter_frame_monitor_size_in_bytes());
// Next iteration if unchecked BasicObjectLocks exist on the stack.
__ b(Lloop);
__ addi(Rcurrent_monitor, Rcurrent_monitor, frame::interpreter_frame_monitor_size_in_bytes());
__ cmpld(CCR0, Rcurrent_monitor, Rbot);
__ bne(CCR0, Lloop);
__ bind(Lexit);
}

// ------------------------------------------------------------------------------
// Check if we found a free slot.
__ bind(Lexit);

__ addi(Rcurrent_monitor, Rcurrent_obj_addr, -(frame::interpreter_frame_monitor_size_in_bytes()) - in_bytes(BasicObjectLock::obj_offset()));
__ addi(Rcurrent_obj_addr, Rcurrent_obj_addr, - frame::interpreter_frame_monitor_size_in_bytes());
__ b(Lfound);
__ cmpdi(CCR0, Rfree_slot, 0);
__ bne(CCR0, Lfound);

// We didn't find a free BasicObjLock => allocate one.
__ align(32, 12);
__ bind(Lallocate_new);
__ add_monitor_to_stack(false, Rscratch1, Rscratch2);
__ mr(Rcurrent_monitor, R26_monitor);
__ addi(Rcurrent_obj_addr, R26_monitor, in_bytes(BasicObjectLock::obj_offset()));
__ mr(Rfree_slot, R26_monitor);

// ------------------------------------------------------------------------------
// We now have a slot to lock.
Expand All @@ -4241,8 +4229,8 @@ void TemplateTable::monitorenter() {
// The object has already been popped from the stack, so the expression stack looks correct.
__ addi(R14_bcp, R14_bcp, 1);

__ std(Robj_to_lock, 0, Rcurrent_obj_addr);
__ lock_object(Rcurrent_monitor, Robj_to_lock);
__ std(Robj_to_lock, in_bytes(BasicObjectLock::obj_offset()), Rfree_slot);
__ lock_object(Rfree_slot, Robj_to_lock);

// Check if there's enough space on the stack for the monitors after locking.
// This emits a single store.
Expand All @@ -4256,46 +4244,40 @@ void TemplateTable::monitorexit() {
transition(atos, vtos);
__ verify_oop(R17_tos);

Register Rcurrent_monitor = R11_scratch1,
Rcurrent_obj = R12_scratch2,
Register Rcurrent_monitor = R3_ARG1,
Rcurrent_obj = R4_ARG2,
Robj_to_lock = R17_tos,
Rcurrent_obj_addr = R3_ARG1,
Rlimit = R4_ARG2;
Rscratch = R11_scratch1,
Rbot = R12_scratch2;

Label Lfound, Lillegal_monitor_state;

// Check corner case: unbalanced monitorEnter / Exit.
__ ld(Rlimit, 0, R1_SP);
__ addi(Rlimit, Rlimit, - (frame::ijava_state_size + frame::interpreter_frame_monitor_size_in_bytes())); // Monitor base
__ ld(Rscratch, _abi0(callers_sp), R1_SP); // load FP

// Set up search loop - start with topmost monitor.
__ mr(Rcurrent_monitor, R26_monitor);
__ addi(Rbot, Rscratch, -frame::ijava_state_size);

// Null pointer check.
__ null_check_throw(Robj_to_lock, -1, R11_scratch1);
__ null_check_throw(Robj_to_lock, -1, Rscratch);

__ cmpld(CCR0, R26_monitor, Rlimit);
__ bgt(CCR0, Lillegal_monitor_state);
// Check corner case: unbalanced monitorEnter / Exit.
__ cmpld(CCR0, Rcurrent_monitor, Rbot);
__ beq(CCR0, Lillegal_monitor_state);

// Find the corresponding slot in the monitors stack section.
{
Label Lloop;

// Start with topmost monitor.
__ addi(Rcurrent_obj_addr, R26_monitor, in_bytes(BasicObjectLock::obj_offset()));
__ addi(Rlimit, Rlimit, in_bytes(BasicObjectLock::obj_offset()));
__ ld(Rcurrent_obj, 0, Rcurrent_obj_addr);
__ addi(Rcurrent_obj_addr, Rcurrent_obj_addr, frame::interpreter_frame_monitor_size_in_bytes());

__ bind(Lloop);
__ ld(Rcurrent_obj, in_bytes(BasicObjectLock::obj_offset()), Rcurrent_monitor);
// Is this entry for same obj?
__ cmpd(CCR0, Rcurrent_obj, Robj_to_lock);
__ beq(CCR0, Lfound);

// Check if last allocated BasicLockObj reached.

__ ld(Rcurrent_obj, 0, Rcurrent_obj_addr);
__ cmpld(CCR0, Rcurrent_obj_addr, Rlimit);
__ addi(Rcurrent_obj_addr, Rcurrent_obj_addr, frame::interpreter_frame_monitor_size_in_bytes());

// Next iteration if unchecked BasicObjectLocks exist on the stack.
__ ble(CCR0, Lloop);
__ addi(Rcurrent_monitor, Rcurrent_monitor, frame::interpreter_frame_monitor_size_in_bytes());
__ cmpld(CCR0, Rcurrent_monitor, Rbot);
__ bne(CCR0, Lloop);
}

// Fell through without finding the basic obj lock => throw up!
Expand All @@ -4305,8 +4287,6 @@ void TemplateTable::monitorexit() {

__ align(32, 12);
__ bind(Lfound);
__ addi(Rcurrent_monitor, Rcurrent_obj_addr,
-(frame::interpreter_frame_monitor_size_in_bytes()) - in_bytes(BasicObjectLock::obj_offset()));
__ unlock_object(Rcurrent_monitor);
}

Expand Down
1 change: 0 additions & 1 deletion src/hotspot/cpu/s390/frame_s390.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,6 @@ intptr_t *frame::initial_deoptimization_info() {
return fp();
}

// Pointer beyond the "oldest/deepest" BasicObjectLock on stack.
BasicObjectLock* frame::interpreter_frame_monitor_end() const {
return interpreter_frame_monitors();
}
Expand Down
1 change: 0 additions & 1 deletion src/hotspot/cpu/zero/frame_zero.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ BasicObjectLock* frame::interpreter_frame_monitor_begin() const {
return get_interpreterState()->monitor_base();
}

// Pointer beyond the "oldest/deepest" BasicObjectLock on stack.
BasicObjectLock* frame::interpreter_frame_monitor_end() const {
return (BasicObjectLock*) get_interpreterState()->stack_base();
}
Expand Down
56 changes: 56 additions & 0 deletions test/hotspot/jtreg/compiler/locks/TestUnlockOSR.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* Copyright (c) 2023 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
* 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 8316746
* @summary During OSR, locks get transferred from interpreter frame.
* Check that unlocking 2 such locks works in the OSR compiled nmethod.
* Some platforms verify that the unlocking happens in the corrent order.
*
* @run main/othervm -Xbatch TestUnlockOSR
*/

public class TestUnlockOSR {
static void test_method(Object a, Object b, int limit) {
synchronized(a) { // allocate space for monitors
synchronized(b) {
}
} // free space to test allocation in reused space
synchronized(a) { // reuse the space
synchronized(b) {
for (int i = 0; i < limit; i++) {}
}
}
}

public static void main(String[] args) {
Object a = new TestUnlockOSR(),
b = new TestUnlockOSR();
// avoid uncommon trap before last unlocks
for (int i = 0; i < 100; i++) { test_method(a, b, 0); }
// trigger OSR
test_method(a, b, 100000);
}
}

3 comments on commit 7d8adfa

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

@TheRealMDoerr
Copy link
Contributor Author

Choose a reason for hiding this comment

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

/backport jdk21u

@openjdk
Copy link

@openjdk openjdk bot commented on 7d8adfa Nov 14, 2023

Choose a reason for hiding this comment

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

@TheRealMDoerr the backport was successfully created on the branch TheRealMDoerr-backport-7d8adfa8 in my personal fork of openjdk/jdk21u. To create a pull request with this backport targeting openjdk/jdk21u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 7d8adfa8 from the openjdk/jdk repository.

The commit being backported was authored by Martin Doerr on 9 Nov 2023 and was reviewed by Richard Reingruber and Lutz Schmidt.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21u:

$ git fetch https://github.com/openjdk-bots/jdk21u.git TheRealMDoerr-backport-7d8adfa8:TheRealMDoerr-backport-7d8adfa8
$ git checkout TheRealMDoerr-backport-7d8adfa8
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21u.git TheRealMDoerr-backport-7d8adfa8

Please sign in to comment.