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

[Andrew Dinn]

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();
}

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.

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