RFR 8042235: redefining method used by multiple MethodHandles crashes VM

[Coleen Phillimore]

Thank you Dan for reviewing this.   New webrev, see below for why.

http://cr.openjdk.java.net/~coleenp/8042235_2/


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
>