RFR (S) 8042796: jvmtiRedefineClasses.cpp: guarantee(false) failed: OLD and/or OBSOLETE method(s) found

[serguei.spitsyn at oracle.com]

Thank you, Coleen!
Serguei

On 5/15/14 1:24 PM, Coleen Phillimore wrote:
>
> Serguei,
>
> This looks good.
> We should eventually extend these tests to have jmethodIDs and stuff 
> like that.
>
> Yes, you have to push the hotspot fix first, then wait a week or so to 
> push the JDK one since pushes aren't coordinated just yet, and you 
> don't want jdk to have a failing test until hotspot is integrated.
>
> Thanks for figuring this out and the discussion!
>
> Coleen
>
> On 5/15/14, 1:21 PM, serguei.spitsyn at oracle.com wrote:
>> Hi Coleen and Dan,
>>
>> I've created a j.l.i test covering this issue.
>> The jdk webrev is:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/jdk/8042796-JVMTI-del-tests.1 
>>
>>
>> The updated hotspot webrev is:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8042796-JVMTI-del.1 
>>
>>
>> I guess, it is Ok to push the hotspot fix while the test is reviewed?
>>
>> Thanks,
>> Serguei
>>
>>
>> On 5/14/14 9:47 AM, Coleen Phillimore wrote:
>>>
>>> Hi Serguei,
>>>
>>> On 5/13/14, 8:31 PM, serguei.spitsyn at oracle.com wrote:
>>>> Coleen,
>>>>
>>>>
>>>> On 5/13/14 4:17 PM, Coleen Phillimore wrote:
>>>>>
>>>>> Serguei,
>>>>>
>>>>> I don't know if this is the right fix.  We shouldn't have deleted 
>>>>> methods in the cpCache because there's no way to keep these 
>>>>> Method*s from being deallocated.  I thought 
>>>>> AdjustCpoolCacheAndVtable => 
>>>>> ConstantPoolCache::adjust_method_entries should fix these entries. 
>>>>> For deleted methods, shouldn't their cpCache entries be set to 
>>>>> unresolved?  Which class had the deleted methods? The one being 
>>>>> redefined?
>>>>
>>>> Thank you for reviewing this and for the questions.
>>>>
>>>> Let's check if my understanding is right as it is relatively easy 
>>>> to miss something.
>>>>
>>>> The original class has been redefined so that the new version does 
>>>> not have several private static methods anymore.
>>>> These methods are called "deleted" but they are still present in 
>>>> the original version of the class.
>>>> Even if we set their cpCache entries to unresolved these entries 
>>>> can be set to resolved again.
>>>> So that I've decided this approach is not going to work in general 
>>>> case.
>>>
>>> Okay, so you answered my question - since the deleted methods are 
>>> private, they can only be found in the class being redefined's 
>>> constant pool cache.   Since the class being redefined's cpCache 
>>> hasn't been deallocated, neither will the Method* that it's pointing 
>>> to.   Phew!   Thank you for the explanation.
>>>
>>>>
>>>> The interesting question is:
>>>>  Q: What is going to happen with these entries when the original 
>>>> version of the class goes away?
>>>>
>>>
>>> Everything will get deallocated, which is fine.
>>>
>>>> I'll try to answer this question by looking at the code.
>>>> It won't be a surprise if there is nothing in place to cover this 
>>>> case.
>>>>
>>>>>
>>>>> It looks like the code never did this.  I think this is missing 
>>>>> functionality.
>>>>
>>>> Yes, that was my guess too.
>>>>
>>>>> Do we not have any tests that delete a method?
>>>>
>>>> Most likely, we don't.
>>>>
>>>>> Your change should add a test anyway - there are class 
>>>>> redefinition tests in jdk/test/java/lang/instrument that can be 
>>>>> adapted and we should probably keep them all together there.
>>>>
>>>> Right, it is better to have some test coverage here.
>>>> I'll check if any of the JLI or JDI tests can be extended to cover 
>>>> this.
>>>> I'm not sure the JLI is good for this task though as deleting the 
>>>> methods is not instrumentation. :)
>>>>
>>>
>>> So the change looks good pending a test case.  There might be one 
>>> already but why didn't it fail this assert?  So, we need a test for 
>>> this case somewhere.
>>>
>>> Thanks,
>>> Coleen
>>>
>>>>
>>>> Thanks!
>>>> Serguei
>>>>
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>>
>>>>> On 5/9/14, 2:20 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> Please, review the fix for:
>>>>>>   https://bugs.openjdk.java.net/browse/JDK-8042796
>>>>>>
>>>>>>
>>>>>> Open webrev:
>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8042796-JVMTI-OLD.1 
>>>>>>
>>>>>>
>>>>>> Summary:
>>>>>>
>>>>>>   This is a Nightly Stabilization issue that was caused by a 
>>>>>> combination of two problems:
>>>>>>     - A regression introduced by the fix of: 
>>>>>> https://bugs.openjdk.java.net/browse/JDK-7182152
>>>>>>     - An SQE testbase infra regression: 
>>>>>> https://bugs.openjdk.java.net/browse/INTJDK-7611018
>>>>>>
>>>>>>   A number of the vm.mlvm tests hits this guarantee taht was 
>>>>>> added by 7182152 (must be relaxed).
>>>>>>   The issue is with the deleted static private methods that are 
>>>>>> still present in the CP cache.
>>>>>>   The fix is to mark the deleted methods with the flag 
>>>>>> JVM_ACC_IS_DELETED and
>>>>>>   then use it to relax the guarantee condition.
>>>>>>
>>>>>>
>>>>>> Testing:
>>>>>>   Running the failing tests: vm.mlvm.indy.func.jvmti
>>>>>>   In progress: nsk.jvmti, nsk.jdi, java.lang.instrument test runs 
>>>>>> on sparcv9 and amd64.
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>
>>>>
>>>
>>
>