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

[Erik Österlund]

Hi David,

On 2019-01-08 14:15, David Holmes wrote:
> 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!

Right. The compiler is nice enough to give us atomic accesses as you 
mention. But since C++11 (which we are planning to upgrade past soonish 
I think), the standard is explicit about stating you can't assume you 
will not get word tearing on volatile accesses, even on naturally 
aligned word sized primitives. Only with native C++ atomics do you get 
those guarantees. Personally I think that is a really evil compatibility 
issue. I guess in practice compilers continue working fine anyway, 
because there is no good reason to break code just for the fun of it.

Anyway, this is why in code I write, I try to use Atomic::load/store as 
good practice when I need atomicity, to reduce headache later on when we 
reroute Atomic::load/store to std::atomic.

/Erik

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