Enforce more constaints concerning multi-phase module init

Author: fangerer

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/hpy/GraalHPyNodes.java

@@ -429,7 +429,6 @@ static Object doGeneric(GraalHPyContext hpyContext, TruffleString mName, Object
                         @Cached HPyCreateFunctionNode addFunctionNode,
                         @Cached CreateMethodNode addLegacyMethodNode,
                         @Cached HPyReadSlotNode readSlotNode,
-                        @Cached HPyAttachFunctionTypeNode attachFunctionTypeNode,
                         @Cached HPyCheckHandleResultNode checkFunctionResultNode,
                         @Cached HPyAsHandleNode asHandleNode,
                         @CachedLibrary(limit = "1") InteropLibrary createLib,
@@ -441,6 +440,7 @@ static Object doGeneric(GraalHPyContext hpyContext, TruffleString mName, Object
             assert checkLayout(moduleDef);
 
             TruffleString mDoc;
+            long size;
             try {
                 Object docPtr = ptrLib.readMember(moduleDef, "doc");
                 if (!ptrLib.isNull(docPtr)) {
@@ -450,7 +450,7 @@ static Object doGeneric(GraalHPyContext hpyContext, TruffleString mName, Object
                 }
 
                 Object sizeObj = ptrLib.readMember(moduleDef, "size");
-                long size = valueLib.asLong(sizeObj);
+                size = valueLib.asLong(sizeObj);
                 if (size < 0) {
                     throw raiseNode.raise(PythonBuiltinClassType.SystemError, tsLiteral("HPy does not permit HPyModuleDef.size < 0"));
                 } else if (size > 0) {
@@ -483,7 +483,6 @@ static Object doGeneric(GraalHPyContext hpyContext, TruffleString mName, Object
             int nMethodDefs = 0;
             Object[] methodDefs = new Object[nModuleDefines];
 
-            boolean hasExecutionSlots = false;
             List<Object> executeSlots = new LinkedList<>();
             Object createFunction = null;
 
@@ -507,6 +506,9 @@ static Object doGeneric(GraalHPyContext hpyContext, TruffleString mName, Object
                                     assert createLib.isExecutable(createFunction);
                                 }
                                 case HPY_MOD_EXEC -> {
+                                    if (createFunction != null) {
+                                        throw raiseNode.raise(PythonErrorType.SystemError, ErrorMessages.HPY_DEFINES_CREATE_AND_OTHER_SLOTS, mName);
+                                    }
                                     /*
                                      * In contrast to CPython, we already parse and store the
                                      * HPy_mod_exec slots here since parsing is a bit more expensive
@@ -532,9 +534,33 @@ static Object doGeneric(GraalHPyContext hpyContext, TruffleString mName, Object
                 // should not happen since we check if 'moduleDefines' has array elements
                 throw CompilerDirectives.shouldNotReachHere();
             }
+
+            // determine of 'legacy_methods' is NULL upfront (required for a consistency check)
+            Object legacyMethods = callGetterNode.call(hpyContext, GRAAL_HPY_MODULE_GET_LEGACY_METHODS, moduleDef);
+            // the field 'legacy_methods' may be 'NULL'
+            boolean hasLegacyMethods = !ptrLib.isNull(legacyMethods);
+
+            // allocate module's HPyGlobals
+            int nModuleGlobals;
+            try {
+                int globalStartIdx = hpyContext.getEndIndexOfGlobalTable();
+                nModuleGlobals = ptrLib.asInt(callGetterNode.call(hpyContext, GRAAL_HPY_MODULE_INIT_GLOBALS, moduleDef, globalStartIdx));
+                hpyContext.initBatchGlobals(globalStartIdx, nModuleGlobals);
+            } catch (UnsupportedMessageException e) {
+                // should not happen unless the number of module global is larger than an `int`
+                throw CompilerDirectives.shouldNotReachHere();
+            }
+
             // create the module object
             Object module;
             if (createFunction != null) {
+                /*
+                 * TODO(fa): this check should be before any other check (and the also test for
+                 * 'size > 0')
+                 */
+                if (hasLegacyMethods || mDoc != null || nModuleGlobals != 0) {
+                    throw raiseNode.raise(SystemError, ErrorMessages.HPY_DEFINES_CREATE_AND_NON_DEFAULT);
+                }
                 module = callCreate(inliningTarget, createFunction, hpyContext, spec, checkFunctionResultNode, asHandleNode, createLib);
                 if (module instanceof PythonModule) {
                     throw raiseNode.raise(SystemError, ErrorMessages.HPY_MOD_CREATE_RETURNED_BUILTIN_MOD);
@@ -555,9 +581,7 @@ static Object doGeneric(GraalHPyContext hpyContext, TruffleString mName, Object
             }
 
             // process legacy methods
-            Object legacyMethods = callGetterNode.call(hpyContext, GRAAL_HPY_MODULE_GET_LEGACY_METHODS, moduleDef);
-            // the field 'legacy_methods' may be 'NULL'
-            if (!ptrLib.isNull(legacyMethods)) {
+            if (hasLegacyMethods) {
                 if (!ptrLib.hasArrayElements(legacyMethods)) {
                     CompilerDirectives.transferToInterpreterAndInvalidate();
                     throw raiseNode.raise(PythonBuiltinClassType.SystemError, ErrorMessages.FIELD_S_DID_NOT_RETURN_AN_ARRAY, "legacyMethods");
@@ -574,16 +598,6 @@ static Object doGeneric(GraalHPyContext hpyContext, TruffleString mName, Object
                 }
             }
 
-            // allocate module's HPyGlobals
-            try {
-                int globalStartIdx = hpyContext.getEndIndexOfGlobalTable();
-                int nModuleGlobals = ptrLib.asInt(callGetterNode.call(hpyContext, GRAAL_HPY_MODULE_INIT_GLOBALS, moduleDef, globalStartIdx));
-                hpyContext.initBatchGlobals(globalStartIdx, nModuleGlobals);
-            } catch (UnsupportedMessageException e) {
-                // should not happen unless the number of module global is larger than an `int`
-                throw CompilerDirectives.shouldNotReachHere();
-            }
-
             if (mDoc != null) {
                 writeAttrNode.execute(module, SpecialAttributeNames.T___DOC__, mDoc);
             }

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/ErrorMessages.java

@@ -1485,6 +1485,13 @@ public abstract class ErrorMessages {
     public static final TruffleString HPY_CANNOT_SPECIFY_LEG_SLOTS_WO_SETTING_LEG = tsLiteral("cannot specify .legacy_slots without setting .builtin_shape=HPyType_BuiltinShape_Legacy");
     public static final TruffleString HPY_CANNOT_USE_CALL_WITH_LEGACY = tsLiteral("Cannot use HPy call protocol with legacy types that inherit the struct. " +
                     "Either set the basicsize to a non-zero value or use legacy slot 'Py_tp_call'.");
+    private static final String HPY_NON_DEFAULT_MESSAGE = "This is not allowed because custom HPy_mod_create slot cannot return a builtin module object and cannot make any use of any other " +
+                    "data defined in the HPyModuleDef. Either do not define HPy_mod_create slot and let the runtime create a builtin module object from the provided HPyModuleDef, or do not " +
+                    "define anything else but the HPy_mod_create slot.";
+    public static final TruffleString HPY_DEFINES_CREATE_AND_NON_DEFAULT = tsLiteral("HPyModuleDef defines a HPy_mod_create slot and some of the other fields are not set to their default value. " +
+                    HPY_NON_DEFAULT_MESSAGE);
+    public static final TruffleString HPY_DEFINES_CREATE_AND_OTHER_SLOTS = tsLiteral("HPyModuleDef defines a HPy_mod_create slot and some other slots or methods. " +
+                    HPY_NON_DEFAULT_MESSAGE);
 
     // AST Validator
     public static final TruffleString ANN_ASSIGN_WITH_SIMPLE_NON_NAME_TARGET = tsLiteral("AnnAssign with simple non-Name target");