RFR 8042235: redefining method used by multiple MethodHandles crashes VM

[Coleen Phillimore]

Thanks Dan!

On 11/19/14, 12:27 PM, Daniel D. Daugherty wrote:
> On 11/19/14 9:59 AM, Coleen Phillimore wrote:
>>
>> Thank you Dan for reviewing this.   New webrev, see below for why.
>>
>> http://cr.openjdk.java.net/~coleenp/8042235_2/
>
> src/share/vm/classfile/javaClasses.hpp
>     No comments.
>
> src/share/vm/classfile/javaClasses.cpp
>     No comments.
>
> src/share/vm/oops/instanceKlass.hpp
>     No comments.
>
> src/share/vm/oops/instanceKlass.cpp
>     No comments.
>
> src/share/vm/prims/jvm.cpp
>     No comments. That JVM_Clone() code is scary... :-)

There are scarier things :)
Coleen
>
> src/share/vm/prims/methodHandles.hpp
>     No comments.
>
> src/share/vm/prims/methodHandles.cpp
>     No comments.
>
> test/compiler/jsr292/RedefineMethodUsedByMultipleMethodHandles.java
>     No comments.
>
>
> Thumbs up.
>
> Dan
>
>
>
>
>>
>>
>> On 11/18/14, 1:24 PM, Daniel D. Daugherty wrote:
>>> On 11/17/14 3:49 PM, Coleen Phillimore wrote:
>>>> Summary: note all MemberNames created on internal list for 
>>>> adjusting method entries.
>>>>
>>>> The JVM MemberNameTable code will push all member names on the list 
>>>> rather than trying to index by method_idnum.  The code to look up 
>>>> MemberName types wasn't used so was removed.  Class redefinition 
>>>> iterates through the table sequentially to update the Method* 
>>>> pointers in saved member names.
>>>>
>>>> This change will work with David Chase's change to the Java code 
>>>> for bug 8013267 without the extra code dealing with class 
>>>> redefinition.
>>>>
>>>> Tested with vm.quick.testlist, jck tests and jtreg tests, including 
>>>> the mlvm tests that failed in the bug report.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8042235/
>>>
>>> src/share/vm/classfile/javaClasses.hpp
>>>     No comments.
>>>
>>> src/share/vm/classfile/javaClasses.cpp
>>>     No comments.
>>>
>>> src/share/vm/oops/instanceKlass.hpp
>>>     No comments.
>>>
>>> src/share/vm/oops/instanceKlass.cpp
>>>     line 2951: _member_names = new (ResourceObj::C_HEAP, mtClass) 
>>> MemberNameTable(idnum_allocated_count());
>>>         Not your bug, but what should happen if this new fails? Or
>>>         is this one of the operator overrides that handles that by
>>>         killing the VM?
>>
>> This is one of the ResourceObj::new calls that does a 
>> vm_exit_out_of_memory.  We could add some code to handle allocation 
>> failure at this part (maybe allocate a very small member name array 
>> instead) but I don't think it's worth it at this point.
>>
>>>
>>> src/share/vm/prims/jvm.cpp
>>>     nit line 612: methodHandle m (THREAD, method);
>>>         Please delete space between 'm ('.
>>
>> Okay.
>>>
>>>     lines 607-609, 616: uses of 'new_obj'
>>>         Should all of these be switched to 'new_obj_h()'?
>>>
>>>         In particular, is line 616 subject to being moved by GC
>>>         if the methodHandle creation goes to a safepoint?
>>
>> So methodHandle doesn't go to a safepoint, and the uses of new_obj 
>> (unhandled) are safe in that call until the add_member_name() call. 
>> But you have to know that the other calls that use new_obj don't go 
>> to a safepoint.  It's always safest to Handle early and pass around 
>> handles.   This function is odd in that it uses the oop to copy into 
>> first though.
>>
>> This comment made me examine this code and the call to 
>> register_finalizer can safepoint.  In this case the MemberName 
>> Method* could be redefined and not copied.  This can't happen because 
>> MemberName is a final class without finalizers, but I've rearranged 
>> the code to be safe for this case if it ever happens.
>>
>> The webrev above is this rearranged code.  I've rerun the 
>> java/lang/invoke tests on it.
>>>
>>>     line 620: return JNIHandles::make_local(env, oop(new_obj_h()));
>>>         is the 'oop(...)' around 'new_obj_h()' redundant? I might
>>>         be rusty, but isn't 'new_obj_h()' the unhandled oop?
>>>
>>
>> Yes, this was unnecessary.
>>> src/share/vm/prims/methodHandles.hpp
>>>     No comments.
>>>
>>> src/share/vm/prims/methodHandles.cpp
>>>     No comments.
>>>
>>> test/compiler/jsr292/RedefineMethodUsedByMultipleMethodHandles.java
>>>     line 142 //    static class FooTransformer implements 
>>> ClassFileTransformer, Opcodes {
>>>         Do you still need this line?
>>
>> No, I removed it.
>>
>> Thanks!
>> Coleen
>>>
>>>
>>> Dan
>>>
>>>
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8042235
>>>>
>>>> Thanks,
>>>> Coleen
>>>
>>
>