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

[serguei.spitsyn at oracle.com]

On 1/14/15 1:02 PM, 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.

Ok.

Thanks,
Serguei

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