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

[serguei.spitsyn at oracle.com]

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