Thank you Serguei - I made all of the suggested changes below.
On 09/19/2013 04:27 AM, [email protected] wrote:
Hi Coleen,
The fix is good, thank you for fixing this!
Just a few minor comments below.
I'll try to avoid commenting the same spots that Dan has already covered.
src/share/vm/oops/instanceKlass.hpp
1171 // A pointer to the current info object so we can handle the deletes.
1172 PreviousVersionNode* _current_p;
Stale comment: "info object" => "node object"
Thank you for spotting this.
Coleen
src/share/vm/oops/instanceKlass.cpp
Nice simplifications, the code became cleaner!
src/share/vm/prims/jvm.cpp
Nice refactoring!
Suggestion: rename the method:
get_class_declared_method_helper ->get_class_declared_methods_helper
src/share/vm/prims/jvmtiImpl.cpp
No comments
src/share/vm/prims/jvmtiRedefineClasses.cpp
No comments
src/share/vm/runtime/handles.hpp
No comments
src/share/vm/runtime/handles.inline.hpp
No comments
Thanks,
Serguei
On 9/18/13 12:37 PM, Coleen Phillimore wrote:
Hi, The new webrev is larger now. I found code where Method* can be
leaked because methodHandles are not freed, and have rewritten
JVM_GetClassDeclaredMethods and JVM_GetClassDeclaredConstructors to
be redefinition safe.
http://cr.openjdk.java.net/~coleenp/8022887_2/
<http://cr.openjdk.java.net/%7Ecoleenp/8022887_2/>
Tested with internal vm.quick.testlist and mlvm tests.
Thanks,
Coleen
On 9/5/2013 2:20 PM, [email protected] wrote:
Coleen,
This is great finding, and also a nice catch by Dan.
Waiting for a new webrev from you.
Thanks,
Serguei
On 9/5/13 9:35 AM, Coleen Phillimore wrote:
Dan,
Thank you for looking at this so quickly. You are right, we are
not only getting public methods, whose number cannot change right
now with redefine classes.
I have to rework this change.
Thanks,
Coleen
On 9/5/2013 12:23 PM, Daniel D. Daugherty wrote:
On 9/5/13 9:33 AM, Coleen Phillimore wrote:
Summary: Need to refetch the methods array from InstanceKlass
after safepoint.
open webrev at http://cr.openjdk.java.net/~coleenp/8022887/
The "frames" links are broken in this webrev. I had to
write down the changed line numbers for jvm.cpp and then
use the "new" link to see the context of the changes.
src/share/vm/oops/instanceKlass.cpp
Nice catch. The old code could return an 'm' value that
referred to a method that wasn't a match. Ouch.
yes, it was a bit of a red herring for a while.
src/share/vm/prims/jvm.cpp
Nice catch of the use of potentially stale method array, but I
think there might be more issues here.
In JVM_GetClassDeclaredMethods:
line 1865: ++num_methods;
<snip>
line 1871: objArrayOop r =
oopFactory::new_objArray(SystemDictionary::reflect_Method_klass(),
num_methods, CHECK_NULL);
<snip>
line 1876: methods = k->methods();
line 1877: methods_length = methods->length();
<snip>
line 1885: result->obj_at_put(out_idx, m);
<snip>
line 1890: assert(out_idx == num_methods, "just checking");
So num_methods is computed before the new_objArray() call
that
can result in a safepoint which can permit a
RedefineClasses()
operation to complete. You refresh methods and
methods_length,
but num_methods still has its pre-RedefineClasses value and
the size of the result array is also at the
pre-RedefineClasses
size. Isn't it possible that we could overflow the result
array
here? And maybe fire that assert() on line 1890.
In JVM_GetClassDeclaredConstructors(), similar concerns for these
lines:
line 1922: ++num_constructors;
<snip>
line 1928: objArrayOop r =
oopFactory::new_objArray(SystemDictionary::reflect_Constructor_klass(),
num_constructors, CHECK_NULL);
<snip>
line 1942: result->obj_at_put(out_idx, m);
<snip>
line 1947: assert(out_idx == num_constructors, "just checking");
Yes, this RedefineClasses() stuff is a serious pain in the butt
because it can change your assumed invariants in the middle of
your function.
Dan
bug link at http://bugs.sun.com/view_bug.do?bug_id=8022887
Tested with the test cases in the bug, and with internal SQE
tests (nsk.quick.testlist).
thanks,
Coleen
