Thanks, Coleen!
Serguei
On 1/30/13 3:04 PM, Coleen Phillimore wrote:
This looks really good! Thanks for doing the refactoring!
Coleen
On 1/28/2013 11:28 PM, [email protected] wrote:
Coleen,
This is a version with a refactoring you requested:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/8006542-JVMTI-JSR292.3/
Now, the NameAndType, FieldRef, InterfaceMethodRef and MethodRef use
the find_or_append_indirect_entry().
Thanks,
Serguei
On 1/28/13 2:18 PM, [email protected] wrote:
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, [email protected] 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, [email protected] 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, [email protected] 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
