RFR 8022887: Assertion hit while using class and redefining it with RedefineClasses s,imultaneously

[Daniel D. Daugherty]

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