[8u] RFR JDK-8161144: Fix for JDK-8147451 failed: Crash in Method::checked_resolve_jmethod_id(_jmethodID*)

[David Holmes]

Hi Shafi,

No further comments from me.

Thanks,
David

On 2/08/2016 3:19 PM, Shafi Ahmad wrote:
> Hi,
>
> Please find updated webrev.
>
> http://cr.openjdk.java.net/~shshahma/8161144/webrev.02/
>
> Regards,
> Shafi
>
>> -----Original Message-----
>> From: Coleen Phillimore
>> Sent: Monday, August 01, 2016 6:01 PM
>> To: David Holmes; Shafi Ahmad; hotspot-runtime-dev at openjdk.java.net
>> Subject: Re: [8u] RFR JDK-8161144: Fix for JDK-8147451 failed: Crash in
>> Method::checked_resolve_jmethod_id(_jmethodID*)
>>
>>
>>
>> On 8/1/16 7:19 AM, David Holmes wrote:
>>> Hi Shafi,
>>>
>>> On 1/08/2016 6:47 PM, Shafi Ahmad wrote:
>>>> Hi David,
>>>>
>>>> Sorry for my half mail.
>>>>
>>>>> -----Original Message-----
>>>>> From: David Holmes
>>>>> Sent: Monday, August 01, 2016 5:32 AM
>>>>> To: Shafi Ahmad; Coleen Phillimore;
>>>>> hotspot-runtime-dev at openjdk.java.net
>>>>> Subject: Re: [8u] RFR JDK-8161144: Fix for JDK-8147451 failed: Crash
>>>>> in
>>>>> Method::checked_resolve_jmethod_id(_jmethodID*)
>>>>>
>>>>> Hi Shafi,
>>>>>
>>>>> On 30/07/2016 1:10 AM, Shafi Ahmad wrote:
>>>>>> Hi All,
>>>>>>
>>>>>> Could I have 2nd Reviewer's review for this change, please?
>>>>>
>>>>> I didn't see you respond to Coleen's query re JDK9. If this is not
>>>>> applicable to
>>>>> JDK9 please add a 9-na label to the bug report.
>>>>>
>>>>> Looking at the code:
>>>>>
>>>>> +   void clear_method(Method* m) {
>>>>> +     for (JNIMethodBlock* b = this; b != NULL; b = b->_next) {
>>>>> +       for (int i = 0; i< number_of_methods; i++) {
>>>>> +         if (b->_methods[i] == m) {
>>>>> +           b->_methods[i] = NULL;
>>>>> +         }
>>>>> +       }
>>>>> +     }
>>>>> +     // not found
>>>>> +   }
>>>>>
>>>>> Based on the "not found" comment I assume you intended to do a
>>>>> return after NULLing out the method?
>>>>>
>>>>> Nit: need space after i in i<
>>>>>
>>>>> ---
>>>>>
>>>>> 1882 void Method::clear_jmethod_id(ClassLoaderData* loader_data) {
>>>>> 1883   loader_data->jmethod_ids()->clear_method(this);
>>>>> 1884 }
>>>>>
>>>>> Not sure why you felt the need to add a new member function for this
>>>>> instead of just doing line #1883 directly at line #113
>>>>
>>>> Just to make it consistent with the existing method like void
>>>> Method::clear_jmethod_ids(ClassLoaderData* loader_data).
>>>
>>> I would not add to the API unnecessarily as it just makes the API
>>> harder to understand.
>>
>> I think this API is good rather than telling Method::deallocate_contents
>> that there's a jmethod_ids pointer in the class_loader data.   If this
>> changes for some reason, it would be good to change it together with the
>> other APIs.
>>
>> Shafi, can you move this function down to below Method::set_on_stack?
>> Then it'll be clearer that it belongs with Method::clear_jmethod_ids().
>>
>> Thanks,
>> Coleen
>>>
>>>>> ---
>>>>>
>>>>>  >>> After this change I am seeing Method::is_method_id() is getting
>>>>>>>> called with NULL and I have done below change to avoid crash >>>
>>>>>>>> - assert(m != NULL, "should be called with non-null method");
>>>>>>>> + if (m ==
>>>>> NULL) {
>>>>>  >>> +    return false;
>>>>>  >>> +  }
>>>>>
>>>>> This is the only call I can see to is_method_id:
>>>>
>>>> Yes, this is the only call.
>>>>
>>>>>
>>>>> 1870 Method* Method::checked_resolve_jmethod_id(jmethodID mid)
>> {
>>>>> 1871   if (mid == NULL) return NULL;
>>>>> 1872   if (!Method::is_method_id(mid)) {
>>>>> 1873     return NULL;
>>>>> 1874   }
>>>>>
>>>>
>>>>
>>>>
>>>>> So I don't see how it can be being passed NULL. If it is then you
>>>>> have a problem!
>>>>
>>>> Here actual parameter 'mid"  of  method is_method_id is not null but
>>>> this  we are calling another method resolve_jmethod_id(mid) which
>>>> returns NULL i.e m becomes null in below code.
>>>>
>>>> 1885 bool Method::is_method_id(jmethodID mid) {
>>>> 1886   Method* m = resolve_jmethod_id(mid);
>>>> 1887   if (m == NULL) {
>>>> 1888     return false;
>>>> 1889   }
>>>
>>> Ah I see - sorry. Thanks for clarifying.
>>>
>>> David
>>>
>>>>
>>>> Regards,
>>>> Shafi
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>
>>>>>> Regards,
>>>>>> Shafi
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Coleen Phillimore
>>>>>>> Sent: Monday, July 25, 2016 5:52 PM
>>>>>>> To: hotspot-runtime-dev at openjdk.java.net
>>>>>>> Subject: Re: [8u] RFR JDK-8161144: Fix for JDK-8147451 failed:
>>>>>>> Crash in
>>>>>>> Method::checked_resolve_jmethod_id(_jmethodID*)
>>>>>>>
>>>>>>>
>>>>>>> This looks good.  Was this a backport or is it still broken in 9?
>>>>>>> thanks,
>>>>>>> Coleen
>>>>>>>
>>>>>>> On 7/25/16 7:53 AM, Shafi Ahmad wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please review the small code change for bug: "JDK-8161144: Fix
>>>>>>>> for
>>>>>>>> JDK-
>>>>>>> 8147451 failed: Crash in
>>>>>>> Method::checked_resolve_jmethod_id(_jmethodID*)" on jdk8u-dev
>>>>>>>>
>>>>>>>> Summary:
>>>>>>>> Method::deallocate_contents() should clear 'this' from list of
>>>>>>>> Methods in
>>>>>>> JNIMethodBlock, similarly to clear_all_methods() does it, when
>>>>>>> class is unloaded.
>>>>>>>> After this change I am seeing Method::is_method_id() is getting
>>>>>>>> called with
>>>>>>> NULL and I have done below change to avoid crash
>>>>>>>>
>>>>>>>> -  assert(m != NULL, "should be called with non-null method");
>>>>>>>> +  if (m == NULL) {
>>>>>>>> +    return false;
>>>>>>>> +  }
>>>>>>>>
>>>>>>>> Webrev: http://cr.openjdk.java.net/~shshahma/8161144/webrev/
>>>>>>>> Jdk8 bug: https://bugs.openjdk.java.net/browse/JDK-8161144
>>>>>>>>
>>>>>>>> Test:  Run jprt
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Shafi
>>>>>>>
>>