pythongh-102660: Handle m_copy Specially for the sys and builtins Modules (pythongh-102661)

ericsnowcurrently · Fidget-Spinner · commit f4367d5b8341 · 2023-03-27T20:31:53.000+08:00
It doesn't make sense to use multi-phase init for these modules. Using a per-interpreter "m_copy" (instead of PyModuleDef.m_base.m_copy) makes this work okay. (This came up while working on pythongh-101660.) Note that we might instead end up disallowing re-load for sys/builtins since they are so special. python#102660
diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h
@@ -111,6 +111,7 @@ struct _is {
 
     PyObject *dict;  /* Stores per-interpreter state */
 
+    PyObject *sysdict_copy;
     PyObject *builtins_copy;
     // Initialized to _PyEval_EvalFrameDefault().
     _PyFrameEvalFunction eval_frame;
diff --git a/Python/bltinmodule.c b/Python/bltinmodule.c
@@ -3098,6 +3098,9 @@ _PyBuiltin_Init(PyInterpreterState *interp)
     }
     Py_DECREF(debug);
 
+    /* m_copy of Py_None means it is copied some other way. */
+    builtinsmodule.m_base.m_copy = Py_NewRef(Py_None);
+
     return mod;
 #undef ADD_TO_ALL
 #undef SETBUILTIN
diff --git a/Python/import.c b/Python/import.c
@@ -978,6 +978,16 @@ _PyImport_CheckSubinterpIncompatibleExtensionAllowed(const char *name)
     return 0;
 }
 
+static inline int
+match_mod_name(PyObject *actual, const char *expected)
+{
+    if (PyUnicode_CompareWithASCIIString(actual, expected) == 0) {
+        return 1;
+    }
+    assert(!PyErr_Occurred());
+    return 0;
+}
+
 static int
 fix_up_extension(PyObject *mod, PyObject *name, PyObject *filename)
 {
@@ -1001,7 +1011,8 @@ fix_up_extension(PyObject *mod, PyObject *name, PyObject *filename)
     // when the extension module doesn't support sub-interpreters.
     // XXX Why special-case the main interpreter?
     if (_Py_IsMainInterpreter(tstate->interp) || def->m_size == -1) {
-        if (def->m_size == -1) {
+        /* m_copy of Py_None means it is copied some other way. */
+        if (def->m_size == -1 && def->m_base.m_copy != Py_None) {
             if (def->m_base.m_copy) {
                 /* Somebody already imported the module,
                    likely under a different name.
@@ -1055,18 +1066,34 @@ import_find_extension(PyThreadState *tstate, PyObject *name,
     PyObject *modules = MODULES(tstate->interp);
 
     if (def->m_size == -1) {
+        PyObject *m_copy = def->m_base.m_copy;
         /* Module does not support repeated initialization */
-        if (def->m_base.m_copy == NULL)
+        if (m_copy == NULL) {
             return NULL;
+        }
+        else if (m_copy == Py_None) {
+            if (match_mod_name(name, "sys")) {
+                m_copy = tstate->interp->sysdict_copy;
+            }
+            else if (match_mod_name(name, "builtins")) {
+                m_copy = tstate->interp->builtins_copy;
+            }
+            else {
+                _PyErr_SetString(tstate, PyExc_ImportError, "missing m_copy");
+                return NULL;
+            }
+        }
+        /* m_copy of Py_None means it is copied some other way. */
         mod = import_add_module(tstate, name);
-        if (mod == NULL)
+        if (mod == NULL) {
             return NULL;
+        }
         mdict = PyModule_GetDict(mod);
         if (mdict == NULL) {
             Py_DECREF(mod);
             return NULL;
         }
-        if (PyDict_Update(mdict, def->m_base.m_copy)) {
+        if (PyDict_Update(mdict, m_copy)) {
             Py_DECREF(mod);
             return NULL;
         }
diff --git a/Python/pystate.c b/Python/pystate.c
@@ -805,6 +805,7 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate)
     assert(interp->imports.importlib == NULL);
     assert(interp->imports.import_func == NULL);
 
+    Py_CLEAR(interp->sysdict_copy);
     Py_CLEAR(interp->builtins_copy);
     Py_CLEAR(interp->dict);
 #ifdef HAVE_FORK
diff --git a/Python/sysmodule.c b/Python/sysmodule.c
@@ -3425,12 +3425,20 @@ _PySys_Create(PyThreadState *tstate, PyObject **sysmod_p)
         return _PyStatus_ERR("failed to create a module object");
     }
 
+    /* m_copy of Py_None means it is copied some other way. */
+    sysmodule.m_base.m_copy = Py_NewRef(Py_None);
+
     PyObject *sysdict = PyModule_GetDict(sysmod);
     if (sysdict == NULL) {
         goto error;
     }
     interp->sysdict = Py_NewRef(sysdict);
 
+    interp->sysdict_copy = PyDict_Copy(sysdict);
+    if (interp->sysdict_copy == NULL) {
+        goto error;
+    }
+
     if (PyDict_SetItemString(sysdict, "modules", modules) < 0) {
         goto error;
     }

Original file line number	Diff line number	Diff line change
`@@ -3098,6 +3098,9 @@ _PyBuiltin_Init(PyInterpreterState *interp)`
`3098`	`3098`	`}`
`3099`	`3099`	`Py_DECREF(debug);`
`3100`	`3100`
	`3101`	`+ /* m_copy of Py_None means it is copied some other way. */`
	`3102`	`+ builtinsmodule.m_base.m_copy = Py_NewRef(Py_None);`
	`3103`	`+`
`3101`	`3104`	`return mod;`
`3102`	`3105`	`#undef ADD_TO_ALL`
`3103`	`3106`	`#undef SETBUILTIN`
Original file line number	Diff line number	Diff line change
`@@ -3425,12 +3425,20 @@ _PySys_Create(PyThreadState tstate, PyObject *sysmod_p)`
`3425`	`3425`	`return _PyStatus_ERR("failed to create a module object");`
`3426`	`3426`	`}`
`3427`	`3427`
	`3428`	`+ /* m_copy of Py_None means it is copied some other way. */`
	`3429`	`+ sysmodule.m_base.m_copy = Py_NewRef(Py_None);`
	`3430`	`+`
`3428`	`3431`	`PyObject *sysdict = PyModule_GetDict(sysmod);`
`3429`	`3432`	`if (sysdict == NULL) {`
`3430`	`3433`	`goto error;`
`3431`	`3434`	`}`
`3432`	`3435`	`interp->sysdict = Py_NewRef(sysdict);`
`3433`	`3436`
	`3437`	`+ interp->sysdict_copy = PyDict_Copy(sysdict);`
	`3438`	`+ if (interp->sysdict_copy == NULL) {`
	`3439`	`+ goto error;`
	`3440`	`+ }`
	`3441`	`+`
`3434`	`3442`	`if (PyDict_SetItemString(sysdict, "modules", modules) < 0) {`
`3435`	`3443`	`goto error;`
`3436`	`3444`	`}`