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

[Coleen Phillimore]

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.

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


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