RFR 8217223 [lworld][c1] monitorenter on value object should throw IllegalMonitorStateException

[Ioi Lam]

Hi Tobias, thanks for the review.

On 1/16/19 9:26 AM, Tobias Hartmann wrote:
> Hi Ioi,
>
> overall, the fix looks good to me!
>
> Here are some comments:
> c1_MacroAssembler_x86.cpp: With UseBiasedLocking, no changes should be required to call the slow
> path because value types have the always_locked_pattern set. Without UseBiasedLocking, an andptr
> instruction should be enough. See relevant code in InterpreterMacroAssembler::lock_object() or this:
> http://cr.openjdk.java.net/~thartmann/8204615/webrev.00/src/hotspot/cpu/x86/macroAssembler_x86.cpp.sdiff.html

I changed the code as you suggested

   // Load object header
   movptr(hdr, Address(obj, hdr_offset));
   // and mark it as unlocked
   orptr(hdr, markOopDesc::unlocked_value);
+ if (EnableValhalla && !UseBiasedLocking) {
+   // Mask always_locked bit such that we go to the slow path if object 
is a value type
+   andptr(hdr, ~markOopDesc::biased_lock_bit_in_place);
+ }


> TestLWorld.java:50-67: I think we still want to run all the scenarios because they define some more
> flag combinations, right? See:
> http://hg.openjdk.java.net/valhalla/valhalla/file/2fd66be05722/test/hotspot/jtreg/compiler/valhalla/valuetypes/ValueTypeTest.java#l214

I will clean up the C1 testing in a different patch, so all these other 
scenarios are tested. This will probably discover more bugs (such as 
JDK-8217284).


>
> c1_Instruction.hpp:1577: the indentation is wrong

Fixed.

Thanks

- Ioi


>
> Thanks,
> Tobias
>
> On 16.01.19 07:32, Ioi Lam wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8217223
>> http://cr.openjdk.java.net/~iklam/valhalla/8217223-c1-monitorenter-throw-imse.v01/
>>
>> Here's a fix for C1 to properly throw IllegalMonitorStateException
>> when synchronizing on value objects.
>>
>> Ideally, we should just call into Runtime1::monitorenter(), which
>> handles the IMSE throwing. However, due to C1's limitation, when
>> an exception happens inside Runtime1::monitorenter(), there's no
>> way for the current method to catch this exception, so for code
>> like this:
>>
>>      try {
>>          synchronized(vt) {}
>>      } catch (Throwable t) {
>>          foo();
>>      }
>>
>> foo() is never called and the exception is thrown to the method's
>> caller.
>>
>> As a work around, I added an explicit check in MonitorEnterStub to
>> call a SimpleExceptionStub to throw the IMSE.
>>
>> Here's what the generated code looks like. The instructions marked
>> by "+" are added by this patch.
>>
>>
>> +UseBiasedLocking case:
>> ======================
>>
>>   ;; MonitorEnterStub slow case
>>    0x00007fe788e285f0: mov    (%rsi),%rdi
>> + 0x00007fe788e285f3: test   $0x405,%edi
>>         ;; 0x405 = markOopDesc::always_locked_pattern
>> + 0x00007fe788e285f9: jne    0x00007fe788e28612
>>    0x00007fe788e285ff: mov    %rsi,0x8(%rsp)
>>    0x00007fe788e28604: mov    %rdx,(%rsp)
>>    0x00007fe788e28608: callq  0x00007fe7889d8840
>>         ;;{runtime_call monitorenter_nofpu}
>>
>>    0x00007fe788e2860d: jmpq   0x00007fe788e28381
>>   ;; SimpleExceptionStub slow case
>> + 0x00007fe788e28612: callq  0x00007fe7889d7e40
>>         ;;{runtime_call throw_illegal_monitor_state_exception}
>>
>>
>> -UseBiasedLocking case:
>> ======================
>>
>>    0x00007fe1e8e2828c: lea    0x20(%rsp),%rdx
>>    0x00007fe1e8e28291: mov    %rsi,0x8(%rdx)
>>    0x00007fe1e8e28295: mov    (%rsi),%rax
>> + 0x00007fe1e8e28298: test   $0x405,%eax
>> + 0x00007fe1e8e2829d: jne    0x00007fe1e8e28528 ;; MonitorEnterStub
>>    0x00007fe1e8e282a3: or     $0x1,%rax
>>    0x00007fe1e8e282a7: mov    %rax,(%rdx)
>>    0x00007fe1e8e282aa: lock cmpxchg %rdx,(%rsi)
>>
>> (the MonitorEnterStub looks identical to the +UseBiasedLocking case).
>>
>> Thanks
>> - Ioi
>>
>>