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

[Christian Thalinger]

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.

src/share/vm/prims/jvmtiRedefineClasses.cpp:

+// value by seaching the index map. Returns zero (-1) if there is no mapped

"Returns zero"?

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

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