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

[Andrew Dinn]

On 21/07/17 15:23, coleen.phillimore at oracle.com wrote:
> Andrew,  Thank you for your comments and suggestions.

All jus' part of the job, ma'am ... :-)

> 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?

Well, I would assume from the name that obj_field_put_volatile is used
to do a volatile write internally from the JVM with the intention that
it be equivalent to doing a volatile write from JDK code.

If so then, in general, I /guess/ that might perhaps demand a full fence
even though on some platforms (like AArch64) we /can/ and /do/ implement
volatile writes in JITted code using only a releasing store. Andrew
Haley might know better on this score.

That said, the method you have defined is only (currently) used to
ensure that the two stores for the mirror and array klass are ordered
correctly as regards a read through from klass to mirror. That usage
definitely only needs a releasing store (assuming the memory system
architecture respects address dependencies).

In which case it might be more appropriate to rename the method you
inserted to metadata_field_put_release -- highlighting the fact that
this is all the order guarantee it provides.

>> 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);
> }
Yes, that was what I was thinking of.

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