RFR (XS): 8023004: JSR 292: java.lang.RuntimeException: Original target method was called.

[Christian Thalinger]

Looks good.

On Oct 23, 2013, at 7:44 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:

> http://cr.openjdk.java.net/~vlivanov/8023004/webrev.01/
> 3 lines changed: 0 ins; 2 del; 1 mod
> 
> Reviewed-by: jrose, ?
> 
> The fix is to skip MemberNameTable update in MHN_getMemberVMInfo.
> 
> I filed a followup bug to implement MemberName interning [1]. It should eliminate possibility of MemberNameTable elements overwrite.
> 
> Testing: failing testcase.
> 
> Contributed-by: sspitsyn
> 
> JBS:  https://bugs.openjdk.java.net/browse/JDK-8023004
> 
> Best regards,
> Vladimir Ivanov
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8027162
> 
> On 10/18/13 8:47 PM, Christian Thalinger wrote:
>> 
>> On Oct 18, 2013, at 1:25 AM, serguei.spitsyn at oracle.com wrote:
>> 
>>> Vladimir,
>>> 
>>> 
>>> Just want to share some details with you.
>>> 
>>> 
>>> On 10/17/13 5:46 AM, Vladimir Ivanov wrote:
>>>> Serguei, Chris, Igor, thanks for looking at this fix.
>>>> 
>>>> I need to look more thoroughly at this code.
>>>> I assumed that member name is initialized only once, but I was wrong - asserting empty slot in MethodHandles::init_method_MemberName fails during initialization of JSR292 core classes.
>>> 
>>> John or Chris, please, correct me if I'm wrong...
>>> But, I think, John had a plan in mind to utilize the MNT for uniqueness of member names.
>>> It was about all member names, not only method member names.
>> 
>> Correct.  It is worrying that the assert fires.  We should get to the bottom of this and fix it properly.
>> 
>>> 
>>> 
>>> Thanks,
>>> Serguei
>>> 
>>>> 
>>>> Best regards,
>>>> Vladimir Ivanov
>>>> 
>>>> On 10/17/13 1:39 PM, serguei.spitsyn at oracle.com wrote:
>>>>> On 10/16/13 1:28 PM, Christian Thalinger wrote:
>>>>>> On Oct 14, 2013, at 12:15 PM, Vladimir Ivanov
>>>>>> <vladimir.x.ivanov at oracle.com> wrote:
>>>>>> 
>>>>>>> http://cr.openjdk.java.net/~vlivanov/8023004/webrev.00/
>>>>>>> 7 lines changed: 5 ins; 1 del; 1 mod
>>>>>>> 
>>>>>>> Assertion code in JDK fetches info from VM using MHN.getMemberVMInfo:
>>>>>>> assert(m.vminfoIsConsistent());
>>>>>>> 
>>>>>>> After that check, class redefinition is broken - it's still possible
>>>>>>> to invoke old version of a method.
>>>>>>> 
>>>>>>> Analysis from Serguei:
>>>>>>> "The problem is here:
>>>>>>> oop MethodHandles::init_method_MemberName(Handle mname, CallInfo&
>>>>>>> info) {
>>>>>>>  . . . . . . . .
>>>>>>> m->method_holder()->add_member_name(m->method_idnum(), mname);
>>>>>>> 
>>>>>>>  return mname();
>>>>>>> }
>>>>>>> 
>>>>>>> The call to add_member_name() resets the member name at the index
>>>>>>> m->method_idnum(). After that one of the two member names with the
>>>>>>> same method_idnum() is out of the MNT and is not updated when the
>>>>>>> method is redefined."
>>>>>> As I understand this the problem is calling
>>>>>> MethodHandles::init_method_MemberName which installs a new MemberName
>>>>>> instance in the MemberNameTable.  Correct?
>>>>>> 
>>>>>> If that's the case then there is a potential race here.
>>>>>> 
>>>>>> +     x = m->method_holder()->get_member_name(m->method_idnum());
>>>>>> +     if (x == NULL) {
>>>>>> +       Handle mname2 = MethodHandles::new_MemberName(CHECK_NULL);
>>>>>>         x = MethodHandles::init_method_MemberName(mname2, info);
>>>>>>       }
>>>>>> 
>>>>>> Racing threads can get back null when calling get_member_name and then
>>>>>> install different MemberName objects in the table.
>>>>> 
>>>>> Good catch, Chris.
>>>>> I do not see a way to fix it easily without some refactoring.
>>>>> 
>>>>> It is probably better to add to the InstanceKlass a helper function that
>>>>> will do
>>>>> this dance with a required synchronization.
>>>>> But the following line needs to be taken out of the
>>>>> init_method_MemberName() :
>>>>> m->method_holder()->add_member_name(m->method_idnum(), mname);
>>>>> 
>>>>> 
>>>>> Thanks,
>>>>> Serguei
>>>>> 
>>>>>> 
>>>>>>> The fix is to check whether there's already a member name associated
>>>>>>> with the before creating a new one.
>>>>>>> 
>>>>>>> Testing: failing test.
>>>>>>> 
>>>>>>> Contributed-by: sspitsyn, vlivanov
>>>>>>> 
>>>>>>> Best regards,
>>>>>>> Vladimir Ivanov
>>>>>>> 
>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8023004
>>>>> 
>>> 
>>