RFR: 8042235: redefining method used by multiple MethodHandles crashes VM

[serguei.spitsyn at oracle.com]

Looks good.

Thanks,
Serguei

On 1/14/15 2:01 PM, Kevin Walls wrote:
>
> Thanks Coleen for confirming that Handle/Oop role in jvm.cpp. That 
> update I just did should contain all the updates then:
>
> http://cr.openjdk.java.net/~kevinw/8042235/webrev.01/
>
> Thanks!
> Kevin
>
>
> On 14/01/2015 21:02, Coleen Phillimore wrote:
>>
>> Kevin,
>>
>> I have reviewed the code and I don't have any additional comments to 
>> Serguei's comments.
>>
>> On 1/14/15, 3:09 PM, serguei.spitsyn at oracle.com wrote:
>>> Hi Kevin,
>>>
>>> Thank you a lot for back porting this!
>>>
>>>
>>> src/share/vm/classfile/javaClasses.cpp
>>>
>>> 2555 tty->print_cr("adjust_vmtarget: target = %x, new_method = %x", 
>>> target, new_method); // KJW
>>>
>>> Is the line above a left over from your tracing and needs to be 
>>> removed?
>>>
>>>
>>> src/share/vm/oops/cpCacheOop.cpp
>>>
>>> -  assert(m != NULL && m->is_method(), "sanity check");
>>> +  // Secondary entry can have vfinal flag and a NULL _f2, giving 
>>> m==NULL here:
>>> +  assert((m != NULL && !is_secondary_entry()) && m->is_method(), 
>>> "sanity check");
>>>
>>> The assert condition seems to become stronger.
>>> Did you really want something like this:
>>>
>>> +  assert(is_secondary_entry()  || (m != NULL && m->is_method()), 
>>> "sanity check");
>>>
>>>
>>> src/share/vm/prims/jvm.cpp
>>>
>>>  637 instanceKlass::cast(m->method_holder())->add_member_name(new_obj);
>>>
>>>
>>>   Do we need to replace new_obj with new_obj() ?
>>>
>>
>> add_member_name takes a Handle so this is ok.  new_obj was changed 
>> into a Handle with this patch.
>>
>> Thanks,
>> Coleen
>>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>> On 1/14/15 10:27 AM, Kevin Walls wrote:
>>>> Hi all,
>>>>
>>>> So an updated webrev in the review for 8042235:
>>>> http://cr.openjdk.java.net/~kevinw/8042235/webrev.01
>>>>
>>>> On the assert I was hitting: I don't think that was really part of 
>>>> this change: the constant pool cache in 7 can have these 
>>>> "secondary" entries, and they can get created with the vfinal flag 
>>>> set, but the _f2 field for a reference left null, which the 
>>>> existing assert in cpCacheOop.cpp:619 would detect. I let it assert 
>>>> now only if !is_secondary_entry().
>>>>
>>>> Thanks
>>>> Kevin
>>>>
>>>>
>>>> On 12/01/2015 22:22, KEVIN WALLS wrote:
>>>>> Hi Serguei -
>>>>>
>>>>> Thanks oops yes I seem to have mishandled that part, I'll change it.
>>>>>
>>>>> Unfortunately I think I still have the wierd constantpoolcache 
>>>>> crash I mentioned in the email just now, gotta keep looking at 
>>>>> that and then I'll update the webrev including this finalizer 
>>>>> setup call.
>>>>>
>>>>> Thanks!
>>>>> Kevin
>>>>>
>>>>>
>>>>> On 12/01/2015 19:50, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Kevin,
>>>>>>
>>>>>> src/share/vm/prims/jvm.cpp
>>>>>>
>>>>>>   645     new_obj = 
>>>>>> instanceKlass::register_finalizer(instanceOop(new_obj_oop), 
>>>>>> CHECK_NULL);
>>>>>>
>>>>>> The above looks incorrect.
>>>>>> The new_obj() must be used in stead of the new_obj_oop.
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>> On 12/17/14 7:48 AM, KEVIN WALLS wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> This is a request for review of a backport to 7u of 8042235. 
>>>>>>> There are a few changes from the original, hence the request here.
>>>>>>>
>>>>>>> On JDK7 this is not a crash, but we run the wrong method, i.e. 
>>>>>>> invocation through a MethodHandle invokes the old version of the 
>>>>>>> method, if it has been redefined.
>>>>>>>
>>>>>>> The test is different also: in jdk8 we have the ASM library, and 
>>>>>>> can visit methods and bytecodes.  Here in 7, I wrote a 
>>>>>>> non-bytecode aware byte replacer method, and replaced some 
>>>>>>> literal bytecode sequence with another.   As we're crafting a 
>>>>>>> method that we will rewrite, we can do something that avoids use 
>>>>>>> of the constant pool (which we haven't actually understood in 
>>>>>>> this trivial rewriter), so we rewrite some simple arithmetic, 
>>>>>>> and from the result of the method it's obvious whether we are 
>>>>>>> running the correct code.
>>>>>>>
>>>>>>> Coleen: thanks for your earlier hints on oop / obj_field vs. 
>>>>>>> address_field.
>>>>>>>
>>>>>>> bug
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8042235
>>>>>>>
>>>>>>> webrev
>>>>>>> http://cr.openjdk.java.net/~kevinw/8042235/webrev.00/
>>>>>>>
>>>>>>> Thanks
>>>>>>> Kevin
>>>>>>
>>>>>
>>>>
>>>
>>
>