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

[David Holmes]

On 8/01/2019 11:08 pm, Erik Österlund wrote:
> 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.

Okay it's late here and I'm tired, but this seems excessively 
conservative. Atomic::store should only be needed for 64-bit non-pointer 
types (in case of 32-bit system) or an unaligned access that isn't 
guaranteed atomic by the platform. Otherwise we'd need Atomic::store and 
Atomic::load all over the place!

David

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