On Apr 11, 2013, at 4:08 PM, serguei.spit...@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
