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

[Zoltán Majó]

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