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

[Coleen Phillimore]

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