RFR: 8161550: [JVMCI] Crash: assert(sig_bt[member_arg_pos] == T_OBJECT)

Fri Aug 19 09:47:05 UTC 2016

Hi Stefan,

On 08/18/2016 06:57 PM, Stefan Anzinger wrote:
> Hi Zoltan,
>
> thank you for your input. With a bit back and fourth I came up with
> another solution for this.

thank you for spending the time!

>
> 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/

That looks good to me!

Best regards,

Zoltan

>
> 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