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

[serguei.spitsyn at oracle.com]

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