RFR: 8161550: [JVMCI] Crash: assert(sig_bt[member_arg_pos] == T_OBJECT)
Doug Simon
doug.simon at oracle.com
Thu Aug 18 17:41:35 UTC 2016
Hi Stefan
I forgot to mention earlier that the javadoc for ResovedJavaType.resolveMethod should also be updated.
Otherwise, looks good.
-Doug
> On 18 Aug 2016, at 18:57, Stefan Anzinger <stefan.anzinger at oracle.com> wrote:
>
> Hi Zoltan,
>
> thank you for your input. With a bit back and fourth I came up with
> another solution for this.
>
> We've decided to always return null when someone tries to resolve a
> method with @PolymorphicSignature annotation.
>
> Please review the latest webrev [1]. Consider, this webrev depends on
> Dougs change for 8164214.
>
> [1] http://cr.openjdk.java.net/~sanzinger/8161550/webrev.04/
>
> Thank you
> Stefan
>
> On 08/10/2016 12:27 PM, Zoltán Majó wrote:
>> Hi Stefan,
>>
>>
>> thank you for the updated webrev!
>>
>> On 08/09/2016 01:26 PM, Stefan Anzinger wrote:
>>> Hi Zoltan,
>>>
>>> Tom tried to integrate this patch, but the assertion error was still in.
>>> I've updated the webrev, to fix the assertion error too.
>>>
>>> Please see updated webrev [1].
>>>
>>> The issue there was, that the assertion triggered, when JVMCI was trying
>>> to resolve MethodHandle.linkTo* methods. To avoid this kind of crash, I
>>> updated method LinkResolver::lookup_method_in_klasses I removed the bool
>>> argument checkpolymorphism.
>>> * There are just two call sites, one call it with true, the other
>>> with false
>>> * The check performed when checkpolymorphism is true is done at the
>>> very end of the method.
>>> thus, I moved check code to the call site where the check should be
>>> performed.
>>
>> I'm fine with moving the check from
>>
>> LinkResolver::lookup_method_in_klasses()
>>
>> to
>>
>> LinkResolver::resolve_method()
>>
>> (where it is actually needed). But it seems that the change in
>> resolve_method()should be done a bit differently so that the method's
>> original logic is preserved.
>>
>> Before you changes, calling lookup_method_in_klasses() in
>> resolve_method() returned null for signature polymorphic methods
>>
>> 707 // 3. lookup method in resolved klass and its super klasses
>> here --> 708 methodHandle resolved_method =
>> lookup_method_in_klasses(link_info, true, false, CHECK_NULL);
>>
>> resolve_method() then called lookup_polymorphic_method() for signature
>> polymorphic methods
>>
>> 709
>> 710 // 4. lookup method in all the interfaces implemented by the
>> resolved klass
>> 711 if (resolved_method.is_null() &&
>> !resolved_klass->is_array_klass()) { // not found in the class hierarchy
>> 712 resolved_method = lookup_method_in_interfaces(link_info,
>> CHECK_NULL);
>> 713
>> 714 if (resolved_method.is_null()) {
>> 715 // JSR 292: see if this is an implicitly generated method
>> MethodHandle.linkToVirtual(*...), etc
>> here --> 716 resolved_method =
>> lookup_polymorphic_method(link_info, (Handle*)NULL, (Handle*)NULL, THREAD);
>> 717 if (HAS_PENDING_EXCEPTION) {
>> 718 nested_exception = Handle(THREAD, PENDING_EXCEPTION);
>> 719 CLEAR_PENDING_EXCEPTION;
>> 720 }
>> 721 }
>> 722 }
>>
>> (because resolved_method was NULL at line 711 andthe MethodHandleclass
>> is a proper class (i.e., not an array class).
>>
>> Itseems to me that with your proposed changeslookup_method_in_klasses()
>> returns non-null for signature polymorphic methods
>>
>> 698 // 3. lookup method in resolved klass and its super klasses
>> here --> 699 methodHandle resolved_method =
>> lookup_method_in_klasses(link_info, false, CHECK_NULL);
>>
>> and then lookup_polymorphic_method() is not called (because resolved
>> method is not null).
>>
>> 701 // 4. lookup method in all the interfaces implemented by
>> the resolved klass
>> here --> 702 if (resolved_method.is_null()) { // not found in the
>> class hierarchy
>> 703 if (!resolved_klass->is_array_klass()) {
>> 704 resolved_method =
>> lookup_method_in_interfaces(link_info, CHECK_NULL);
>> 705 if (resolved_method.is_null()) {
>> 706 // JSR 292: see if this is an implicitly generated
>> method MethodHandle.linkToVirtual(*...), etc
>> 707 resolved_method =
>> lookup_polymorphic_method(link_info, (Handle*)NULL, (Handle*)NULL, THREAD);
>> 708 if (HAS_PENDING_EXCEPTION) {
>> 709 nested_exception = Handle(THREAD, PENDING_EXCEPTION);
>> 710 CLEAR_PENDING_EXCEPTION;
>> 711 }
>> 712 }
>>
>> A solution would be to add
>>
>> if (!resolved_method.is_null() &&
>> MethodHandles::is_signature_polymorphic(resolved_method->intrinsic_id())) {
>> // Do not link directly to these. The VM must produce a synthetic
>> one using lookup_polymorphic_method.
>> resolved_method = NULL;
>> }
>>
>> before
>>
>> 710 // 4. lookup method in all the interfaces implemented by the
>> resolved klass
>> 711 if (resolved_method.is_null() &&
>> !resolved_klass->is_array_klass()) { // not found in the class hierarchy
>>
>> and remove
>>
>> 716 } else {
>> 717 vmIntrinsics::ID iid = resolved_method->intrinsic_id();
>> 718 if (MethodHandles::is_signature_polymorphic(iid)) {
>> 719 // Do not link directly to these. The VM must produce
>> a synthetic one using lookup_polymorphic_method.
>> 720 resolved_method = NULL;
>> 721 }
>>
>> Am I seeing this wrong?
>>
>> Also, please use correct indentation in linkResolver.cpp. Please use
>> spaces instead of tabs.
>>
>> A small issue: Your comments in TestResolvedJavaType.java are missinga verb
>>
>> 582 // Polymorphic signatures do not be
>> looked up.
>> 617 // Polymorphic signatures do not be
>> looked up.
>>
>> Maybe say something like "Polymorphic signatures are not looked up" (or
>> should not be looked up).
>>
>> Could you please run the jdk java/lang/invoke tests for the next
>> revision of the webrev (in addition to JPRT)?
>>
>> Thank you!
>>
>> Best regards,
>>
>>
>> Zoltan
>>
>>
>>>
>>> [1] http://cr.openjdk.java.net/~sanzinger/8161550/webrev.01/
>>>
>>> Stefan
>>>
>>>
>>> On 08/03/2016 04:43 AM, Zoltán Majó wrote:
>>>> Hi Stefan,
>>>>
>>>>
>>>> thank you for fixing this issue! Your fix looks good to me.
>>>>
>>>> Do you need a sponsor? If yes, I can sponsor your change.
>>>>
>>>> Best regards,
>>>>
>>>>
>>>> Zoltan
>>>>
>>>>
>>>> On 08/02/2016 07:07 AM, Stefan Anzinger wrote:
>>>>> Hello,
>>>>>
>>>>> this RFR fixes 8161550. JVMCI must not return intrinsified MethodHandle
>>>>> methods. The test had to be updated and the shortcut evaluation in
>>>>> HotSpotResolvedObjectTypeImpl.resolveMethod needs take care of this
>>>>> fact.
>>>>>
>>>>> http://cr.openjdk.java.net/~sanzinger/8161550/webrev.00/
>>>>>
>>>>> Stefan
>>
More information about the hotspot-compiler-dev
mailing list