RFR (S) 8182397: Race in field updates when creating ArrayKlasses can lead to crash

[coleen.phillimore at oracle.com]

Andrew,  Thank you for your comments and suggestions.

On 7/21/17 9:47 AM, Andrew Dinn wrote:
> Hi Coleen,
>
> On 21/07/17 00:44, coleen.phillimore at oracle.com wrote:
>> Summary: Update array_klass field in component mirror after
>> klass.java_mirror field for concurrent readers in compiled code.
>>
>> See bug for details.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8182397.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8182397
>>
>> Ran tier1 testing with -Xcomp and without.  Thank you to Tom for the
>> test and diagnosis.
> The re-ordering of the writes to
>
>    k->set_java_mirror(mirror());
>    . . .
>    set_array_klass(comp_mirror(), k);
>
> is all that is needed, assuming that the second write has release
> semantics. I discussed this with Andrew Haley and he confirmed my
> assumption (also Tom's) that on AArch64 there is no need for a load
> acquire on the reader side because of the address dependency between any
> load of the component mirror's array klass k and ensuing load of k's mirror.
>
> However, I think your implementation of method
> metadata_field_put_volatile() is overkill. You have
>
> void oopDesc::metadata_field_put_volatile(int offset, Metadata* value) {
>    OrderAccess::release();
>    *metadata_field_addr(offset) = value;
>    OrderAccess::fence();
> }

I copied this from

void oopDesc::obj_field_put_volatile(int offset, oop value) {
   OrderAccess::release();
   obj_field_put(offset, value);
   OrderAccess::fence();
}

Is it overkill also in the obj_field_put_volatile case?
>
> That fence() will impose a full memory barrier which is more than is
> actually needed. It should be ok just to use another release.
>
> void oopDesc::metadata_field_put_volatile(int offset, Metadata* value) {
>    OrderAccess::release();
>    *metadata_field_addr(offset) = value;
>    OrderAccess::release();
> }
>
> Alternatively, I believe you could implement the assign of using a call
> to OrderAccess::store_release_ptr which would achieve the same outcome
> on AArch64.

This is nicer.  Is this what you mean?

void oopDesc::metadata_field_put_volatile(int offset, Metadata* value) {
   OrderAccess::release_store_ptr(metadata_field_addr(offset), value);
}


Thanks!
Coleen
>
> regards,
>
>
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander