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

[serguei.spitsyn at oracle.com]

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