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

[Vladimir Ivanov]

Thank you, Chris.

Best regards,
Vladimir Ivanov

On 10/23/13 9:27 PM, Christian Thalinger wrote:
> 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
>>>>>>
>>>>
>>>
>