RFR: 8253540: InterpreterRuntime::monitorexit should be a JRT_LEAF function

[Daniel D.Daugherty]

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