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

[coleen.phillimore at oracle.com]

On 1/8/19 9:14 AM, Erik Österlund wrote:
> 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.

This is discomforting, unless the compiler warns you about your volatile 
accesses without atomics.

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