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

[serguei.spitsyn at oracle.com]

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"


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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20130919/c8ef08bd/attachment-0001.html