RFR (S) 8215575: C2 crash: assert(get_instanceKlass()->is_loaded()) failed: must be at least loaded

[Erik Österlund]

Hi David,

On 2019-01-08 13:58, David Holmes wrote:
> On 8/01/2019 9:59 pm, Erik Österlund wrote:
>> Hi David,
>>
>> The required synchronization is that the _subklass link is 
>> read/written with at least acquire/release semantics, 
>> correspondingly. And now they are. (when appending, the link gets 
>> written with a conservative CAS, and the link is loaded with 
>> load_acquire).
>
> Okay. I took a look inside append_to_sibling_list and see there is 
> lots of ordering control in there.
>
> Aside: why do you need an Atomic::store in set_next_sibling ??

Because it is read concurrently. Despite being read concurrently, they 
do not need load_acquire, because the entries read are strictly older 
than the entry you call it on, due to prepending in the list. So 
therefore, the acquire of the _subklass link protects both the _subklass 
and all _siblings it has. But I still want the atomicity to avoid e.g. 
word tearing. Now you won't get word tearing anyway because compilers 
are nice to us, but by annotating it as Atomic, we can eventually get 
better guarantees about that once we plug in Atomic to C++11 atomics.

Thanks,
/Erik

> Thanks,
> David
>
>> Thanks,
>> /Erik
>>
>> On 2019-01-08 02:49, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> On 8/01/2019 5:50 am, coleen.phillimore at oracle.com wrote:
>>>> Summary: Set InstanceKlass::loaded before adding classes to the 
>>>> subklass list, which can be read concurrently by the compiler.
>>>
>>> I think you need a storestore barrier to ensure the new order is 
>>> preserved.
>>>
>>> Cheers,
>>> David
>>>
>>>> Thanks to Erik for the diagnosis and suggested fix.  See bug 
>>>> comments for more details.
>>>>
>>>> Tested with hs-tier1-3, 6 and 8.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8215575.01/webrev
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8215575
>>>>
>>>> Thanks,
>>>> Coleen
>>