hs25 review request (round 2): 8007037 JSR 292: the VM_RedefineClasses::append_entry() should do cross-checks with indy operands

[serguei.spitsyn at oracle.com]

Hi Christian,


On 4/17/13 4:06 PM, Christian Thalinger wrote:
> On Apr 11, 2013, at 4:08 PM, serguei.spitsyn at oracle.com wrote:
>
>> Please, review the hs25 fix (round 2) below.
>>
>> Open webrev:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/8007037-JVMTI-JSR292.2/
> One thing that makes it hard to follow what going on are the variable names, e.g.:
>
> new_bs_i
> old_bs_i
> merge_ops
> found_i
> argc (unusual usage of that name :-)
> …
>
> I suppose _i is for index?  bs for bootstrap?  I don't expect you to change this; just pointing out.
Yes, _i stands for index and bs is for bootstrap.
For most of these names I just follow the style that was in that file.

> src/share/vm/prims/jvmtiRedefineClasses.cpp:
>
> +// value by seaching the index map. Returns zero (-1) if there is no mapped
>
> "Returns zero"?

Nice catch, I'll fix it. It must be just -1.

>
> The change seems to be fine.  For correctness I really rely on your testing here.

Thanks a lot for reviewing, I really appreciate it!

Thanks,
Serguei
>
> -- Chris
>
>> CR:
>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8007037
>> https://jbs.oracle.com/bugs/browse/JDK-8007037
>>
>>
>> This webrev includes a review fix from Coleen about explicit deallocation
>> of the old operands array in ConstantPool::resize_operands() and a couple
>> of fixes for the corner case when the old CP has no operands array:
>>
>> 1121 void ConstantPool::extend_operands(constantPoolHandle ext_cp, TRAPS) {
>> . . .
>> 1130   if (operand_array_length(operands()) == 0) {
>> 1131     ClassLoaderData* loader_data = pool_holder()->class_loader_data();
>> 1132     Array<u2>* new_ops = MetadataFactory::new_array<u2>(loader_data, delta_size, CHECK);
>> 1133     // The first element index defines the offset of second part
>> +        operand_offset_at_put(new_ops, 0, 2*delta_len); // offset in new array
>> 1135     set_operands(new_ops);
>> 1136   } else {
>> 1137     resize_operands(delta_len, delta_size, CHECK);
>> 1138   }
>> 1139
>> 1140 } // end extend_operands()
>>
>>
>> void VM_RedefineClasses::append_operand(constantPoolHandle scratch_cp, int old_bs_i,
>> 505        constantPoolHandle *merge_cp_p, int *merge_cp_length_p, TRAPS) {
>> . . .
>>
>> - 518   int new_base = (*merge_cp_p)->operand_next_offset_at(new_bs_i);
>>
>> --
>>
>> + 518   // We have _operands_cur_length == 0 when the merge_cp operands is empty yet.
>> + 519   // However, the operand_offset_at(0) was set in the extend_operands() call.
>> + 520   int new_base = (new_bs_i == 0) ? (*merge_cp_p)->operand_offset_at(0)
>> + 521                                  : (*merge_cp_p)->operand_next_offset_at(new_bs_i - 1);
>>
>>
>>
>> Description:
>>
>> References from INDY bootstrap method specifier operands to CP entries
>> and back must be correctly merged at class redefinition.
>>
>> Some background.
>>
>> An invokedynamic bytecode spec:
>> http://docs.oracle.com/javase/specs/jvms/se7/html/jvms-6.html#jvms-6.5.invokedynamic
>>
>> A invokedynamic instruction has an argument which is an index to the *Constant Pool* item.
>> That index must be a symbolic reference to a *call-site specifier*:
>> http://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.4.10
>>
>> A CP item of the type *CONSTANT_InvokeDynamic_inf* has an index into
>> the *bootstrap method attribute* of the class file:
>> http://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.7.21
>>
>> The *|BootstrapMethods|* attribute elements normally have references to other *Constant Pool* items.
>>
>> In VM the *bootstrap method attribute* is represented by the *operands* array of the *ConstantPool*.
>>
>> The problem is is that all the force and back cross links between *ConstantPool* elements
>> and *operands* array elements must be correctly merged at class redefinition.
>>
>> Test coverage:
>>   vm.mlvm, nsk.jvmti, nsk.jdi tests on multiple platforms (32 vs 64-bit too).
>>   The testing looks good so far.
>>   One difficulty is that new vm.mlvm tests are currently failed because of multiple reasons.
>>
>>
>> Thanks,
>> Serguei