RFR 8042235: redefining method used by multiple MethodHandles crashes VM

[Daniel D. Daugherty]

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... :-)

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