RFR (S) 8225681: vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine fails due a) MT-unsafe modification of inline cache

[Erik Osterlund]

Hi Coleen,

> On 27 Sep 2019, at 15:46, coleen.phillimore at oracle.com wrote:
> 
> 
> 
>> On 9/27/19 9:20 AM, erik.osterlund at oracle.com wrote:
>> For any race to be observed, wouldn't we need to have concurrent writes not protected by the CompiledICLocker? If that indeed happens, then that seems like a bug, right? Or maybe I'm missing something.
> 
> I can't actually answer that because I didn't observe the races. Was this always protected by the CompiledICLocker?

Yes.

> I can remove the volatiles if they are protected by the CompiledICLocker.

That would be great.

Thanks,
/Erik

> Coleen
> 
>> 
>> /Erik
>> 
>>> On 9/27/19 2:18 PM, coleen.phillimore at oracle.com wrote:
>>> 
>>> 
>>>> On 9/27/19 8:00 AM, erik.osterlund at oracle.com wrote:
>>>> Hi Coleen,
>>>> 
>>>> I don't understand this. It makes the frame-local variable volatile. But surely the frame local variable is accessed only by the current thread. And the whole thing is protected by the CompiledICLocker. Would you mind explaining what we are worried about here?
>>> 
>>> The worry is that old_method would be fetched multiple times from data() in the assert.  Another thread could be updating data(), if I'm not mistaken.  Making destination volatile might not be necessary, but it was like that in s390, which experienced the race.
>>> 
>>> Coleen
>>> 
>>>> 
>>>> Thanks,
>>>> /Erik
>>>> 
>>>>> On 9/26/19 5:59 PM, coleen.phillimore at oracle.com wrote:
>>>>> 
>>>>> For the record, I added volatiles after discussions with Goetz:
>>>>> 
>>>>> http://cr.openjdk.java.net/~coleenp/2019/8225681.02/webrev
>>>>> 
>>>>> Which builds on s390 and ppc, and verified on Oracle platforms as well.
>>>>> 
>>>>> Coleen
>>>>> 
>>>>>> On 9/26/19 7:24 AM, coleen.phillimore at oracle.com wrote:
>>>>>> Thanks Erik!
>>>>>> Coleen
>>>>>> 
>>>>>>> On 9/26/19 5:47 AM, erik.osterlund at oracle.com wrote:
>>>>>>> Hi Coleen,
>>>>>>> 
>>>>>>> Looks good.
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> /Erik
>>>>>>> 
>>>>>>>> On 9/25/19 11:22 PM, coleen.phillimore at oracle.com wrote:
>>>>>>>> Summary: allow old methods in CompiledStaticDirectCall::set_to_interpreted
>>>>>>>> 
>>>>>>>> This is the comment in the bug that describes this race and this fix:
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8225681?focusedCommentId=14278441&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14278441 
>>>>>>>> 
>>>>>>>> The rest of the bug and sightings are actually caused by https://bugs.openjdk.java.net/browse/JDK-8226690,
>>>>>>>> and this one might have been caused by it also, but the race that Erik describes is possible as well.
>>>>>>>> 
>>>>>>>> The s390 code had an exception for callee->is_compiled_lambda_form() which should probably apply to all the platforms, so the code is the same on all the platforms with this change.
>>>>>>>> 
>>>>>>>> Tested with tier1-6.
>>>>>>>> 
>>>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/2019/8225681.01/webrev
>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8225681
>>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> Coleen
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
>