Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8267042

bug in monitor locking/unlocking on ARM32 C1 due to uninitialized BasicObjectLock::_displaced_header

    XMLWordPrintable

    Details

    • Subcomponent:
    • Resolved In Build:
      b03
    • CPU:
      aarch32
    • OS:
      generic

      Backports

        Description

        On 5/12/21 1:02 PM, Chris Cole wrote:
        > Hi,
        >
        > Not sure if this is the appropriate place or method to report an OpenJDK
        > bug, if not please advise.
        >
        > I have discovered a bug with ARM32 C1 monitor locking/unlocking that can
        > result in deadlock. The bug was introduced with JCK-8241234 "Unify monitor
        > enter/exit runtime entries" [1]. This change introduced a call to
        > ObjectSynchronizer::quick_entry() within the logic for
        > Runtime1::monitorenter().
        > If the monitor is inflated and already owned by the current thread, then
        > ObjectSynchronizer::quick_entry() simply increments
        > ObjectMonitor::_recursions and returns. In this case
        > Runtime1::monitorenter() returns to the JIT compiled code without calling
        > "lock->set_displaced_header(markWord::unused_mark())" (see [2]). For ARM32
        > the _displaced_header field is not always initialized in JIT code before
        > calling Runtime1::monitorenter() helper. If the uninitialized value of
        > _displaced_header field on stack happens to be NULL, this causes an issue
        > because the JIT code to exit the monitor first checks for a NULL
        > _displaced_header as an indication for non-inflated recursive locking which
        > is a noop for exiting the monitor (see [3]). This means that the
        > Runtime1::monitorexit() helper is not called as required to exit this
        > inflated monitor, and the ObjectMonitor::_recursions is not decremented as
        > required. This leads to thread not unlocking the monitor when required and
        > deadlock when another thread tries to lock the monitor.
        >
        > This bug is not present on AArch64 and x86, because the displaced header is
        > initialized in JIT code with the "unlocked object header" value (which is
        > non-zero) before calling Runtime1::monitorenter() helper (see [4] and [5]).
        > Note sure about other CPU architectures.
        >
        > I see two ways to fix this.
        > 1) In ObjectSynchronizer::quick_entry() move the
        > "lock->set_displaced_header(markWord::unused_mark())" statement to before
        > the "if (owner == current)" at line 340 in share/runtime/synchronizer.cpp
        > (see [6]), so that Runtime1::monitorenter() helper logic always initializes
        > the displaced header field as was the case before JCK-8241234.
        > 2) For ARM32 add JIT code to initialize the displaced header field before
        > calling Runtime1::monitorenter() helper as done for AArch64 and x86.
        >
        > Not sure which is better (or maybe both are required for some reason I am
        > not aware of). I believe this "displacted header" on the stack can be
        > looked at by stack walkers but I am not familiar with the exact details and
        > if there are implications on this fix.
        >
        > The bug is also present in OpenJDK 11.0.10 and later (introduced by the
        > backport of JDK-8241234 [1]).
        >
        > I/my company (Sage Embedded Software) has signed the Oracle Contributor
        > Agreement (OCA) and have been granted access to JCK.
        >
        > The bug can be reproduced in my environment with the OpenJDK Community TCK
        > testing of java.io.PipedReader that deadlocks, but because reproduction of
        > the issue requires uninitialized stack field to be zero, it might not
        > happen in some environments. I have a Java test case that can reproduce
        > this issue on ARM in 32 bit mode. It is pasted inline below at the end of
        > the email. There is a "getZeroOnStack()" method that I think helps get a
        > zero into the uninitialized _displaced_header field. The test case source
        > code is copied from OpenJDK java.io.PipedReader source code and then
        > modified. It needs to be run with only C1 enabled (I am using minimal
        > variant to enforce this) and the following command line options
        > (-XX:-BackgroundCompilation -XX:CompileThreshold=500
        > -XX:CompileOnly="com.sageembedded.test.MonitorBugTest::receive"). The test
        > case should run and then end with "Source thread done" and "Reading
        > complete" output if the bug is not reproduced. If the monitor bug is
        > reproduced the test case will not exit and the main thread will be
        > deadlocked, with the main thread last printing "read() before wait" and
        > missing "read() after wait" and "Reading complete". If useful I can provide
        > the output of this test cause including -XX:PrintAssebly and logging that I
        > added to ObjectSynchronizer::quick_entry() that shows uninitialized
        > lock->_displaced_header and ObjectMonitor->_recursions continue to get
        > incremented (into the 1000s) as the MonitorBugTest.receive() method is
        > called in a loop.
        >
        > Please let me know if there is anything else that would be helpful. I hope
        > to become active in the OpenJDK Community. My time is a little limited at
        > the moment, so sometimes it might take a day to respond (I have 3 and 6
        > year old kids). In the coming years I expect to have additional time to be
        > more involved in the OpenJDK Community.
        >
        > Best regards,
        > Chris Cole
        > Sage Embedded Software LLC
        >
        > [1] https://bugs.openjdk.java.net/browse/JDK-8241234
        > [2]
        > https://github.com/openjdk/jdk/blob/dfe8833f5d9a9ac59857143a86d07f85769b8eae/src/hotspot/share/runtime/synchronizer.cpp#L343
        > [3]
        > https://github.com/openjdk/jdk/blob/dfe8833f5d9a9ac59857143a86d07f85769b8eae/src/hotspot/cpu/x86/c1_MacroAssembler_x86.cpp#L130
        > [4]
        > https://github.com/openjdk/jdk/blob/71b8ad45b4de6836e3bb2716ebf136f3f8ea2198/src/hotspot/cpu/aarch64/c1_MacroAssembler_aarch64.cpp#L95
        > [5]
        > https://github.com/openjdk/jdk/blob/dfe8833f5d9a9ac59857143a86d07f85769b8eae/src/hotspot/cpu/x86/c1_MacroAssembler_x86.cpp#L74
        > [6]
        > https://github.com/openjdk/jdk/blob/dfe8833f5d9a9ac59857143a86d07f85769b8eae/src/hotspot/share/runtime/synchronizer.cpp#L340

          Attachments

            Issue Links

              Activity

                People

                Assignee:
                bulasevich Boris Ulasevich
                Reporter:
                dcubed Daniel Daugherty
                Votes:
                0 Vote for this issue
                Watchers:
                8 Start watching this issue

                  Dates

                  Created:
                  Updated:
                  Resolved: