RFR: JDK-8035663 Suspicious failure of test java/util/concurrent/Phaser/FickleRegister.java

[David Holmes]

Thanks Coleen!

David

On 26/11/2014 9:05 AM, Coleen Phillimore wrote:
>
> David,
>
> I think this code looks good.  The mutex seems like the right approach.
> Thank you for fixing this.  It looked really broken now that you've
> described it in such readable detail.
>
> Coleen
>
> On 11/19/14, 12:17 AM, David Holmes wrote:
>> webrev:
>>
>> http://cr.openjdk.java.net/~dholmes/8035663/webrev.jdk9/
>>
>> This test failure exposed a number of issues with the logic in
>> unsafe.cpp for handling atomic updates of Java long fields on
>> platforms without any direct support for a 64-bit CAS operation -
>> platforms for which supports_cx8 is not true. This only impacts our SE
>> Embedded PPC32 platform (where we have been using this fix for some
>> time now) but in case other such platforms came along I wanted to get
>> this pushed to mainline.
>>
>> What the unsafe code did was to use the object containing the field as
>> a lock object for reading and writing the field. This seems reasonable
>> on the surface but in fact had a fatal flaw - because we were locking
>> a Java-level visible object inside what was considered to be a
>> lock-free code-path by the application and library logic, we could
>> actually induce a deadlock - which is why the test failed.
>>
>> In addition the code had two further flaws:
>>
>> 1. Because the field could also be updated via direct assignment in
>> Java code the unsafe code needed to perform an Atomic::load of the
>> field. And for good measure we also employ an Atomic::store to ensure
>> no interference with direct reads of the field in Java code.
>>
>> 2. The address of the field was being calculated before using the
>> ObjectLocker to lock the object, but locking could encounter a
>> safepoint check allowing the object to relocated by the GC, and we
>> would then use a stale address.
>>
>> To fix all of this we:
>> - introduce a special Mutex to use instead of the deadlock-inducing
>> Java object
>> - use Atomic::load and Atomic::store to access the jlong field
>> - avoid safepoints when locking (alternatively you could ensure you
>> calculate the address after acquiring the lock )
>>
>> Thanks,
>> David
>