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

[Coleen Phillimore]

Thank you Serguei - I made all of the suggested changes below.

On 09/19/2013 04:27 AM, serguei.spitsyn at oracle.com 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, 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/3e906bee/attachment-0001.html