RFR (S) 8182397: Race in field updates when creating ArrayKlasses can lead to crash

[Erik Österlund]

> On 25 Jul 2017, at 19:18, coleen.phillimore at oracle.com wrote:
> 
> 
> 
> On 7/25/17 11:36 AM, Erik Österlund wrote:
>> Hi Coleen,
>> 
>> I agree that if this is accessed purely in JIT compiled code that we control, and the value is stable, i.e. goes from NULL to not NULL, and then never changes, then this seems safe without acquire in this specific case.
>> Having said that, the corresponding java_lang_Class::array_klass() function loads the klass with a non-volatile load, rather than a load_acquire. Now, it seems like the only use of ths function is in InstanceKlass::oop_print_on(), so I guess it is not crucial performance wise. It would be great if the java_lang_Class::array_klass() was safe to use in our code base so that new users that accidentally use it for more than printing will not be surprised by this piece of code and how it inconsistent to other hotspot code violates our synchronization practices of linking release_store with load_acquire, in favor of a non-volatile load.
>> 
>> Would you agree it would be beneficial to use load_acquire at least in this not so hot path in the C++ code? Then we have indisputably correct synchronization in the C++ code, can dodge the whole consume discussion (we do what every other acquire/release pair does in hotspot - business as usual), and leave the optimized case to our own JIT compilers that we trust retain the data dependency. I would be happy with that.
> 
> Yes, I checked this in but I should have done this.  I mistakenly thought the only accesses for this field were through compiled code and thought there wasn't a java_lang_Class::array_klass() accessor. But there is but it's only used for printing.
> 
> I'll file a bug to make the array_klass() use load acquire for cleanliness and send it out in a bit.  Sorry to jump the gun on pushing.

No problem, as long as we fix it. ;)

Thanks,
/Erik

> thanks,
> Coleen
> 
>> 
>> Thanks,
>> /Erik
>> 
>> On 2017-07-24 21:39, coleen.phillimore at oracle.com wrote:
>>> 
>>> Thank you for your comments, Erik and Andrew.   I like the idea of rules for innocent coders (ones who aren't versed in the complexities of architecture ordering semantics like myself), that say things like "if you have a store-release, you should have a load-acquire on the other side".  I would follow this if the load were in the C++ code.  Since it's in the JIT compiled code, I think taking advantage of the knowledge that this is a dependent load (if I understand this correctly) seems acceptable and recommended because performance counts.
>>> 
>>> Thanks,
>>> Coleen
>>> 
>>> On 7/24/17 1:40 PM, Andrew Haley wrote:
>>>> On 24/07/17 17:44, Erik Osterlund wrote:
>>>>> Sorry for jumping in to the conversation a bit late.
>>>>> Since David Holmes is on vacation, I have to take his place in
>>>>> questioning the elision of acquire on the reader side being
>>>>> okay. Because I know he would if he was here.
>>>>> 
>>>>> In hotspot we almost *never* elide the acquire on the reader side,
>>>>> relying on dependent loads.
>>>> I'm sure we do in plenty of places, it's just undocumented or maybe
>>>> even unknown to the original author.  We certainly do this a lot in
>>>> the code that the JIT compilers generate, as you note below.
>>>> 
>>>>> This would be an exception to that rule, and I do not see why this
>>>>> is warranted. In order to utilize the dependent load properties in
>>>>> shared code, we need memory ordering semantics for this. That memory
>>>>> ordering semantics is called consume in C++11, and has arguably
>>>>> turned out to be a terrible idea. Hotspot does not have a consume
>>>>> memory ordering like C++11, that allows utilizing dependent loads,
>>>>> and I do not think we intend to introduce it any time soon unless
>>>>> there are very compelling reasons to do so. In its absence, we
>>>>> should use load_acquire instead, and not micro optimize it away.
>>>>> 
>>>>> AFAIK, compiler implementers are not happy with C++11 consume memory
>>>>> ordering semantics and implement it with acquire instead as there
>>>>> are too many problems with consume.
>>>> I strongly disagree.  Consume is fine now: I think you're dealing with
>>>> out-of-date information.  It took a while to figure out how to specify
>>>> it, that's all.
>>>> 
>>>>> To elide acquire in an acquire release pair in shared code, very
>>>>> strong motivation is needed. Like for example how constructors
>>>>> publish final object references with release but relies on dependent
>>>>> load in JIT compiled code to elide acquire, because using acquire
>>>>> semantics on all reference loads is too expensive, so it is
>>>>> warranted here.
>>>> Right.  So we already depend on that property, so...
>>>> 
>>>>> Therefore, I am wondering if perhaps we want to add an acquire on
>>>>> the load side anyway, so we do not have to introduce consume memory
>>>>> ordering semantics or something similar in the shared code
>>>>> base. Using a normal load without any memory ordering semantics in
>>>>> shared code to act like a load consume rings alarm clocks in my
>>>>> head.
>>>> I don't think it's that much of a big deal, really.  Putting in an
>>>> acquire fence every time we read a java mirror strikes me as madness
>>>> when consume is all we need on any target.  Even Power PC doesn't need
>>>> a fence instruction there.  At least make sure that targets can turn
>>>> that off: we should not pay the price for something we don't need.
>>>> 
>>> 
>> 
>