RFR(S): 8219642: ciReplay loads wrong data when MethodData size changes

[Nils Eliasson]

Thank you Vladimir!

// Nils

On 2019-02-28 19:03, Vladimir Kozlov wrote:
> Okay, I agree with your arguments. Reviewed.
>
> Thanks,
> Vladimir
>
> On 2/28/19 5:38 AM, Nils Eliasson wrote:
>>
>> On 2019-02-26 18:59, Vladimir Kozlov wrote:
>>> On 2/26/19 1:14 AM, Nils Eliasson wrote:
>>>> Hi Vladimir,
>>>>
>>>> There is a mutex in a MethodData, it changed size with the removal 
>>>> of sneaky looking:
>>>>
>>>> http://hg.openjdk.java.net/jdk/jdk/file/554c3c813ad6/src/hotspot/share/oops/methodData.hpp#l1971 
>>>>
>>>>
>>>> When a replay files is written - we take the size of the MethodData 
>>>> here:
>>>>
>>>> http://hg.openjdk.java.net/jdk/jdk/file/554c3c813ad6/src/hotspot/share/ci/ciMethodData.cpp#l685 
>>>>
>>>>
>>>> Then we serializes it as a char* array.
>>>>
>>>> Without my change, we trust the data without verifying it, and end 
>>>> up reading wrong trap data.
>>>
>>> I understand what you are talking about. But as I stated before, 
>>> ciReplay was not designed to support multiply JVM versions. It is 
>>> not only MethodData but also other structures/data used by JIT could 
>>> be different.
>>>
>>> On other hand it is our tool and I am fine to make it more flexible.
>>>
>>> My concern about you changes changes is part where you remove 
>>> existing data. I think we should bailout in this case.
>>> I am fine with padding - is it enough to fix the issue?
>>
>> I know it isn't supposed to be a perfect tool between version, but 
>> this time it prevented me from reproducing bugs on both 12 and 13 
>> during the rampdown.
>>
>> The Mutex got smaller when we dropped lock sneaking - so dropping 
>> data is the most important case. I also hope the added warnings will 
>> be a good hint the next time the MethodData changes.
>>
>> Regards,
>>
>> Nils
>>
>>
>>>
>>> Thanks,
>>> Vladimir
>>>
>>>>
>>>> // Nils
>>>>
>>>>
>>>> On 2019-02-25 19:10, Vladimir Kozlov wrote:
>>>>> Hi Nils,
>>>>>
>>>>> Is it happening because different version of VM is used for 
>>>>> replay? Or different flags?
>>>>> ciReplay doesn't support mixing different versions because of 
>>>>> differences in data.
>>>>>
>>>>> What information is removed in MD? we should not remove any 
>>>>> profiling information and traps information.
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> On 2/25/19 5:16 AM, Nils Eliasson wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I stumbled upon this problem when trying to 
>>>>>> reproducehttps://bugs.openjdk.java.net/browse/JDK-8219448on JDK 
>>>>>> 13. The crash was recorded on a late 12 build, but the issue 
>>>>>> doesn't reproduce on 13. A bisection revealed that JDK-8210832 
>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8210832>"Remove sneaky 
>>>>>> locking in class Monitor" caused the problem, at it doesn't even 
>>>>>> touch the compilers.
>>>>>>
>>>>>> The problem is that when ciReplay serializes a ciMethodData it 
>>>>>> will serialize a MethodData as an array preceded by the size.
>>>>>> But a MethodData contains an inlined Mutex, and its size changed 
>>>>>> with the removal of sneaky locking.
>>>>>>
>>>>>> This fix adds code for detecting a size change of MethodData, and 
>>>>>> tries to recover by adding padding or dropping data. Since all 
>>>>>> non significant serialization data are in the beginning, the 
>>>>>> padding or dropping of data is done from the start.
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8219642
>>>>>>
>>>>>> http://cr.openjdk.java.net/~neliasso/8219642/webrev.01/
>>>>>>
>>>>>> Please review,
>>>>>>
>>>>>> Nils Eliasson
>>>>>>
>>>>>>
>>>>>>