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

[Nils Eliasson]

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