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

[Daniel D. Daugherty]

On 9/18/13 1: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/>

Again, your 'frames' links are broken, but those "Previous File"
and "Next File" links look interesting... :-)

Short version: thumbs up

Longer version...

src/share/vm/runtime/handles.hpp
     No comments.

src/share/vm/runtime/handles.inline.hpp
     No comments.


src/share/vm/oops/instanceKlass.hpp
     So this comment:

         ConstantPool*    _prev_constant_pool;  // regular or weak reference

     is stale, i.e., leftover from the pre-PermGenRemoval days.
     And it looks like you're ripping out the PreviousVersionInfo
     wrapper around PreviousVersionNode stuff because the constant
     pool is no longer an oop so we don't need that extra layer of
     protection anymore. I definitely like the cleanup.

src/share/vm/oops/instanceKlass.cpp
     More CDE of stuff from the pre-PermGenRemoval days. Looks good.

And buried in all the CDE, is a critical fix from the previous
     round:

*** 3518,3527 ****
--- 3511,3522 ----
         m = methods()->at(index);
         if (m->method_idnum() == idnum) {
           return m;
         }
       }
+     // None found, return null for the caller to handle.
+     return NULL;
     }
     return m;
   }

src/share/vm/prims/jvm.cpp
     I like the refactoring into get_class_declared_method_helper().

     Can you change:

1918   return get_class_declared_method_helper(env, ofClass, publicOnly, 
false,
1919 SystemDictionary::reflect_Method_klass(), THREAD);

     to:

1918   return get_class_declared_method_helper(env, ofClass, publicOnly,
1919                                           false /* want_constructor */,
1920 SystemDictionary::reflect_Method_klass(), THREAD);

     And can you change:

1926   return get_class_declared_method_helper(env, ofClass, publicOnly, 
true,
1927 SystemDictionary::reflect_Constructor_klass(), THREAD);

     to:

1927   return get_class_declared_method_helper(env, ofClass, publicOnly,
1928                                           true /* want_constructor */,
1929 SystemDictionary::reflect_Constructor_klass(), THREAD);


src/share/vm/prims/jvmtiImpl.cpp
     More CDE from the removal of PreviousVersionInfo. Looks good.


src/share/vm/prims/jvmtiRedefineClasses.cpp
     More CDE from the removal of PreviousVersionInfo. Looks good.

So if I understand this change correctly:

1) fix Method* InstanceKlass::method_with_idnum(int idnum) {
    to not return a mis-matched Method* value
2) refactor JVM_GetClassDeclaredMethods() and
    JVM_GetClassDeclaredConstructors() to share common code and to
    tolerate an interleaved JVM/TI RedefineClasses() call
3) CDE of PreviousVersionInfo and the cleanup of PreviousVersionWalker
    that the CDE allows

Nicely done.

Dan



>
> Tested with internal vm.quick.testlist and mlvm tests.
>
> Thanks,
> Coleen
>
>
> On 9/5/2013 2:20 PM, serguei.spitsyn at oracle.com 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
>>>>
>>>
>>
>