RFR: 8253540: InterpreterRuntime::monitorexit should be a JRT_LEAF function
Daniel D.Daugherty
dcubed at openjdk.java.net
Wed Sep 23 19:22:50 UTC 2020
On Wed, 23 Sep 2020 19:16:45 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:
>> The review invite says this:
>>
>>> That the monitor has already been unlocked, or is a null stacklock
>>> monitor has been already checked in the caller
>>
>> so I also need to go look at the caller contexts.
>>
>> Update: So I went and took a look and very quickly found this:
>>
>> src/hotspot/cpu/x86/interp_masm_x86.cpp:
>>
>> void InterpreterMacroAssembler::unlock_object(Register lock_reg) {
>> assert(lock_reg == LP64_ONLY(c_rarg1) NOT_LP64(rdx),
>> "The argument is only for looks. It must be c_rarg1");
>>
>> if (UseHeavyMonitors) {
>> call_VM(noreg,
>> CAST_FROM_FN_PTR(address, InterpreterRuntime::monitorexit),
>> lock_reg);
>> } else {
>>
>> So when -XX:+UseHeavyMonitors is used, we make a direct call
>> to InterpreterRuntime::monitorexit() without checking the lock_reg
>> parameter at all.
>
> Read the caller of this function.
Here's the else branch (!UseHeavyMonitors) in InterpreterMacroAssembler::unlock_object:
src/hotspot/cpu/x86/interp_masm_x86.cpp:
1287 } else {
1288 Label done;
1289
1290 const Register swap_reg = rax; // Must use rax for cmpxchg instruction
1291 const Register header_reg = LP64_ONLY(c_rarg2) NOT_LP64(rbx); // Will contain the old oopMark
1292 const Register obj_reg = LP64_ONLY(c_rarg3) NOT_LP64(rcx); // Will contain the oop
1293
1294 save_bcp(); // Save in case of exception
1295
1296 // Convert from BasicObjectLock structure to object and BasicLock
1297 // structure Store the BasicLock address into %rax
1298 lea(swap_reg, Address(lock_reg, BasicObjectLock::lock_offset_in_bytes()));
1299
1300 // Load oop into obj_reg(%c_rarg3)
1301 movptr(obj_reg, Address(lock_reg, BasicObjectLock::obj_offset_in_bytes()));
1302
1303 // Free entry
1304 movptr(Address(lock_reg, BasicObjectLock::obj_offset_in_bytes()), (int32_t)NULL_WORD);
1305
1306 if (UseBiasedLocking) {
1307 biased_locking_exit(obj_reg, header_reg, done);
1308 }
1309
1310 // Load the old header from BasicLock structure
1311 movptr(header_reg, Address(swap_reg,
1312 BasicLock::displaced_header_offset_in_bytes()));
1313
1314 // Test for recursion
1315 testptr(header_reg, header_reg);
1316
1317 // zero for recursive case
1318 jcc(Assembler::zero, done);
1319
1320 // Atomic swap back the old header
1321 lock();
1322 cmpxchgptr(header_reg, Address(obj_reg, oopDesc::mark_offset_in_bytes()));
1323
1324 // zero for simple unlock of a stack-lock case
1325 jcc(Assembler::zero, done);
1326
1327 // Call the runtime routine for slow case.
1328 movptr(Address(lock_reg, BasicObjectLock::obj_offset_in_bytes()),
1329 obj_reg); // restore obj
1330 call_VM(noreg,
1331 CAST_FROM_FN_PTR(address, InterpreterRuntime::monitorexit),
1332 lock_reg);
1333
1334 bind(done);
1335
1336 restore_bcp();
1337 }
If the BasicLock passed in via lock_reg is NULL, then this code will crash
on L1298 (I think) before we get to InterpreterRuntime::monitorexit().
However, I haven't checked the other code paths yet.
What I did notice is that this code path does not check for the
monitor being owned by the calling thread so the deletion of
this block from InterpreterRuntime::monitorexit():
if (elem == NULL || h_obj()->is_unlocked()) {
THROW(vmSymbols::java_lang_IllegalMonitorStateException());
}
is a problem. While it might be true that the "elem == NULL" part
cannot happen, the "h_obj()->is_unlocked()" can definitely happen
and this change results in the IllegalMonitorStateException not
being thrown.
I think this change isn't going to work.
> Read the caller of this function.
I have and I have even started quoting caller code.
-------------
PR: https://git.openjdk.java.net/jdk/pull/320
More information about the hotspot-runtime-dev
mailing list