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

[serguei.spitsyn at oracle.com]

Kevin,

Thank you for the updates!
-Serguei


On 1/14/15 1:58 PM, Kevin Walls wrote:
> Thanks Serguei --
>
> src/share/vm/classfile/javaClasses.cpp:
> Oops left in my trace, gone.
>
> src/share/vm/oops/cpCacheOop.cpp:
> Yes that wasn't quite right, yours is much better.
>
>
> src/share/vm/prims/jvm.cpp:
> We accept a Handle in instanceKlass::add_member_name(Handle mem_name), 
> and add_member_name can safepoint so  thought we want to pass the 
> Handle not the Oop.  However I just looked at 8u again and it uses 
> new_obj() in this call.  I'll check on this...  I'm updating the 
> webrev in the same location while looking at this last point again.
>
> Thanks!
> Kevin
>
>
>
> On 14/01/2015 20:09, 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() ?
>>
>>
>>
>> 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
>>>>>
>>>>
>>>
>>
>