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

[serguei.spitsyn at oracle.com]

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