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

[coleen.phillimore at oracle.com]

New webrev, see below:
http://cr.openjdk.java.net/~coleenp/8182397.02/webrev/index.html

On 7/21/17 10:26 PM, Kim Barrett wrote:
>> 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.

Interesting.  k can be NULL for primitive types but these mirrors are 
created in create_basic_type_mirror now.  The conditionals weren't taken 
out when that function was added.

Added at the beginning of create_mirror:
   assert(k != NULL, "Use create_basic_type_mirror for primitive types");


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

How about:

       set_component_mirror(mirror(), comp_mirror());
       // See below for ordering dependencies between field array_klass 
in component mirror
       // and java_mirror in this klass.

and this comment is moved to:

     // Setup indirection from klass->mirror
     // after any exceptions can happen during allocations.
     k->set_java_mirror(mirror());
     if (comp_mirror() != NULL) {
       // Set after k->java_mirror() is published, because compiled code 
running
       // concurrently doesn't expect a k to have a null java_mirror.
       set_array_klass(comp_mirror(), k);
     }

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

Yes, I made the minor change without a new webrev.

There are both conventions.   There's obj_field_put_volatile, which I 
used this convention.  There's also release_obj_field_put, which seems 
more appropriate for this metadata_field_put function, ie. changed to 
release_metadata_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.

Sure, fixed.
>
> ------------------------------------------------------------------------------
> 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.
>
> ------------------------------------------------------------------------------
>

The test always crashes with the bug present.

Thanks,
Coleen