[7u-dev] Request for approval for 8042235: redefining method used by multiple MethodHandles crashes VM

[Kevin Walls]

Hi,

This is a backport approval request for 8042235 into 7u. The 7u change 
is a little different from 8 onwards, some discussion below that led to 
the final webrev.

bug
https://bugs.openjdk.java.net/browse/JDK-8042235

webrev
http://cr.openjdk.java.net/~kevinw/8042235/webrev.01/

Thanks
Kevin


-------- Forwarded Message --------
Subject: 	Re: RFR: 8042235: redefining method used by multiple 
MethodHandles crashes VM
Date: 	Wed, 14 Jan 2015 14:06:35 -0800
From: 	serguei.spitsyn at oracle.com <serguei.spitsyn at oracle.com>
To: 	Kevin Walls <kevin.walls at oracle.com>, Coleen Phillimore 
<coleen.phillimore at oracle.com>, hotspot-runtime-dev at openjdk.java.net



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