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

[Vladimir Kozlov]

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