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

[Kevin Walls]

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