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

Tobias Hartmann tobias.hartmann at oracle.com
Thu Jan 17 08:01:35 UTC 2019


Hi Ioi,

sounds good. Thanks for making these changes!

Best regards,
Tobias

On 16.01.19 23:53, Ioi Lam wrote:
> 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
>>>
>>>


More information about the valhalla-dev mailing list