On 9/5/13 10:35 AM, Coleen Phillimore wrote:
Dan,
Thank you for looking at this so quickly.
No problem. What motivated this was I just finished analyzing that
SIGSEGV crash in my AdHoc test run that I thought might be this bug.
I felt like I had enough context...
You are right, we are not only getting public methods, whose number
cannot change right now with redefine classes.
Still an outstanding find on the stale method array stuff and on
the "wrong" method match return...
I have to rework this change.
I'll keep an eye open for the new version.
Dan
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
