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

[Coleen Phillimore]

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