gh-117953: Small Cleanup of Extensions-Related Machinery Code (gh-118167)

This is a collection of very basic cleanups I've pulled out of gh-118116.  It is mostly renaming variables and moving a couple bits of code in functionally equivalent ways.

Include/internal/pycore_import.h

@@ -22,6 +22,7 @@ extern int _PyImport_SetModuleString(const char *name, PyObject* module);
 extern void _PyImport_AcquireLock(PyInterpreterState *interp);
 extern int _PyImport_ReleaseLock(PyInterpreterState *interp);
 
+// This is used exclusively for the sys and builtins modules:
 extern int _PyImport_FixupBuiltin(
     PyObject *mod,
     const char *name,            /* UTF-8 encoded string */

Python/import.c

@@ -1125,10 +1125,10 @@ _PyImport_CheckSubinterpIncompatibleExtensionAllowed(const char *name)
 
 static PyObject *
 get_core_module_dict(PyInterpreterState *interp,
-                     PyObject *name, PyObject *filename)
+                     PyObject *name, PyObject *path)
 {
     /* Only builtin modules are core. */
-    if (filename == name) {
+    if (path == name) {
         assert(!PyErr_Occurred());
         if (PyUnicode_CompareWithASCIIString(name, "sys") == 0) {
             return interp->sysdict_copy;
@@ -1143,11 +1143,11 @@ get_core_module_dict(PyInterpreterState *interp,
 }
 
 static inline int
-is_core_module(PyInterpreterState *interp, PyObject *name, PyObject *filename)
+is_core_module(PyInterpreterState *interp, PyObject *name, PyObject *path)
 {
     /* This might be called before the core dict copies are in place,
        so we can't rely on get_core_module_dict() here. */
-    if (filename == name) {
+    if (path == name) {
         if (PyUnicode_CompareWithASCIIString(name, "sys") == 0) {
             return 1;
         }
@@ -1159,7 +1159,7 @@ is_core_module(PyInterpreterState *interp, PyObject *name, PyObject *filename)
 }
 
 static int
-fix_up_extension(PyObject *mod, PyObject *name, PyObject *filename)
+fix_up_extension(PyObject *mod, PyObject *name, PyObject *path)
 {
     if (mod == NULL || !PyModule_Check(mod)) {
         PyErr_BadInternalCall();
@@ -1180,7 +1180,7 @@ fix_up_extension(PyObject *mod, PyObject *name, PyObject *filename)
     // bpo-44050: Extensions and def->m_base.m_copy can be updated
     // when the extension module doesn't support sub-interpreters.
     if (def->m_size == -1) {
-        if (!is_core_module(tstate->interp, name, filename)) {
+        if (!is_core_module(tstate->interp, name, path)) {
             assert(PyUnicode_CompareWithASCIIString(name, "sys") != 0);
             assert(PyUnicode_CompareWithASCIIString(name, "builtins") != 0);
             if (def->m_base.m_copy) {
@@ -1202,7 +1202,7 @@ fix_up_extension(PyObject *mod, PyObject *name, PyObject *filename)
 
     // XXX Why special-case the main interpreter?
     if (_Py_IsMainInterpreter(tstate->interp) || def->m_size == -1) {
-        if (_extensions_cache_set(filename, name, def) < 0) {
+        if (_extensions_cache_set(path, name, def) < 0) {
             return -1;
         }
     }
@@ -1227,10 +1227,10 @@ _PyImport_FixupExtensionObject(PyObject *mod, PyObject *name,
 
 static PyObject *
 import_find_extension(PyThreadState *tstate, PyObject *name,
-                      PyObject *filename)
+                      PyObject *path)
 {
     /* Only single-phase init modules will be in the cache. */
-    PyModuleDef *def = _extensions_cache_get(filename, name);
+    PyModuleDef *def = _extensions_cache_get(path, name);
     if (def == NULL) {
         return NULL;
     }
@@ -1253,7 +1253,7 @@ import_find_extension(PyThreadState *tstate, PyObject *name,
         if (m_copy == NULL) {
             /* It might be a core module (e.g. sys & builtins),
                for which we don't set m_copy. */
-            m_copy = get_core_module_dict(tstate->interp, name, filename);
+            m_copy = get_core_module_dict(tstate->interp, name, path);
             if (m_copy == NULL) {
                 return NULL;
             }
@@ -1292,16 +1292,16 @@ import_find_extension(PyThreadState *tstate, PyObject *name,
     int verbose = _PyInterpreterState_GetConfig(tstate->interp)->verbose;
     if (verbose) {
         PySys_FormatStderr("import %U # previously loaded (%R)\n",
-                           name, filename);
+                           name, path);
     }
     return mod;
 }
 
 static int
 clear_singlephase_extension(PyInterpreterState *interp,
-                            PyObject *name, PyObject *filename)
+                            PyObject *name, PyObject *path)
 {
-    PyModuleDef *def = _extensions_cache_get(filename, name);
+    PyModuleDef *def = _extensions_cache_get(path, name);
     if (def == NULL) {
         if (PyErr_Occurred()) {
             return -1;
@@ -1322,7 +1322,7 @@ clear_singlephase_extension(PyInterpreterState *interp,
     }
 
     /* Clear the cached module def. */
-    _extensions_cache_delete(filename, name);
+    _extensions_cache_delete(path, name);
 
     return 0;
 }
@@ -1336,11 +1336,20 @@ int
 _PyImport_FixupBuiltin(PyObject *mod, const char *name, PyObject *modules)
 {
     int res = -1;
+    assert(mod != NULL && PyModule_Check(mod));
+
     PyObject *nameobj;
     nameobj = PyUnicode_InternFromString(name);
     if (nameobj == NULL) {
         return -1;
     }
+
+    PyModuleDef *def = PyModule_GetDef(mod);
+    if (def == NULL) {
+        PyErr_BadInternalCall();
+        goto finally;
+    }
+
     if (PyObject_SetItem(modules, nameobj, mod) < 0) {
         goto finally;
     }
@@ -1348,6 +1357,7 @@ _PyImport_FixupBuiltin(PyObject *mod, const char *name, PyObject *modules)
         PyMapping_DelItem(modules, nameobj);
         goto finally;
     }
+
     res = 0;
 
 finally:
@@ -1382,39 +1392,45 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec)
     }
 
     PyObject *modules = MODULES(tstate->interp);
+    struct _inittab *found = NULL;
     for (struct _inittab *p = INITTAB; p->name != NULL; p++) {
         if (_PyUnicode_EqualToASCIIString(name, p->name)) {
-            if (p->initfunc == NULL) {
+            found = p;
-                /* Cannot re-init internal module ("sys" or "builtins") */
-                return import_add_module(tstate, name);
-            }
-            mod = (*p->initfunc)();
-            if (mod == NULL) {
-                return NULL;
-            }
-
-            if (PyObject_TypeCheck(mod, &PyModuleDef_Type)) {
-                return PyModule_FromDefAndSpec((PyModuleDef*)mod, spec);
-            }
-            else {
-                /* Remember pointer to module init function. */
-                PyModuleDef *def = PyModule_GetDef(mod);
-                if (def == NULL) {
-                    return NULL;
-                }
-
-                def->m_base.m_init = p->initfunc;
-                if (_PyImport_FixupExtensionObject(mod, name, name,
-                                                   modules) < 0) {
-                    return NULL;
-                }
-                return mod;
-            }
         }
     }
+    if (found == NULL) {
+        // not found
+        Py_RETURN_NONE;
+    }
 
-    // not found
+    PyModInitFunction p0 = (PyModInitFunction)found->initfunc;
-    Py_RETURN_NONE;
+    if (p0 == NULL) {
+        /* Cannot re-init internal module ("sys" or "builtins") */
+        assert(is_core_module(tstate->interp, name, name));
+        return import_add_module(tstate, name);
+    }
+
+    mod = p0();
+    if (mod == NULL) {
+        return NULL;
+    }
+
+    if (PyObject_TypeCheck(mod, &PyModuleDef_Type)) {
+        return PyModule_FromDefAndSpec((PyModuleDef*)mod, spec);
+    }
+    else {
+        /* Remember pointer to module init function. */
+        PyModuleDef *def = PyModule_GetDef(mod);
+        if (def == NULL) {
+            return NULL;
+        }
+
+        def->m_base.m_init = p0;
+        if (_PyImport_FixupExtensionObject(mod, name, name, modules) < 0) {
+            return NULL;
+        }
+        return mod;
+    }
 }
 
 
@@ -3724,7 +3740,7 @@ static PyObject *
 _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file)
 /*[clinic end generated code: output=83249b827a4fde77 input=c31b954f4cf4e09d]*/
 {
-    PyObject *mod, *name, *path;
+    PyObject *mod, *name, *filename;
     FILE *fp;
 
     name = PyObject_GetAttrString(spec, "name");
@@ -3732,36 +3748,56 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file)
         return NULL;
     }
 
-    path = PyObject_GetAttrString(spec, "origin");
+    filename = PyObject_GetAttrString(spec, "origin");
-    if (path == NULL) {
+    if (filename == NULL) {
         Py_DECREF(name);
         return NULL;
     }
 
     PyThreadState *tstate = _PyThreadState_GET();
-    mod = import_find_extension(tstate, name, path);
+    mod = import_find_extension(tstate, name, filename);
     if (mod != NULL || _PyErr_Occurred(tstate)) {
         assert(mod == NULL || !_PyErr_Occurred(tstate));
         goto finally;
     }
 
+    if (PySys_Audit("import", "OOOOO", name, filename,
+                    Py_None, Py_None, Py_None) < 0)
+    {
+        goto finally;
+    }
+
+    /* Is multi-phase init or this is the first time being loaded. */
+
+    /* We would move this (and the fclose() below) into
+     * _PyImport_GetModInitFunc(), but it isn't clear if the intervening
+     * code relies on fp still being open. */
     if (file != NULL) {
-        fp = _Py_fopen_obj(path, "r");
+        fp = _Py_fopen_obj(filename, "r");
         if (fp == NULL) {
             goto finally;
         }
     }
-    else
+    else {
         fp = NULL;
+    }
 
     mod = _PyImport_LoadDynamicModuleWithSpec(spec, fp);
+    if (mod != NULL) {
+        /* Remember the filename as the __file__ attribute */
+        if (PyModule_AddObjectRef(mod, "__file__", filename) < 0) {
+            PyErr_Clear(); /* Not important enough to report */
+        }
+    }
 
-    if (fp)
+    // XXX Shouldn't this happen in the error cases too.
+    if (fp) {
         fclose(fp);
+    }
 
 finally:
     Py_DECREF(name);
-    Py_DECREF(path);
+    Py_DECREF(filename);
     return mod;
 }
 

Python/importdl.c

@@ -97,9 +97,10 @@ PyObject *
 _PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *fp)
 {
 #ifndef MS_WINDOWS
-    PyObject *pathbytes = NULL;
+    PyObject *filename_bytes = NULL;
+    const char *filename_buf;
 #endif
-    PyObject *name_unicode = NULL, *name = NULL, *path = NULL, *m = NULL;
+    PyObject *name_unicode = NULL, *name = NULL, *filename = NULL, *m = NULL;
     const char *name_buf, *hook_prefix;
     const char *oldcontext, *newcontext;
     dl_funcptr exportfunc;
@@ -126,26 +127,23 @@ _PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *fp)
     }
     name_buf = PyBytes_AS_STRING(name);
 
-    path = PyObject_GetAttrString(spec, "origin");
+    filename = PyObject_GetAttrString(spec, "origin");
-    if (path == NULL)
+    if (filename == NULL) {
-        goto error;
-
-    if (PySys_Audit("import", "OOOOO", name_unicode, path,
-                    Py_None, Py_None, Py_None) < 0) {
         goto error;
     }
 
 #ifdef MS_WINDOWS
-    exportfunc = _PyImport_FindSharedFuncptrWindows(hook_prefix, name_buf,
+    exportfunc = _PyImport_FindSharedFuncptrWindows(
-                                                    path, fp);
+            hook_prefix, name_buf, filename, fp);
 #else
-    pathbytes = PyUnicode_EncodeFSDefault(path);
+    filename_bytes = PyUnicode_EncodeFSDefault(filename);
-    if (pathbytes == NULL)
+    if (filename_bytes == NULL) {
         goto error;
-    exportfunc = _PyImport_FindSharedFuncptr(hook_prefix, name_buf,
+    }
-                                             PyBytes_AS_STRING(pathbytes),
+    filename_buf = PyBytes_AS_STRING(filename_bytes);
-                                             fp);
+    exportfunc = _PyImport_FindSharedFuncptr(
-    Py_DECREF(pathbytes);
+            hook_prefix, name_buf, filename_buf, fp);
+    Py_DECREF(filename_bytes);
 #endif
 
     if (exportfunc == NULL) {
@@ -157,7 +155,7 @@ _PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *fp)
                 hook_prefix, name_buf);
             if (msg == NULL)
                 goto error;
-            PyErr_SetImportError(msg, name_unicode, path);
+            PyErr_SetImportError(msg, name_unicode, filename);
             Py_DECREF(msg);
         }
         goto error;
@@ -199,7 +197,7 @@ _PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *fp)
     if (PyObject_TypeCheck(m, &PyModuleDef_Type)) {
         Py_DECREF(name_unicode);
         Py_DECREF(name);
-        Py_DECREF(path);
+        Py_DECREF(filename);
         return PyModule_FromDefAndSpec((PyModuleDef*)m, spec);
     }
 
@@ -228,25 +226,20 @@ _PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *fp)
     }
     def->m_base.m_init = p0;
 
-    /* Remember the filename as the __file__ attribute */
-    if (PyModule_AddObjectRef(m, "__file__", path) < 0) {
-        PyErr_Clear(); /* Not important enough to report */
-    }
-
     PyObject *modules = PyImport_GetModuleDict();
-    if (_PyImport_FixupExtensionObject(m, name_unicode, path, modules) < 0)
+    if (_PyImport_FixupExtensionObject(m, name_unicode, filename, modules) < 0)
         goto error;
 
     Py_DECREF(name_unicode);
     Py_DECREF(name);
-    Py_DECREF(path);
+    Py_DECREF(filename);
 
     return m;
 
 error:
     Py_DECREF(name_unicode);
     Py_XDECREF(name);
-    Py_XDECREF(path);
+    Py_XDECREF(filename);
     Py_XDECREF(m);
     return NULL;
 }