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

[Tobias Hartmann]

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

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

c1_Instruction.hpp:1577: the indentation is wrong

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
> 
>