8152903: [JVMCI] CompilerToVM::resolveMethod should correctly handle private methods in interfaces

Reviewed-by: iveresov

hotspot/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotResolvedObjectTypeImpl.java

@@ -392,18 +392,13 @@ final class HotSpotResolvedObjectTypeImpl extends HotSpotResolvedJavaType implem
         return JavaKind.Object;
     }
 
-    @Override
-    public ResolvedJavaMethod resolveConcreteMethod(ResolvedJavaMethod method, ResolvedJavaType callerType) {
-        ResolvedJavaMethod resolvedMethod = resolveMethod(method, callerType);
-        if (resolvedMethod == null || resolvedMethod.isAbstract()) {
-            return null;
-        }
-        return resolvedMethod;
-    }
-
     @Override
     public ResolvedJavaMethod resolveMethod(ResolvedJavaMethod method, ResolvedJavaType callerType) {
         assert !callerType.isArray();
+        if (isInterface()) {
+            // Methods can only be resolved against concrete types
+            return null;
+        }
         if (method.isConcrete() && method.getDeclaringClass().equals(this) && method.isPublic()) {
             return method;
         }

hotspot/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotResolvedPrimitiveType.java

@@ -168,11 +168,6 @@ public final class HotSpotResolvedPrimitiveType extends HotSpotResolvedJavaType
         return false;
     }
 
