[8u] RFR JDK-8161144: Fix for JDK-8147451 failed: Crash in Method::checked_resolve_jmethod_id(_jmethodID*)
Shafi Ahmad
shafi.s.ahmad at oracle.com
Tue Aug 2 17:36:01 UTC 2016
Thanks Coleen and David for the review.
Regards,
Shafi
> -----Original Message-----
> From: Coleen Phillimore
> Sent: Tuesday, August 02, 2016 8:23 PM
> To: Shafi Ahmad; David Holmes; 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*)
>
>
> I think this looks good. Thank you!
> Coleen
>
> On 8/2/16 1:19 AM, 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
>
More information about the hotspot-runtime-dev
mailing list