[14] RFR (XS): 8235305: Corrupted oops embedded in nmethods due to parallel modification during optional evacuation

[Thomas Schatzl]

Hi Stefan + Stefan,

   thanks for your reviews :)

Thomas

On 20.01.20 12:28, Stefan Karlsson wrote:
> Looks good.
> 
> StefanK
> 
> On 2020-01-17 14:27, Thomas Schatzl wrote:
>> Hi Stefan,
>>
>> On 17.01.20 10:06, Stefan Johansson wrote:
>>> Hi Thomas,
>>>
>>> On 2020-01-16 14:20, Thomas Schatzl wrote:
>>>> Hi all,
>>>>
>>>>    can I get reviews for this change that fixes a bug in the 
>>>> abortable mixed gc algorithm where G1 might corrupt oops embedded in 
>>>> nmethods due to parallel modification during an optional evacuation 
>>>> phase?
>>>>
>>>> G1 currently collects embedded oops in nmethods twice: once in the 
>>>> optional roots list, and once as nmethods in the strong code roots 
>>>> list for a particular region.
>>>>
>>>> Now it can happen that this oop embedded in in the code stream is 
>>>> unaligned, so if that oop is modified during relocation word tearing 
>>>> may occur, causing follow-up crashes.
>>>>
>>>> The fix is to not collect oops from nmethods in the optional code 
>>>> root list as the strong code root list for a particular region 
>>>> already always contains it anyway.
>>>>
>>>> Thanks go to stefank, eriko and sjohanss for helping with analyzing, 
>>>> testing and the discussion around it.
>>>>
>>>> CR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8235305
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~tschatzl/8235305/webrev/
>>>
>>> Fix looks good. 
>>
>> Thanks for your review.
>>
>>> Just some things around the naming of the template parameter and enum 
>>> after adding this. I don't have a much better idea
>> [...]
>>
>> Talked to them about this and I'm good with their suggestion:
>>
>> http://cr.openjdk.java.net/~tschatzl/8235305/webrev.1 (full)
>> http://cr.openjdk.java.net/~tschatzl/8235305/webrev.0_to_1 (diff)
>>
>> Thanks,
>>    Thomas
>>