-    @Override
-    public ResolvedJavaMethod resolveConcreteMethod(ResolvedJavaMethod method, ResolvedJavaType callerType) {
-        return null;
-    }
-
     @Override
     public ResolvedJavaMethod resolveMethod(ResolvedJavaMethod method, ResolvedJavaType callerType) {
         return null;

hotspot/src/jdk.vm.ci/share/classes/jdk.vm.ci.meta/src/jdk/vm/ci/meta/ResolvedJavaType.java

@@ -217,27 +217,34 @@ public interface ResolvedJavaType extends JavaType, ModifiersProvider {
 
     /**
      * Resolves the method implementation for virtual dispatches on objects of this dynamic type.
-     * This resolution process only searches "up" the class hierarchy of this type.
+     * This resolution process only searches "up" the class hierarchy of this type. A broader search
+     * that also walks "down" the hierarchy is implemented by
+     * {@link #findUniqueConcreteMethod(ResolvedJavaMethod)}. For interface types it returns null
+     * since no concrete object can be an interface.
      *
      * @param method the method to select the implementation of
      * @param callerType the caller or context type used to perform access checks
-     * @return the link-time resolved method (might be abstract) or {@code null} if it can not be
+     * @return the method that would be selected at runtime (might be abstract) or {@code null} if
-     *         linked
+     *         it can not be resolved
      */
     ResolvedJavaMethod resolveMethod(ResolvedJavaMethod method, ResolvedJavaType callerType);
 
     /**
-     * Resolves the method implementation for virtual dispatches on objects of this dynamic type.
+     * A convenience wrapper for {@link #resolveMethod(ResolvedJavaMethod, ResolvedJavaType)} that
-     * This resolution process only searches "up" the class hierarchy of this type. A broader search
+     * only returns non-abstract methods.
-     * that also walks "down" the hierarchy is implemented by
-     * {@link #findUniqueConcreteMethod(ResolvedJavaMethod)}.
      *
      * @param method the method to select the implementation of
      * @param callerType the caller or context type used to perform access checks
      * @return the concrete method that would be selected at runtime, or {@code null} if there is no
      *         concrete implementation of {@code method} in this type or any of its superclasses
      */
-    ResolvedJavaMethod resolveConcreteMethod(ResolvedJavaMethod method, ResolvedJavaType callerType);
+    default ResolvedJavaMethod resolveConcreteMethod(ResolvedJavaMethod method, ResolvedJavaType callerType) {
+        ResolvedJavaMethod resolvedMethod = resolveMethod(method, callerType);
+        if (resolvedMethod == null || resolvedMethod.isAbstract()) {
+            return null;
+        }
+        return resolvedMethod;
+    }
 
     /**
      * Given a {@link ResolvedJavaMethod} A, returns a concrete {@link ResolvedJavaMethod} B that is

hotspot/src/share/vm/interpreter/linkResolver.cpp

@@ -127,30 +127,7 @@ void CallInfo::set_common(KlassHandle resolved_klass,
   _resolved_appendix = Handle();
   DEBUG_ONLY(verify());  // verify before making side effects
 
-  if (CompilationPolicy::must_be_compiled(selected_method)) {
+  CompilationPolicy::compile_if_required(selected_method, THREAD);
-    // This path is unusual, mostly used by the '-Xcomp' stress test mode.
-
-    // Note: with several active threads, the must_be_compiled may be true
-    //       while can_be_compiled is false; remove assert
-    // assert(CompilationPolicy::can_be_compiled(selected_method), "cannot compile");
-    if (!THREAD->can_call_java()) {
-      // don't force compilation, resolve was on behalf of compiler
-      return;
-    }
-    if (selected_method->method_holder()->is_not_initialized()) {
-      // 'is_not_initialized' means not only '!is_initialized', but also that
-      // initialization has not been started yet ('!being_initialized')
-      // Do not force compilation of methods in uninitialized classes.
-      // Note that doing this would throw an assert later,
-      // in CompileBroker::compile_method.
-      // We sometimes use the link resolver to do reflective lookups
-      // even before classes are initialized.
-      return;
-    }
-    CompileBroker::compile_method(selected_method, InvocationEntryBci,
-                                  CompilationPolicy::policy()->initial_compile_level(),
-                                  methodHandle(), 0, "must_be_compiled", CHECK);
-  }
 }
 
 // utility query for unreflecting a method

hotspot/src/share/vm/jvmci/jvmciCompilerToVM.cpp

@@ -628,65 +628,35 @@ C2V_VMENTRY(jint, getVtableIndexForInterfaceMethod, (JNIEnv *, jobject, jobject
 C2V_END
 
 C2V_VMENTRY(jobject, resolveMethod, (JNIEnv *, jobject, jobject receiver_jvmci_type, jobject jvmci_method, jobject caller_jvmci_type))
-  Klass* recv_klass = CompilerToVM::asKlass(receiver_jvmci_type);
+  KlassHandle recv_klass = CompilerToVM::asKlass(receiver_jvmci_type);
-  Klass* caller_klass = CompilerToVM::asKlass(caller_jvmci_type);
+  KlassHandle caller_klass = CompilerToVM::asKlass(caller_jvmci_type);
-  Method* method = CompilerToVM::asMethod(jvmci_method);
+  methodHandle method = CompilerToVM::asMethod(jvmci_method);
 
-  if (recv_klass->is_array_klass() || (InstanceKlass::cast(recv_klass)->is_linked())) {
+  KlassHandle h_resolved   (THREAD, method->method_holder());
-    Klass* holder_klass = method->method_holder();
+  Symbol* h_name      = method->name();
-    Symbol* method_name = method->name();
+  Symbol* h_signature = method->signature();
-    Symbol* method_signature = method->signature();
 
-    if (holder_klass->is_interface()) {
+  bool check_access = true;
-      // do link-time resolution to check all access rules.
+  LinkInfo link_info(h_resolved, h_name, h_signature, caller_klass, check_access);
-      LinkInfo link_info(holder_klass, method_name, method_signature, caller_klass, true);
+  methodHandle m;
-      methodHandle resolved_method = LinkResolver::linktime_resolve_interface_method_or_null(link_info);
+  // Only do exact lookup if receiver klass has been linked.  Otherwise,
-      if (resolved_method.is_null() || resolved_method->is_private()) {
+  // the vtable has not been setup, and the LinkResolver will fail.
-        return NULL;
+  if (recv_klass->is_array_klass() ||
-      }
+      InstanceKlass::cast(recv_klass())->is_linked() && !recv_klass->is_interface()) {
-      assert(recv_klass->is_subtype_of(holder_klass), "");
+    if (h_resolved->is_interface()) {
-      // do actual lookup
+      m = LinkResolver::resolve_interface_call_or_null(recv_klass, link_info);
-      methodHandle sel_method = LinkResolver::lookup_instance_method_in_klasses(recv_klass, resolved_method->name(), resolved_method->signature(), CHECK_AND_CLEAR_0);
-      oop result = CompilerToVM::get_jvmci_method(sel_method, CHECK_NULL);
-      return JNIHandles::make_local(THREAD, result);
     } else {
-      // do link-time resolution to check all access rules.
+      m = LinkResolver::resolve_virtual_call_or_null(recv_klass, link_info);
-      LinkInfo link_info(holder_klass, method_name, method_signature, caller_klass, true);
-      methodHandle resolved_method = LinkResolver::linktime_resolve_virtual_method_or_null(link_info);
-      if (resolved_method.is_null()) {
-        return NULL;
-      }
-      // do actual lookup (see LinkResolver::runtime_resolve_virtual_method)
-      int vtable_index = Method::invalid_vtable_index;
-      Method* selected_method;
-
-      if (resolved_method->method_holder()->is_interface()) { // miranda method
-        vtable_index = LinkResolver::vtable_index_of_interface_method(holder_klass, resolved_method);
-        assert(vtable_index >= 0 , "we should have valid vtable index at this point");
-
-        selected_method = recv_klass->method_at_vtable(vtable_index);
-      } else {
-        // at this point we are sure that resolved_method is virtual and not
-        // a miranda method; therefore, it must have a valid vtable index.
-        assert(!resolved_method->has_itable_index(), "");
-        vtable_index = resolved_method->vtable_index();
-        // We could get a negative vtable_index for final methods,
-        // because as an optimization they are they are never put in the vtable,
-        // unless they override an existing method.
-        // If we do get a negative, it means the resolved method is the the selected
-        // method, and it can never be changed by an override.
-        if (vtable_index == Method::nonvirtual_vtable_index) {
-          assert(resolved_method->can_be_statically_bound(), "cannot override this method");
-          selected_method = resolved_method();
-        } else {
-          selected_method = recv_klass->method_at_vtable(vtable_index);
-        }
-      }
-      oop result = CompilerToVM::get_jvmci_method(selected_method, CHECK_NULL);
-      return JNIHandles::make_local(THREAD, result);
     }
   }
-  return NULL;
+
+  if (m.is_null()) {
+    // Return NULL only if there was a problem with lookup (uninitialized class, etc.)
+    return NULL;
+  }
+
+  oop result = CompilerToVM::get_jvmci_method(m, CHECK_NULL);
+  return JNIHandles::make_local(THREAD, result);
 C2V_END
 
 C2V_VMENTRY(jboolean, hasFinalizableSubclass,(JNIEnv *, jobject, jobject jvmci_type))

hotspot/src/share/vm/runtime/compilationPolicy.cpp

@@ -107,6 +107,33 @@ bool CompilationPolicy::must_be_compiled(methodHandle m, int comp_level) {
          (UseCompiler && AlwaysCompileLoopMethods && m->has_loops() && CompileBroker::should_compile_new_jobs()); // eagerly compile loop methods
 }
 
+void CompilationPolicy::compile_if_required(methodHandle selected_method, TRAPS) {
+  if (must_be_compiled(selected_method)) {
+    // This path is unusual, mostly used by the '-Xcomp' stress test mode.
+
+    // Note: with several active threads, the must_be_compiled may be true
+    //       while can_be_compiled is false; remove assert
+    // assert(CompilationPolicy::can_be_compiled(selected_method), "cannot compile");
+    if (!THREAD->can_call_java() || THREAD->is_Compiler_thread()) {
+      // don't force compilation, resolve was on behalf of compiler
+      return;
+    }
+    if (selected_method->method_holder()->is_not_initialized()) {
+      // 'is_not_initialized' means not only '!is_initialized', but also that
+      // initialization has not been started yet ('!being_initialized')
+      // Do not force compilation of methods in uninitialized classes.
+      // Note that doing this would throw an assert later,
+      // in CompileBroker::compile_method.
+      // We sometimes use the link resolver to do reflective lookups
+      // even before classes are initialized.
+      return;
+    }
+    CompileBroker::compile_method(selected_method, InvocationEntryBci,
+        CompilationPolicy::policy()->initial_compile_level(),
+        methodHandle(), 0, "must_be_compiled", CHECK);
+  }
+}
+
 // Returns true if m is allowed to be compiled
 bool CompilationPolicy::can_be_compiled(methodHandle m, int comp_level) {
   // allow any levels for WhiteBox

hotspot/src/share/vm/runtime/compilationPolicy.hpp

@@ -43,13 +43,19 @@ class CompilationPolicy : public CHeapObj<mtCompiler> {
   static elapsedTimer       _accumulated_time;
 
   static bool               _in_vm_startup;
+
+  // m must be compiled before executing it
+  static bool must_be_compiled(methodHandle m, int comp_level = CompLevel_all);
+
 public:
   static  void set_in_vm_startup(bool in_vm_startup) { _in_vm_startup = in_vm_startup; }
   static  void completed_vm_startup();
   static  bool delay_compilation_during_startup()    { return _in_vm_startup; }
 
-  // m must be compiled before executing it
+  // If m must_be_compiled then request a compilation from the CompileBroker.
-  static bool must_be_compiled(methodHandle m, int comp_level = CompLevel_all);
+  // This supports the -Xcomp option.
+  static void compile_if_required(methodHandle m, TRAPS);
+
   // m is allowed to be compiled
   static bool can_be_compiled(methodHandle m, int comp_level = CompLevel_all);
   // m is allowed to be osr compiled

hotspot/src/share/vm/runtime/javaCalls.cpp

@@ -343,13 +343,7 @@ void JavaCalls::call_helper(JavaValue* result, const methodHandle& method, JavaC
   }
 #endif
 
-
+  CompilationPolicy::compile_if_required(method, CHECK);
-  assert(thread->can_call_java(), "cannot compile from the native compiler");
-  if (CompilationPolicy::must_be_compiled(method)) {
-    CompileBroker::compile_method(method, InvocationEntryBci,
-                                  CompilationPolicy::policy()->initial_compile_level(),
-                                  methodHandle(), 0, "must_be_compiled", CHECK);
-  }
 
   // Since the call stub sets up like the interpreter we call the from_interpreted_entry
   // so we can go compiled via a i2c. Otherwise initial entry method will always

hotspot/test/compiler/jvmci/compilerToVM/ResolveMethodTest.java

@@ -137,8 +137,14 @@ public class ResolveMethodTest {
         HotSpotResolvedObjectType callerMetaspace = CompilerToVMHelper
                 .lookupType(Utils.toJVMTypeSignature(tcase.caller),
                         getClass(), /* resolve = */ true);
+        HotSpotResolvedObjectType receiverMetaspace = CompilerToVMHelper
+                .lookupType(Utils.toJVMTypeSignature(tcase.receiver),
+                        getClass(), /* resolve = */ true);
+
+        // Can only resolve methods on a linked class so force initialization
+        receiverMetaspace.initialize();
         HotSpotResolvedJavaMethod resolvedMetaspaceMethod
-                = CompilerToVMHelper.resolveMethod(holderMetaspace,
+                = CompilerToVMHelper.resolveMethod(receiverMetaspace,
                         metaspaceMethod, callerMetaspace);
         if (tcase.isPositive) {
             Asserts.assertNotNull(resolvedMetaspaceMethod,

hotspot/test/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/ResolvedJavaTypeResolveConcreteMethodTest.java

@@ -105,7 +105,7 @@ public class ResolvedJavaTypeResolveConcreteMethodTest {
         ResolvedJavaMethod di = getMethod(i, "d");
         ResolvedJavaMethod dc = getMethod(c, "d");
 
-        assertEquals(di, i.resolveConcreteMethod(di, c));
+        assertEquals(null, i.resolveConcreteMethod(di, c));
         assertEquals(di, b.resolveConcreteMethod(di, c));
         assertEquals(dc, c.resolveConcreteMethod(di, c));
     }

hotspot/test/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/ResolvedJavaTypeResolveMethodTest.java

@@ -105,7 +105,7 @@ public class ResolvedJavaTypeResolveMethodTest {
         ResolvedJavaMethod di = getMethod(i, "d");
         ResolvedJavaMethod dc = getMethod(c, "d");
 
-        assertEquals(di, i.resolveMethod(di, c));
+        assertEquals(null, i.resolveMethod(di, c));
         assertEquals(di, b.resolveMethod(di, c));
         assertEquals(dc, c.resolveMethod(di, c));
     }

hotspot/test/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java

@@ -583,29 +583,20 @@ public class TestResolvedJavaType extends TypeUniverse {
         return declarations;
     }
 
-    private static void checkResolveMethod(ResolvedJavaType type, ResolvedJavaType context, ResolvedJavaMethod decl, ResolvedJavaMethod expected) {
-        ResolvedJavaMethod impl = type.resolveConcreteMethod(decl, context);
-        assertEquals(expected, impl);
-    }
-
     @Test
     public void resolveMethodTest() {
         ResolvedJavaType context = metaAccess.lookupJavaType(TestResolvedJavaType.class);
         for (Class<?> c : classes) {
-            if (c.isInterface() || c.isPrimitive()) {
+            ResolvedJavaType type = metaAccess.lookupJavaType(c);
-                ResolvedJavaType type = metaAccess.lookupJavaType(c);
+            if (c.isInterface()) {
                 for (Method m : c.getDeclaredMethods()) {
-                    if (JAVA_VERSION <= 1.7D || (!isStatic(m.getModifiers()) && !isPrivate(m.getModifiers()))) {
+                    ResolvedJavaMethod resolved = metaAccess.lookupJavaMethod(m);
-                        ResolvedJavaMethod resolved = metaAccess.lookupJavaMethod(m);
+                    ResolvedJavaMethod impl = type.resolveMethod(resolved, context);
-                        ResolvedJavaMethod impl = type.resolveMethod(resolved, context);
+                    assertEquals(m.toString(), null, impl);
-                        ResolvedJavaMethod expected = resolved.isDefault() || resolved.isAbstract() ? resolved : null;
-                        assertEquals(m.toString(), expected, impl);
-                    } else {
-                        // As of JDK 8, interfaces can have static and private methods
-                    }
                 }
+            } else if (c.isPrimitive()) {
+                assertEquals("No methods expected", c.getDeclaredMethods().length, 0);
             } else {
-                ResolvedJavaType type = metaAccess.lookupJavaType(c);
                 VTable vtable = getVTable(c);
                 for (Method impl : vtable.methods.values()) {
                     Set<Method> decls = findDeclarations(impl, c);
@@ -613,7 +604,7 @@ public class TestResolvedJavaType extends TypeUniverse {
                         ResolvedJavaMethod m = metaAccess.lookupJavaMethod(decl);
                         if (m.isPublic()) {
                             ResolvedJavaMethod i = metaAccess.lookupJavaMethod(impl);
-                            checkResolveMethod(type, context, m, i);
+                            assertEquals(m.toString(), i, type.resolveMethod(m, context));
                         }
                     }
                 }
@@ -625,20 +616,16 @@ public class TestResolvedJavaType extends TypeUniverse {
     public void resolveConcreteMethodTest() {
         ResolvedJavaType context = metaAccess.lookupJavaType(TestResolvedJavaType.class);
         for (Class<?> c : classes) {
-            if (c.isInterface() || c.isPrimitive()) {
+            ResolvedJavaType type = metaAccess.lookupJavaType(c);
-                ResolvedJavaType type = metaAccess.lookupJavaType(c);
+            if (c.isInterface()) {
                 for (Method m : c.getDeclaredMethods()) {
-                    if (JAVA_VERSION <= 1.7D || (!isStatic(m.getModifiers()) && !isPrivate(m.getModifiers()))) {
+                    ResolvedJavaMethod resolved = metaAccess.lookupJavaMethod(m);
-                        ResolvedJavaMethod resolved = metaAccess.lookupJavaMethod(m);
+                    ResolvedJavaMethod impl = type.resolveConcreteMethod(resolved, context);
-                        ResolvedJavaMethod impl = type.resolveConcreteMethod(resolved, context);
+                    assertEquals(m.toString(), null, impl);
-                        ResolvedJavaMethod expected = resolved.isDefault() ? resolved : null;
-                        assertEquals(m.toString(), expected, impl);
-                    } else {
-                        // As of JDK 8, interfaces can have static and private methods
-                    }
                 }
+            } else if (c.isPrimitive()) {
+                assertEquals("No methods expected", c.getDeclaredMethods().length, 0);
             } else {
-                ResolvedJavaType type = metaAccess.lookupJavaType(c);
                 VTable vtable = getVTable(c);
                 for (Method impl : vtable.methods.values()) {
                     Set<Method> decls = findDeclarations(impl, c);
@@ -646,7 +633,7 @@ public class TestResolvedJavaType extends TypeUniverse {
                         ResolvedJavaMethod m = metaAccess.lookupJavaMethod(decl);
                         if (m.isPublic()) {
                             ResolvedJavaMethod i = metaAccess.lookupJavaMethod(impl);
-                            checkResolveMethod(type, context, m, i);
+                            assertEquals(i, type.resolveConcreteMethod(m, context));
                         }
                     }
                 }