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

[Seán Coffey]

Approved.

regards,
Sean.

On 14/01/2015 22:32, Kevin Walls wrote:
>
> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
>
>