Simplify reflective constructor invocations by using FactoryMethod

Author: Christian Wimmer

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/reflect/ReflectionAccessorHolder.java

@@ -30,7 +30,6 @@
 
 import com.oracle.svm.core.annotate.NeverInline;
 import com.oracle.svm.core.jdk.InternalVMMethod;
-import com.oracle.svm.core.reflect.SubstrateConstructorAccessor.ConstructorNewInstanceFunctionPointer;
 import com.oracle.svm.core.reflect.SubstrateMethodAccessor.MethodInvokeFunctionPointer;
 import com.oracle.svm.core.util.VMError;
 
@@ -47,21 +46,13 @@ public final class ReflectionAccessorHolder {
      * Signature prototype for invoking a method via a {@link SubstrateMethodAccessor}. Must match
      * the signature of {@link MethodInvokeFunctionPointer#invoke}
      */
-    static Object invokePrototype(boolean invokeSpecial, Object obj, Object[] args) {
-        throw VMError.shouldNotReachHere("Only used as a prototype for generated methods");
-    }
-
-    /**
-     * Signature prototype for allocating a new instance via a {@link SubstrateConstructorAccessor}.
-     * Must match the signature of {@link ConstructorNewInstanceFunctionPointer#invoke}
-     */
-    static Object newInstancePrototype(Object[] args) {
+    private static Object invokePrototype(boolean invokeSpecial, Object obj, Object[] args) {
         throw VMError.shouldNotReachHere("Only used as a prototype for generated methods");
     }
 
     /*
      * Methods for throwing exceptions when a method or constructor is used in an illegal way. These
-     * methods are invoked via function pointers, so must have the same signature as the prototypes
+     * methods are invoked via function pointers, so must have the same signature as the prototype
      * above.
      */
 
@@ -70,7 +61,7 @@ private static void methodHandleInvokeError(boolean invokeSpecial, Object obj, O
         throw new InvocationTargetException(new UnsupportedOperationException("MethodHandle.invoke() and MethodHandle.invokeExact() cannot be invoked through reflection"));
     }
 
-    private static Object newInstanceError(Object[] args) throws InstantiationException {
+    private static Object newInstanceError(boolean invokeSpecial, Object obj, Object[] args) throws InstantiationException {
         throw new InstantiationException("Only non-abstract instance classes can be instantiated using reflection");
     }
 
@@ -80,30 +71,25 @@ private static Object newInstanceError(Object[] args) throws InstantiationExcept
      */
 
     @NeverInline("Exception slow path")
-    private static InvocationTargetException throwInvocationTargetException(Throwable target) throws InvocationTargetException {
-        throw new InvocationTargetException(target);
-    }
-
-    @NeverInline("Exception slow path")
-    private static void throwIllegalArgumentExceptionForMethod(Object member, Object obj, Object[] args) {
-        throwIllegalArgumentException(member, false, obj, args);
+    private static void throwIllegalArgumentExceptionWithReceiver(Object member, Object obj, Object[] args) {
+        throwIllegalArgumentException(member, true, obj, args);
     }
 
     @NeverInline("Exception slow path")
-    private static void throwIllegalArgumentExceptionForConstructor(Object member, Object[] args) {
-        throwIllegalArgumentException(member, true, null, args);
+    private static void throwIllegalArgumentExceptionWithoutReceiver(Object member, Object[] args) {
+        throwIllegalArgumentException(member, false, null, args);
     }
 
     /**
      * We do not know which check in the generated method caused the exception, so we cannot print
      * detailed information about that. But printing the signature of the method and all the types
      * of the actual arguments should make it obvious what the problem is.
      */
-    private static void throwIllegalArgumentException(Object member, boolean constructor, Object obj, Object[] args) {
+    private static void throwIllegalArgumentException(Object member, boolean withReceiver, Object obj, Object[] args) {
         String sep = System.lineSeparator();
         StringBuilder msg = new StringBuilder();
         msg.append("Illegal arguments for invoking ").append(member);
-        if (!constructor) {
+        if (withReceiver) {
             msg.append(sep).append("  obj: ").append(obj == null ? "null" : obj.getClass().getTypeName());
         }
         if (args == null) {

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/reflect/SubstrateConstructorAccessor.java

@@ -30,19 +30,13 @@
 
 import org.graalvm.nativeimage.c.function.CFunctionPointer;
 
-import com.oracle.svm.core.annotate.InvokeJavaFunctionPointer;
 import com.oracle.svm.core.jdk.InternalVMMethod;
+import com.oracle.svm.core.reflect.SubstrateMethodAccessor.MethodInvokeFunctionPointer;
 import com.oracle.svm.core.util.VMError;
 
 @InternalVMMethod
 public abstract class SubstrateConstructorAccessor {
 
-    interface ConstructorNewInstanceFunctionPointer extends CFunctionPointer {
-        /** Must match the signature of {@link ReflectionAccessorHolder#newInstancePrototype}. */
-        @InvokeJavaFunctionPointer
-        Object invoke(Object[] args);
-    }
-
     private final Executable member;
     private final CFunctionPointer newInstanceFunctionPointer;
 
@@ -52,11 +46,11 @@ protected SubstrateConstructorAccessor(Executable member, CFunctionPointer newIn
     }
 
     public Object newInstance(Object[] args) {
-        ConstructorNewInstanceFunctionPointer functionPointer = (ConstructorNewInstanceFunctionPointer) this.newInstanceFunctionPointer;
+        MethodInvokeFunctionPointer functionPointer = (MethodInvokeFunctionPointer) this.newInstanceFunctionPointer;
         if (functionPointer.isNull()) {
             throw newInstanceError();
         }
-        return functionPointer.invoke(args);
+        return functionPointer.invoke(false, null, args);
     }
 
     private RuntimeException newInstanceError() {

substratevm/src/com.oracle.svm.reflect/src/com/oracle/svm/reflect/hosted/ReflectionFeature.java

@@ -27,7 +27,6 @@
 // Checkstyle: allow reflection
 
 import java.lang.invoke.MethodHandle;
-import java.lang.reflect.Constructor;
 import java.lang.reflect.Executable;
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
@@ -85,9 +84,8 @@ public class ReflectionFeature implements GraalFeature {
     final Map<Executable, Object> accessors = new ConcurrentHashMap<>();
 
     private static final Method invokePrototype = ReflectionUtil.lookupMethod(ReflectionAccessorHolder.class, "invokePrototype", boolean.class, Object.class, Object[].class);
-    private static final Method newInstancePrototype = ReflectionUtil.lookupMethod(ReflectionAccessorHolder.class, "newInstancePrototype", Object[].class);
     private static final Method methodHandleInvokeErrorMethod = ReflectionUtil.lookupMethod(ReflectionAccessorHolder.class, "methodHandleInvokeError", boolean.class, Object.class, Object[].class);
-    private static final Method newInstanceErrorMethod = ReflectionUtil.lookupMethod(ReflectionAccessorHolder.class, "newInstanceError", Object[].class);
+    private static final Method newInstanceErrorMethod = ReflectionUtil.lookupMethod(ReflectionAccessorHolder.class, "newInstanceError", boolean.class, Object.class, Object[].class);
 
     FeatureImpl.BeforeAnalysisAccessImpl analysisAccess;
 
@@ -109,24 +107,24 @@ Object getOrCreateAccessor(Executable member) {
      * instances use function pointer calls to invocation stubs. The invocation stubs unpack the
      * Object[] array arguments and invoke the actual method.
      * 
-     * The stubs are methods with manually created Graal IR: {@link ReflectiveInvokeMethod} and
-     * {@link ReflectiveNewInstanceMethod}. Since they are only invoked via function pointers and
-     * never at a normal call site, they need to be registered for compilation manually. From the
-     * point of view of the static analysis, they are root methods.
+     * The stubs are methods with manually created Graal IR: {@link ReflectiveInvokeMethod}. Since
+     * they are only invoked via function pointers and never at a normal call site, they need to be
+     * registered for compilation manually. From the point of view of the static analysis, they are
+     * root methods.
      * 
      * {@link ConcurrentHashMap#computeIfAbsent} guarantees that this method is called only once per
      * member, so no further synchronization is necessary.
      */
     private Object createAccessor(Executable member) {
         String name = SubstrateUtil.uniqueShortName(member);
+        ResolvedJavaMethod prototype = analysisAccess.getMetaAccess().lookupJavaMethod(invokePrototype).getWrapped();
         if (member instanceof Method) {
             ResolvedJavaMethod invokeMethod;
             if (member.getDeclaringClass() == MethodHandle.class && (member.getName().equals("invoke") || member.getName().equals("invokeExact"))) {
                 /* Method handles must not be invoked via reflection. */
                 invokeMethod = analysisAccess.getMetaAccess().lookupJavaMethod(methodHandleInvokeErrorMethod);
             } else {
-                ResolvedJavaMethod prototype = analysisAccess.getMetaAccess().lookupJavaMethod(invokePrototype).getWrapped();
-                invokeMethod = createReflectiveInvokeMethod(name, prototype, (Method) member);
+                invokeMethod = new ReflectiveInvokeMethod(name, prototype, member);
             }
             return ImageSingletons.lookup(SubstrateReflectionAccessorFactory.class).createMethodAccessor(member, register(invokeMethod));
 
@@ -142,8 +140,7 @@ private Object createAccessor(Executable member) {
                  */
                 newInstanceMethod = analysisAccess.getMetaAccess().lookupJavaMethod(newInstanceErrorMethod);
             } else {
-                ResolvedJavaMethod prototype = analysisAccess.getMetaAccess().lookupJavaMethod(newInstancePrototype).getWrapped();
-                newInstanceMethod = createReflectiveNewInstanceMethod(name, prototype, (Constructor<?>) member);
+                newInstanceMethod = new ReflectiveInvokeMethod(name, prototype, member);
             }
             return ImageSingletons.lookup(SubstrateReflectionAccessorFactory.class).createConstructorAccessor(member, register(newInstanceMethod));
         }
@@ -155,14 +152,6 @@ private CFunctionPointer register(ResolvedJavaMethod method) {
         return new MethodPointer(aMethod);
     }
 
-    protected ResolvedJavaMethod createReflectiveInvokeMethod(String name, ResolvedJavaMethod prototype, Method method) {
-        return new ReflectiveInvokeMethod(name, prototype, method);
-    }
-
-    protected ResolvedJavaMethod createReflectiveNewInstanceMethod(String name, ResolvedJavaMethod prototype, Constructor<?> constructor) {
-        return new ReflectiveNewInstanceMethod(name, prototype, constructor);
-    }
-
     protected void inspectAccessibleField(@SuppressWarnings("unused") Field field) {
     }
 

substratevm/src/com.oracle.svm.reflect/src/com/oracle/svm/reflect/hosted/ReflectionGraphKit.java

@@ -26,7 +26,9 @@
 
 // Checkstyle: allow reflection
 
+import java.lang.reflect.Constructor;
 import java.lang.reflect.Executable;
+import java.lang.reflect.InvocationTargetException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
@@ -60,12 +62,15 @@
 import org.graalvm.compiler.nodes.java.InstanceOfNode;
 import org.graalvm.compiler.nodes.type.StampTool;
 
+import com.oracle.graal.pointsto.infrastructure.UniverseMetaAccess;
 import com.oracle.graal.pointsto.meta.HostedProviders;
 import com.oracle.svm.core.graal.nodes.LoweredDeadEndNode;
 import com.oracle.svm.core.meta.SubstrateObjectConstant;
 import com.oracle.svm.core.reflect.ReflectionAccessorHolder;
 import com.oracle.svm.core.util.VMError;
+import com.oracle.svm.hosted.code.FactoryMethodSupport;
 import com.oracle.svm.hosted.phases.HostedGraphKit;
+import com.oracle.svm.util.ReflectionUtil;
 
 import jdk.vm.ci.meta.JavaKind;
 import jdk.vm.ci.meta.ResolvedJavaMethod;
@@ -137,20 +142,23 @@ public void emitIllegalArgumentException(Executable member, ValueNode obj, Value
         ResolvedJavaMethod targetMethod;
         ValueNode[] arguments;
         if (obj == null) {
-            targetMethod = findMethod(ReflectionAccessorHolder.class, "throwIllegalArgumentExceptionForConstructor", true);
+            targetMethod = findMethod(ReflectionAccessorHolder.class, "throwIllegalArgumentExceptionWithoutReceiver", true);
             arguments = new ValueNode[]{memberNode, args};
         } else {
-            targetMethod = findMethod(ReflectionAccessorHolder.class, "throwIllegalArgumentExceptionForMethod", true);
+            targetMethod = findMethod(ReflectionAccessorHolder.class, "throwIllegalArgumentExceptionWithReceiver", true);
             arguments = new ValueNode[]{memberNode, obj, args};
         }
         createJavaCallWithExceptionAndUnwind(CallTargetNode.InvokeKind.Static, targetMethod, arguments);
         append(new LoweredDeadEndNode());
     }
 
+    private static final Constructor<InvocationTargetException> invocationTargetExceptionConstructor = ReflectionUtil.lookupConstructor(InvocationTargetException.class, Throwable.class);
+
     public void emitInvocationTargetException() {
         AbstractMergeNode merge = continueWithMerge(invocationTargetExceptionPaths);
         ValueNode exception = createPhi(invocationTargetExceptionPaths, merge);
-        ResolvedJavaMethod throwInvocationTargetException = findMethod(ReflectionAccessorHolder.class, "throwInvocationTargetException", true);
+        ResolvedJavaMethod throwInvocationTargetException = FactoryMethodSupport.singleton().lookup((UniverseMetaAccess) getMetaAccess(),
+                        getMetaAccess().lookupJavaMethod(invocationTargetExceptionConstructor), true);
         createJavaCallWithExceptionAndUnwind(CallTargetNode.InvokeKind.Static, throwInvocationTargetException, exception);
         append(new LoweredDeadEndNode());
     }

substratevm/src/com.oracle.svm.reflect/src/com/oracle/svm/reflect/hosted/ReflectiveInvokeMethod.java

@@ -29,7 +29,7 @@
 import static org.graalvm.compiler.nodeinfo.NodeCycles.CYCLES_0;
 import static org.graalvm.compiler.nodeinfo.NodeSize.SIZE_0;
 
-import java.lang.reflect.Method;
+import java.lang.reflect.Executable;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -55,10 +55,12 @@
 import org.graalvm.compiler.nodes.spi.CanonicalizerTool;
 import org.graalvm.compiler.phases.common.inlining.InliningUtil;
 
+import com.oracle.graal.pointsto.infrastructure.UniverseMetaAccess;
 import com.oracle.graal.pointsto.meta.HostedProviders;
 import com.oracle.graal.pointsto.phases.SubstrateIntrinsicGraphBuilder;
 import com.oracle.svm.core.meta.SharedMethod;
 import com.oracle.svm.core.reflect.SubstrateMethodAccessor;
+import com.oracle.svm.hosted.code.FactoryMethodSupport;
 import com.oracle.svm.hosted.code.NonBytecodeStaticMethod;
 
 import jdk.vm.ci.meta.JavaKind;
@@ -67,9 +69,9 @@
 
 public class ReflectiveInvokeMethod extends NonBytecodeStaticMethod {
 
-    private final Method method;
+    private final Executable method;
 
-    public ReflectiveInvokeMethod(String name, ResolvedJavaMethod prototype, Method method) {
+    public ReflectiveInvokeMethod(String name, ResolvedJavaMethod prototype, Executable method) {
         super(name, prototype.getDeclaringClass(), prototype.getSignature(), prototype.getConstantPool());
         this.method = method;
     }
@@ -91,12 +93,21 @@ public StructuredGraph buildGraph(DebugContext ctx, ResolvedJavaMethod m, Hosted
         graphKit.getFrameState().clearLocals();
 
         ResolvedJavaMethod targetMethod = providers.getMetaAccess().lookupJavaMethod(method);
+        if (targetMethod.isStatic() || targetMethod.isConstructor()) {
+            graphKit.emitEnsureInitializedCall(targetMethod.getDeclaringClass());
+        }
+        if (targetMethod.isConstructor()) {
+            /*
+             * For a constructor, we invoke a synthetic static factory method that combines both the
+             * allocation and the constructor invocation.
+             */
+            targetMethod = FactoryMethodSupport.singleton().lookup((UniverseMetaAccess) providers.getMetaAccess(), targetMethod, false);
+        }
+
         Class<?>[] argTypes = method.getParameterTypes();
         int receiverOffset = targetMethod.isStatic() ? 0 : 1;
         ValueNode[] args = new ValueNode[argTypes.length + receiverOffset];
-        if (targetMethod.isStatic()) {
-            graphKit.emitEnsureInitializedCall(targetMethod.getDeclaringClass());
-        } else {
+        if (!targetMethod.isStatic()) {
             /*
              * The specification explicitly demands a NullPointerException and not a
              * IllegalArgumentException when the receiver of a non-static method is null
@@ -112,11 +123,12 @@ public StructuredGraph buildGraph(DebugContext ctx, ResolvedJavaMethod m, Hosted
         graphKit.fillArgsArray(argumentArray, receiverOffset, args, argTypes);
 
         InvokeKind invokeKind;
+        assert !targetMethod.isConstructor() : "Constructors are already rewritten to static factory methods";
         if (targetMethod.isStatic()) {
             invokeKind = InvokeKind.Static;
         } else if (targetMethod.isInterface()) {
             invokeKind = InvokeKind.Interface;
-        } else if (targetMethod.canBeStaticallyBound() || targetMethod.isConstructor()) {
+        } else if (targetMethod.canBeStaticallyBound()) {
             invokeKind = InvokeKind.Special;
         } else {
             invokeKind = InvokeKind.Virtual;
@@ -172,7 +184,7 @@ public StructuredGraph buildGraph(DebugContext ctx, ResolvedJavaMethod m, Hosted
         }
         graphKit.createReturn(returnValue, JavaKind.Object);
 
-        graphKit.emitIllegalArgumentException(method, receiver, argumentArray);
+        graphKit.emitIllegalArgumentException(method, invokeKind == InvokeKind.Static ? null : receiver, argumentArray);
         graphKit.emitInvocationTargetException();
 
         for (InvokeWithExceptionNode invoke : invokes) {