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

[Kim Barrett]

> On Jul 20, 2017, at 7:44 PM, 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.
> 
> Thanks,
> Coleen

------------------------------------------------------------------------------  
src/share/vm/classfile/javaClasses.cpp 
 888     k->set_java_mirror(mirror());

This was protected by "if (k != NULL)", but you removed that.  That
removal seems correct, as there are lots of preceeding uses of k that
are wrong if it's NULL.  However, the similar "if" on line 837 also
seems unnecessary and even more confusing now.

------------------------------------------------------------------------------ 
src/share/vm/classfile/javaClasses.cpp 
 862       // Set after k->java_mirror() is published, because compiled code running
 863       // concurrently doesn't expect a k to have a null java_mirror.
 864       // set_array_klass(comp_mirror, k);

Rather than this new comment and the commented out set_array_klass, I
would rather there be a comment here directing to the later
set_java_mirror and set_array_klass pair, and a comment with that pair
discussing the importance of the order.

------------------------------------------------------------------------------ 
src/share/vm/oops/oop.inline.hpp
 449 void oopDesc::metadata_field_put_volatile(int offset, Metadata* value) {
 450   OrderAccess::release();
 451   *metadata_field_addr(offset) = value;
 452   OrderAccess::fence();
 453 }

Why not use OrderAccess::release_store_ptr_fence?

While I see why we need the release, I don't see any explanation for
why we need the trailing fence.

Oh, I see.  I'm looking at a webrev that hasn't incorporated some of
the later discussion.  Note that the usual naming convention seems to
be to to use a "release_" prefix for this sort of thing.  See, for
example, release_XXX_field_put.

Also, renaming set_array_klass to release_set_array_klass seems called
for.  There are only two callers, one of which I think doesn't care.

------------------------------------------------------------------------------ 
test/runtime/CreateMirror/ArraysNewInstanceBug.java

I'm guessing this test is subject to false negatives?  That is, it may
or may not crash when the bug exists, but won't crash if the bug is
fixed?  If so, that seems worth mentioning in a comment, or maybe the
@summary.

------------------------------------------------------------------------------