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

[coleen.phillimore at oracle.com]

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