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

[Daniel D. Daugherty]

I found an existing test that exercises the add side of the
house, but haven't found one that exercises the delete side (yet):

$ pwd
/work/shared/mirrors/src_clones/jdk9/dev_baseline

di$ ls jdk/test/java/lang/instrument/RedefineMethodAddInvoke*
jdk/test/java/lang/instrument/RedefineMethodAddInvoke.sh
jdk/test/java/lang/instrument/RedefineMethodAddInvokeAgent.java
jdk/test/java/lang/instrument/RedefineMethodAddInvokeApp.java
jdk/test/java/lang/instrument/RedefineMethodAddInvokeTarget.java
jdk/test/java/lang/instrument/RedefineMethodAddInvokeTarget_1.java
jdk/test/java/lang/instrument/RedefineMethodAddInvokeTarget_2.java

Dan


On 5/14/14 11:30 AM, serguei.spitsyn at oracle.com wrote:
> Hi Coleen,
>
> Thank you for the review and analysis!
> I'm working on adding a test coverage for this issue.
>
> 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
>>>>
>>>
>>
>