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

[Erik Österlund]

Hi Coleen,

On 2019-01-09 00:21, coleen.phillimore at oracle.com wrote:
>
>
> 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.

That is discomforting indeed. The compiler does not warn about using 
volatile, because volatile is perfectly well defined, just not for 
concurrency at all. And the compiler doesn't know that you are using it 
for that. So instead, this is treated like all other C++ undefined 
behaviour: you have to just remember the whole standard in the back of 
your head all the time when you are coding, max out on your coffee, and 
figure out that things are undefined behaviour and we are really not 
allowed to do it, despite compilers saying everything is fine. And it 
will probably take us years to get rid of these assumptions. That's why 
I'm already annotating the intentions in my code to make that easier. 
Hopefully compilers give us a break until we get there.

/Erik

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