Review Request (S) 8006542: JSR 292: the VM_RedefineClasses::append_entry() must support invokedynamic entry kinds

[Coleen Phillimore]

This looks really good!  Thanks for doing the refactoring!
Coleen

On 1/28/2013 11:28 PM, serguei.spitsyn at oracle.com wrote:
> Coleen,
>
> This is a version with a refactoring you requested:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/8006542-JVMTI-JSR292.3/ 
>
>
> Now, the NameAndType, FieldRef, InterfaceMethodRef and MethodRef use 
> the find_or_append_indirect_entry().
>
>
> Thanks,
> Serguei
>
>
> On 1/28/13 2:18 PM, serguei.spitsyn at oracle.com wrote:
>> Hi Coleen,
>>
>>
>> This is a new webrev:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/8006542-JVMTI-JSR292.2/ 
>>
>>
>> As you pointed out, the InvokeDynamic entry support should also do 
>> cross-checks with
>> the bootstrap method operands (arguments) and recursive extra appends 
>> if necessary.
>>
>> I've filed a separate bug to track this issue:
>>     https://jbs.oracle.com/bugs/browse/JDK-8007037
>>
>> I want to separate this issue to be able to integrate what I have now 
>> and concentrate on the rest later.
>> The VM SQE team developed new INDY tests that cover the 
>> RedefineClasses issues.
>> I hope to adopt and use new tests soon to make sure most of the 
>> issues are actually resolved.
>>
>>
>> Thanks,
>> Serguei
>>
>>
>> On 1/24/13 5:23 PM, serguei.spitsyn at oracle.com wrote:
>>> Hi Coleen,
>>>
>>>
>>> Thank you a lot for the review!
>>>
>>> On 1/24/13 3:55 PM, Coleen Phillimore wrote:
>>>>
>>>> Hi Serguei,
>>>>
>>>> Putting functions in alphabetical order seems silly, it's better to 
>>>> have utility functions directly above (or below) the function that 
>>>> calls them.  I'd take out the comment.
>>>>
>>>> I have started looking at this code a bit.  This function 
>>>> find_or_append_indirect_entry() should be used for the other 
>>>> indirect entries also, shouldn't it?  The code looks familiar to 
>>>> the inside of the case statement to FieldRef, InterfaceRef and 
>>>> MethodRef.
>>>
>>> You've got it right.
>>> Yes, I noticed it but did not want to mess with it until it is 
>>> proven to work well.
>>> My plan was to fix it for the FieldRef, InterfaceRef and MethodRef 
>>> in a next round of fixes.
>>>
>>>>
>>>> Also is boot_spec an indirect index too that has to be appended?
>>>>
>>>>  564       int boot_spec = 
>>>> scratch_cp->invoke_dynamic_bootstrap_method_ref_index_at(scratch_i);
>>>
>>> This is a nice catch, will fix it.
>>> I thought, it is an index into the operands array, but it refers to 
>>> a CONSTANT_MethodHandle element.
>>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>>
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>> On 01/24/2013 05:56 PM, serguei.spitsyn at oracle.com wrote:
>>>>> Christian,
>>>>>
>>>>>
>>>>> Thank you a lot for the review!
>>>>>
>>>>> Nice catch with the ordering.
>>>>> In fact, I tried to follow the order but missed that the 
>>>>> find_new_index should go first. :)
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 1/24/13 2:14 PM, Christian Thalinger wrote:
>>>>>> On Jan 22, 2013, at 4:07 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>
>>>>>>> Please, review the fix for:
>>>>>>> https://jbs.oracle.com/bugs/browse/JDK-8006542
>>>>>>>
>>>>>>>
>>>>>>> Open webrev:
>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/8006542-JVMTI-JSR292.1/ 
>>>>>>>
>>>>>> src/share/vm/prims/jvmtiRedefineClasses.hpp:
>>>>>>
>>>>>> +  // Support for constant pool merging (these routines are in 
>>>>>> alpha order):
>>>>>>     void append_entry(constantPoolHandle scratch_cp, int scratch_i,
>>>>>>       constantPoolHandle *merge_cp_p, int *merge_cp_length_p, 
>>>>>> TRAPS);
>>>>>> +  int find_or_append_indirect_entry(constantPoolHandle 
>>>>>> scratch_cp, int scratch_i,
>>>>>> +    constantPoolHandle *merge_cp_p, int *merge_cp_length_p, TRAPS);
>>>>>>     int find_new_index(int old_index);
>>>>>>
>>>>>> Not really alpha order ;-)
>>>>>>
>>>>>> Otherwise this looks good (as far as I can tell).
>>>>>>
>>>>>> -- Chris
>>>>>>
>>>>>>> Summary:
>>>>>>>   Need a support for invokedynamic entry kinds when new and old 
>>>>>>> constant pools are merged.
>>>>>>>
>>>>>>> Testing:
>>>>>>>   vm/mlvm/indy/func/jvmti/redefineClassInBootstrap
>>>>>>>   Asked the VM SQE team to develop new INDY tests for better 
>>>>>>> coverage.
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>