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

[Coleen Phillimore]

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