[aarch64-port-dev ] 8153107: Unbalanced recursive locking

[Andrey Petushkov]

> On 13 Jun 2018, at 16:37, Andrew Haley <aph at redhat.com> wrote:
> 
> On 06/13/2018 01:40 PM, Andrey Petushkov wrote:
> 
>> It looks like the cause of the
>> https://bugs.openjdk.java.net/browse/JDK-8153107
>> <https://bugs.openjdk.java.net/browse/JDK-8153107> problem is that
>> ObjectSynchronizer::quick_enter implementation leaves the BasicLock
>> marked as recursive (DHW==0) in case that the lock is inflated and
>> already taken. As such the routines which first check the recursive
>> stack locking (e.g. fast_exit) consider this lock as non-inflated
>> recursive and hence do not decrement _recursions field of
>> ObjectMonitor, leaving the monitor locked forever. In contrary the
>> slow path does work the right way: ObjectSynchronizer::slow_enter
>> first marks stack lock as non-locked and non-recursive (DHW==3) and
>> then delegates to ObjectMonitor::enter to increment _recursions.
> 
>> So from the source code prospective this bug looks like
>> platform-independent, contrary to what’s was found under
>> https://bugs.openjdk.java.net/browse/JDK-8131715
>> <https://bugs.openjdk.java.net/browse/JDK-8131715>. Unfortunately I
>> cannot prove it with an example. The only case it fails for me is
>> runtime/CreateMirror/ArraysNewInstanceBug.java hotspot jtreg test on
>> aarch32 openjdk port. The following changes fix the problem:
> 
> Ouch.  The code is due to 8167501, which was a bug in the Oracle ARM64
> and ARM32 ports.  This bug was never seen in the AARCH64 open port,
> so the workaround shouldn't be enabled for it.
> 
> 8167501: ARMv7 Linux C2 compiler crashes running jtreg harness on MP systems
> Reviewed-by: dcubed
> diff -r afd6ae4fec81 -r 71d7ced6c439 hotspot/src/share/vm/runtime/sharedRuntime.cpp
> --- a/hotspot/src/share/vm/runtime/sharedRuntime.cpp	Thu Nov 03 11:53:20 2016 +0530
> +++ b/hotspot/src/share/vm/runtime/sharedRuntime.cpp	Thu Nov 03 10:44:17 2016 -0400
> @@ -1983,8 +1983,10 @@
> // Handles the uncommon case in locking, i.e., contention or an inflated lock.
> JRT_BLOCK_ENTRY(void, SharedRuntime::complete_monitor_locking_C(oopDesc* _obj, BasicLock* lock, JavaThread* thread))
>   // Disable ObjectSynchronizer::quick_enter() in default config
> -  // on AARCH64 until JDK-8153107 is resolved.
> -  if (AARCH64_ONLY((SyncFlags & 256) != 0 &&) !SafepointSynchronize::is_synchronizing()) {
> +  // on AARCH64 and ARM until JDK-8153107 is resolved.
> +  if (ARM_ONLY((SyncFlags & 256) != 0 &&)
> +      AARCH64_ONLY((SyncFlags & 256) != 0 &&)
> +      !SafepointSynchronize::is_synchronizing()) {
>     // Only try quick_enter() if we're not trying to reach a safepoint
> 
> Why did you move the code which handles recursive locking counts?  Do
> you believe that it's incorrect where it is, and if so, why?
That’s exactly what I’ve been trying to describe. The displaced header shall be assigned 3 when recursive locking happens on inflated lock. At least with the current design of the exit code which uses it as an indicator that inflated lock is present.
So it seems to me that currently the number of recursive locks on an object is the count of relevant stack lock structures with mark(DHW)!=3 (i.e. first lock + any stack locks prior to inflation) + _recursions field value of the inflated lock structure. The sequence of operations under review was breaking this assumption: any recursion via quick_enter was increasing both number of stack locks with DHW==0 and value of _recursions. While the slow_enter only increased _recursions
The above is idea I got from reading the code, I hope someone more experienced (@Daniel Daugherty?) can chime in and prove me right or wrong

Andrey
> 
> -- 
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671