Do not reference run-time objects in C ext root nodes

fangerer · fangerer · commit ec8dad4c2a84 · 2020-11-04T15:45:32.000+01:00
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/ExternalFunctionNodes.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/ExternalFunctionNodes.java
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/PythonCextBuiltins.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/PythonCextBuiltins.java
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/common/CExtContext.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/common/CExtContext.java
@@ -41,6 +41,10 @@
 package com.oracle.graal.python.builtins.objects.cext.common;
 
 import com.oracle.graal.python.runtime.PythonContext;
+import com.oracle.truffle.api.Assumption;
+import com.oracle.truffle.api.CompilerDirectives;
+import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
+import com.oracle.truffle.api.Truffle;
 
 public abstract class CExtContext {
 
@@ -62,6 +66,12 @@ public abstract class CExtContext {
 
     /** A factory for creating context-specific conversion nodes. */
     private final ConversionNodeSupplier supplier;
+    
+    /**
+     * A shared assumption that indicates that the delegate of a built-in function that decorates a
+     * native member method or similar did not change.
+     */
+    @CompilationFinal private Assumption callableStableAssumption;
 
     public CExtContext(PythonContext context, Object llvmLibrary, ConversionNodeSupplier supplier) {
         this.context = context;
@@ -106,4 +116,11 @@ public static boolean isMethFastcallWithKeywords(int flags) {
         return (flags & METH_FASTCALL) != 0 && (flags & METH_KEYWORDS) != 0;
     }
 
+    public Assumption getCallableStableAssumption() {
+        if (callableStableAssumption == null) {
+            CompilerDirectives.transferToInterpreterAndInvalidate();
+            callableStableAssumption = Truffle.getRuntime().createAssumption("method descriptor delegate stable assumption");
+        }
+        return callableStableAssumption;
+    }
 }
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/hpy/GraalHPyNodes.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/hpy/GraalHPyNodes.java
@@ -58,6 +58,7 @@
 
 import com.oracle.graal.python.PythonLanguage;
 import com.oracle.graal.python.builtins.PythonBuiltinClassType;
+import com.oracle.graal.python.builtins.modules.ExternalFunctionNodes;
 import com.oracle.graal.python.builtins.modules.ExternalFunctionNodes.MethKeywordsRoot;
 import com.oracle.graal.python.builtins.modules.ExternalFunctionNodes.MethNoargsRoot;
 import com.oracle.graal.python.builtins.modules.ExternalFunctionNodes.MethORoot;
@@ -67,6 +68,7 @@
 import com.oracle.graal.python.builtins.modules.PythonCextBuiltins.MethONode;
 import com.oracle.graal.python.builtins.modules.PythonCextBuiltins.MethVarargsNode;
 import com.oracle.graal.python.builtins.objects.PNone;
+import com.oracle.graal.python.builtins.objects.cell.PCell;
 import com.oracle.graal.python.builtins.objects.cext.PythonNativeObject;
 import com.oracle.graal.python.builtins.objects.cext.capi.CExtNodes.FromCharPointerNode;
 import com.oracle.graal.python.builtins.objects.cext.common.CExtCommonNodes.ConvertPIntToPrimitiveNode;
@@ -444,8 +446,9 @@ static PBuiltinFunction doIt(GraalHPyContext context, Object methodDef,
 
             // CPy-style methods
             // TODO(fa) support static and class methods
-            PRootNode rootNode = createWrapperRootNode(language, flags, methodName, mlMethObj);
-            PBuiltinFunction function = createWrapperFunction(factory, methodName, rootNode);
+            PRootNode rootNode = createWrapperRootNode(language, flags, methodName);
+            PCell[] closure = ExternalFunctionNodes.createPythonClosure(mlMethObj, factory, context.getCallableStableAssumption());
+            PBuiltinFunction function = factory.createBuiltinFunction(methodName, null, 0, closure, PythonUtils.getOrCreateCallTarget(rootNode));
 
             // write doc string; we need to directly write to the storage otherwise it is disallowed
             // writing to builtin types.
@@ -467,20 +470,15 @@ private static boolean checkLayout(Object methodDef) {
         }
 
         @TruffleBoundary
-        private static PBuiltinFunction createWrapperFunction(PythonObjectFactory factory, String name, PRootNode rootNode) {
-            return factory.createBuiltinFunction(name, null, 0, PythonUtils.getOrCreateCallTarget(rootNode));
-        }
-
-        @TruffleBoundary
-        private static PRootNode createWrapperRootNode(PythonLanguage language, int flags, String name, Object callable) {
+        private static PRootNode createWrapperRootNode(PythonLanguage language, int flags, String name) {
             if (CExtContext.isMethNoArgs(flags)) {
-                return new MethNoargsRoot(language, name, callable, MethNoargsNode.METH_NOARGS_CONVERTER);
+                return new MethNoargsRoot(language, name, MethNoargsNode.METH_NOARGS_CONVERTER);
             } else if (CExtContext.isMethO(flags)) {
-                return new MethORoot(language, name, callable, MethONode.METH_O_CONVERTER);
+                return new MethORoot(language, name, MethONode.METH_O_CONVERTER);
             } else if (CExtContext.isMethKeywords(flags)) {
-                return new MethKeywordsRoot(language, name, callable, MethKeywordsNode.METH_KEYWORDS_CONVERTER);
+                return new MethKeywordsRoot(language, name, MethKeywordsNode.METH_KEYWORDS_CONVERTER);
             } else if (CExtContext.isMethVarargs(flags)) {
-                return new MethVarargsRoot(language, name, callable, MethVarargsNode.METH_VARARGS_CONVERTER);
+                return new MethVarargsRoot(language, name, MethVarargsNode.METH_VARARGS_CONVERTER);
             }
             throw new IllegalStateException("illegal method flags");
         }
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/function/PBuiltinFunction.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/function/PBuiltinFunction.java
@@ -33,6 +33,7 @@
 import com.oracle.graal.python.builtins.BoundBuiltinCallable;
 import com.oracle.graal.python.builtins.PythonBuiltinClassType;
 import com.oracle.graal.python.builtins.objects.PNone;
+import com.oracle.graal.python.builtins.objects.cell.PCell;
 import com.oracle.graal.python.builtins.objects.function.PArguments.ThreadState;
 import com.oracle.graal.python.builtins.objects.object.PythonBuiltinObject;
 import com.oracle.graal.python.builtins.objects.object.PythonObjectLibrary;
@@ -72,10 +73,24 @@ public final class PBuiltinFunction extends PythonBuiltinObject implements Bound
     private final Object enclosingType;
     private final RootCallTarget callTarget;
     private final Signature signature;
+
+    /**
+     * Built-in function can usually not have a closure but we also use this class to represent
+     * method descriptors and for those, we need to store the actual receiver (e.g. a native
+     * function pointer) and pass the closure to the appropriate root node. So, if the built-in
+     * function is implemented in Java (which is the main use case), the closure will be
+     * {@code null}.
+     */
+    @CompilationFinal(dimensions = 1) private final PCell[] closure;
+
     @CompilationFinal(dimensions = 1) private final PNone[] defaults;
     @CompilationFinal(dimensions = 1) private final PKeyword[] kwDefaults;
 
     public PBuiltinFunction(PythonLanguage lang, String name, Object enclosingType, int numDefaults, RootCallTarget callTarget) {
+        this(lang, name, enclosingType, numDefaults, null, callTarget);
+    }
+    
+    public PBuiltinFunction(PythonLanguage lang, String name, Object enclosingType, int numDefaults, PCell[] closure, RootCallTarget callTarget) {
         super(PythonBuiltinClassType.PBuiltinFunction, PythonBuiltinClassType.PBuiltinFunction.getInstanceShape(lang));
         this.name = name;
         if (enclosingType != null) {
@@ -93,6 +108,7 @@ public PBuiltinFunction(PythonLanguage lang, String name, Object enclosingType,
         for (int i = 0; i < keywordNames.length; i++) {
             kwDefaults[i] = new PKeyword(keywordNames[i], PNone.NO_VALUE);
         }
+        this.closure = closure;
     }
 
     public RootNode getFunctionRootNode() {
@@ -145,6 +161,10 @@ public Object getEnclosingType() {
         return enclosingType;
     }
 
+    public PCell[] getClosure() {
+        return closure;
+    }
+
     @Override
     public String toString() {
         CompilerAsserts.neverPartOfCompilation();
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/call/CallDispatchNode.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/call/CallDispatchNode.java
@@ -129,7 +129,7 @@ protected Object callBuiltinFunctionCached(VirtualFrame frame, @SuppressWarnings
     protected Object callBuiltinFunctionCachedCt(VirtualFrame frame, @SuppressWarnings("unused") PBuiltinFunction callee, Object[] arguments,
                     @SuppressWarnings("unused") @Cached("callee.getCallTarget()") RootCallTarget ct,
                     @Cached("createCtInvokeNode(callee)") CallTargetInvokeNode invoke) {
-        return invoke.execute(frame, null, null, null, arguments);
+        return invoke.execute(frame, null, null, callee.getClosure(), arguments);
     }
 
     @Specialization(replaces = {"callFunctionCached", "callFunctionCachedCode", "callFunctionCachedCt"})
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/call/FunctionInvokeNode.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/call/FunctionInvokeNode.java
@@ -128,7 +128,7 @@ public static FunctionInvokeNode create(PFunction callee) {
     public static FunctionInvokeNode create(PBuiltinFunction callee) {
         RootCallTarget callTarget = getCallTarget(callee);
         boolean builtin = isBuiltin(callee);
-        return FunctionInvokeNodeGen.create(null, callTarget, null, null, builtin, false);
+        return FunctionInvokeNodeGen.create(null, callTarget, null, callee.getClosure(), builtin, false);
     }
 
     /**
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/call/GenericInvokeNode.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/call/GenericInvokeNode.java
@@ -158,6 +158,7 @@ Object invokeBuiltin(Frame frame, PBuiltinFunction callee, Object[] arguments,
                     @Shared("isNullFrameProfile") @Cached("createBinaryProfile()") ConditionProfile isNullFrameProfile,
                     @Shared("isClassBodyProfile") @Cached("createBinaryProfile()") ConditionProfile isClassBodyProfile,
                     @Shared("isGeneratorFunctionProfile") @Cached("createBinaryProfile()") ConditionProfile isGeneratorFunctionProfile) {
+        PArguments.setClosure(arguments, callee.getClosure());
         RootCallTarget callTarget = getCallTarget(callee);
         return doCall(frame, null, callTarget, arguments, context, callNode, callContext, isNullFrameProfile, isClassBodyProfile, isGeneratorFunctionProfile);
     }
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/call/special/CallSpecialMethodNode.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/call/special/CallSpecialMethodNode.java
@@ -95,6 +95,11 @@ private <T extends PythonBuiltinBaseNode> T getBuiltin(VirtualFrame frame, PBuil
         if (builtinNodeFactory == null) {
             return null; // see for example MethodDescriptorRoot and subclasses
         }
+        // built-in function that have a closure cannot be AST-inlined otherwise we would
+        // potentially overwrite the caller's closure
+        if (func.getClosure() != null) {
+            return null;
+        }
         assert builtinNodeFactory.getNodeClass().getAnnotationsByType(Builtin.class).length > 0 : "PBuiltinFunction " + func + " is expected to have a Builtin annotated node.";
         if (builtinNodeFactory.getNodeClass().getAnnotationsByType(Builtin.class)[0].needsFrame() && frame == null) {
             return null;
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/runtime/object/PythonObjectFactory.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/runtime/object/PythonObjectFactory.java
@@ -463,6 +463,16 @@ public PBuiltinFunction createBuiltinFunction(String name, Object type, int numD
         return trace(new PBuiltinFunction(getLanguage(), name, type, numDefaults, callTarget));
     }
 
+    /**
+     * Creates a built-in function that has a closure. This is mostly useful if the built-in
+     * function is not implemented in Java and needs to delegate to a different receiver. Then the
+     * closure can store this receiver. However, as soon as you have a non-null closure,
+     * AST-inlining won't be possible.
+     */
+    public PBuiltinFunction createBuiltinFunction(String name, Object type, int numDefaults, PCell[] closure, RootCallTarget callTarget) {
+        return trace(new PBuiltinFunction(getLanguage(), name, type, numDefaults, closure, callTarget));
+    }
+
     public GetSetDescriptor createGetSetDescriptor(Object get, Object set, String name, Object type) {
         return trace(new GetSetDescriptor(getLanguage(), get, set, name, type));
     }

Original file line number	Diff line number	Diff line change
`@@ -129,7 +129,7 @@ protected Object callBuiltinFunctionCached(VirtualFrame frame, @SuppressWarnings`
`129`	`129`	`protected Object callBuiltinFunctionCachedCt(VirtualFrame frame, @SuppressWarnings("unused") PBuiltinFunction callee, Object[] arguments,`
`130`	`130`	`@SuppressWarnings("unused") @Cached("callee.getCallTarget()") RootCallTarget ct,`
`131`	`131`	`@Cached("createCtInvokeNode(callee)") CallTargetInvokeNode invoke) {`
`132`		`- return invoke.execute(frame, null, null, null, arguments);`
	`132`	`+ return invoke.execute(frame, null, null, callee.getClosure(), arguments);`
`133`	`133`	`}`
`134`	`134`
`135`	`135`	`@Specialization(replaces = {"callFunctionCached", "callFunctionCachedCode", "callFunctionCachedCt"})`
Original file line number	Diff line number	Diff line change
`@@ -128,7 +128,7 @@ public static FunctionInvokeNode create(PFunction callee) {`
`128`	`128`	`public static FunctionInvokeNode create(PBuiltinFunction callee) {`
`129`	`129`	`RootCallTarget callTarget = getCallTarget(callee);`
`130`	`130`	`boolean builtin = isBuiltin(callee);`
`131`		`- return FunctionInvokeNodeGen.create(null, callTarget, null, null, builtin, false);`
	`131`	`+ return FunctionInvokeNodeGen.create(null, callTarget, null, callee.getClosure(), builtin, false);`
`132`	`132`	`}`
`133`	`133`
`134`	`134`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -158,6 +158,7 @@ Object invokeBuiltin(Frame frame, PBuiltinFunction callee, Object[] arguments,`
`158`	`158`	`@Shared("isNullFrameProfile") @Cached("createBinaryProfile()") ConditionProfile isNullFrameProfile,`
`159`	`159`	`@Shared("isClassBodyProfile") @Cached("createBinaryProfile()") ConditionProfile isClassBodyProfile,`
`160`	`160`	`@Shared("isGeneratorFunctionProfile") @Cached("createBinaryProfile()") ConditionProfile isGeneratorFunctionProfile) {`
	`161`	`+ PArguments.setClosure(arguments, callee.getClosure());`
`161`	`162`	`RootCallTarget callTarget = getCallTarget(callee);`
`162`	`163`	`return doCall(frame, null, callTarget, arguments, context, callNode, callContext, isNullFrameProfile, isClassBodyProfile, isGeneratorFunctionProfile);`
`163`	`164`	`}`