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

[serguei.spitsyn at oracle.com]

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