From 79f39a405e1290bcdf8a787785bfe93df25a7689 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 16 Apr 2024 14:52:27 -0600 Subject: [PATCH 01/23] Add _Py_ext_module_loader_result.singlephase. --- Include/internal/pycore_importdl.h | 1 + Python/importdl.c | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/Include/internal/pycore_importdl.h b/Include/internal/pycore_importdl.h index adb89167d124eb..8cce8e7f2d5144 100644 --- a/Include/internal/pycore_importdl.h +++ b/Include/internal/pycore_importdl.h @@ -46,6 +46,7 @@ extern int _Py_ext_module_loader_info_init_from_spec( struct _Py_ext_module_loader_result { PyModuleDef *def; PyObject *module; + int singlephase; }; extern PyModInitFunction _PyImport_GetModInitFunc( struct _Py_ext_module_loader_info *info, diff --git a/Python/importdl.c b/Python/importdl.c index 3e4ea175c0cfbc..d384a761e82aba 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -245,7 +245,9 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, struct _Py_ext_module_loader_info *info, struct _Py_ext_module_loader_result *p_res) { - struct _Py_ext_module_loader_result res = {0}; + struct _Py_ext_module_loader_result res = { + .singlephase=-1, + }; const char *name_buf = PyBytes_AS_STRING(info->name_encoded); /* Call the module init function. */ @@ -293,6 +295,7 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, if (PyObject_TypeCheck(m, &PyModuleDef_Type)) { /* multi-phase init */ + res.singlephase = 0; res.def = (PyModuleDef *)m; /* Run PyModule_FromDefAndSpec() to finish loading the module. */ } @@ -308,6 +311,7 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, } else { /* single-phase init (legacy) */ + res.singlephase = 1; res.module = m; if (!PyModule_Check(m)) { From aa78a49d908cc90ed916e31d1641a0b835d5bdd7 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 17 Apr 2024 13:43:37 -0600 Subject: [PATCH 02/23] res.singlephase -> res.kind --- Include/internal/pycore_importdl.h | 7 ++++++- Python/importdl.c | 7 ++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/Include/internal/pycore_importdl.h b/Include/internal/pycore_importdl.h index 8cce8e7f2d5144..de0e7d08306e63 100644 --- a/Include/internal/pycore_importdl.h +++ b/Include/internal/pycore_importdl.h @@ -46,7 +46,12 @@ extern int _Py_ext_module_loader_info_init_from_spec( struct _Py_ext_module_loader_result { PyModuleDef *def; PyObject *module; - int singlephase; + enum _Py_ext_module_loader_result_kind { + _Py_ext_module_loader_result_UNKNOWN = 0, + _Py_ext_module_loader_result_SINGLEPHASE = 1, + _Py_ext_module_loader_result_MULTIPHASE = 2, + _Py_ext_module_loader_result_INVALID = 3, + } kind; }; extern PyModInitFunction _PyImport_GetModInitFunc( struct _Py_ext_module_loader_info *info, diff --git a/Python/importdl.c b/Python/importdl.c index d384a761e82aba..4c765105a4e378 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -246,7 +246,7 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, struct _Py_ext_module_loader_result *p_res) { struct _Py_ext_module_loader_result res = { - .singlephase=-1, + .kind=_Py_ext_module_loader_result_UNKNOWN, }; const char *name_buf = PyBytes_AS_STRING(info->name_encoded); @@ -283,6 +283,7 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, /* This can happen when a PyModuleDef is returned without calling * PyModuleDef_Init on it */ + res.kind = _Py_ext_module_loader_result_INVALID; PyErr_Format(PyExc_SystemError, "init function of %s returned uninitialized object", name_buf); @@ -295,7 +296,7 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, if (PyObject_TypeCheck(m, &PyModuleDef_Type)) { /* multi-phase init */ - res.singlephase = 0; + res.kind = _Py_ext_module_loader_result_MULTIPHASE; res.def = (PyModuleDef *)m; /* Run PyModule_FromDefAndSpec() to finish loading the module. */ } @@ -311,7 +312,7 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, } else { /* single-phase init (legacy) */ - res.singlephase = 1; + res.kind = _Py_ext_module_loader_result_SINGLEPHASE; res.module = m; if (!PyModule_Check(m)) { From 938e50fcf174e3494a80359f9f9fd15b79df5ad4 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 16 Apr 2024 14:52:27 -0600 Subject: [PATCH 03/23] Add _Py_ext_module_loader_result.err. --- Include/internal/pycore_importdl.h | 4 ++ Python/import.c | 3 +- Python/importdl.c | 60 ++++++++++++++++++++---------- 3 files changed, 47 insertions(+), 20 deletions(-) diff --git a/Include/internal/pycore_importdl.h b/Include/internal/pycore_importdl.h index de0e7d08306e63..d6872c9ed00d9d 100644 --- a/Include/internal/pycore_importdl.h +++ b/Include/internal/pycore_importdl.h @@ -52,7 +52,11 @@ struct _Py_ext_module_loader_result { _Py_ext_module_loader_result_MULTIPHASE = 2, _Py_ext_module_loader_result_INVALID = 3, } kind; + char err[200]; }; +extern void _Py_ext_module_loader_result_apply_error( + struct _Py_ext_module_loader_result *res); + extern PyModInitFunction _PyImport_GetModInitFunc( struct _Py_ext_module_loader_info *info, FILE *fp); diff --git a/Python/import.c b/Python/import.c index 447114ab115bc7..ddc91feed53ee6 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1369,6 +1369,7 @@ import_find_extension(PyThreadState *tstate, } struct _Py_ext_module_loader_result res; if (_PyImport_RunModInitFunc(def->m_base.m_init, info, &res) < 0) { + _Py_ext_module_loader_result_apply_error(&res); return NULL; } assert(!PyErr_Occurred()); @@ -1405,7 +1406,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, if (_PyImport_RunModInitFunc(p0, info, &res) < 0) { /* We discard res.def. */ assert(res.module == NULL); - assert(PyErr_Occurred()); + _Py_ext_module_loader_result_apply_error(&res); goto finally; } assert(!PyErr_Occurred()); diff --git a/Python/importdl.c b/Python/importdl.c index 4c765105a4e378..f39cadad4f117e 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -205,6 +205,24 @@ _Py_ext_module_loader_info_init_from_spec( } +void +_Py_ext_module_loader_result_apply_error( + struct _Py_ext_module_loader_result *res) +{ + if (res->err[0] != '\0') { + if (PyErr_Occurred()) { + _PyErr_FormatFromCause(PyExc_SystemError, res->err); + } + else { + PyErr_SetString(PyExc_SystemError, res->err); + } + } + else { + assert(PyErr_Occurred()); + } +} + + PyModInitFunction _PyImport_GetModInitFunc(struct _Py_ext_module_loader_info *info, FILE *fp) @@ -259,19 +277,27 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, /* Validate the result (and populate "res". */ +#ifdef NDEBUG +# define SET_ERROR(...) \ + (void)snprintf(res.err, Py_ARRAY_LENGTH(res.err), __VA_ARGS__) +#else +# define SET_ERROR(...) \ + do { \ + int n = snprintf(res.err, Py_ARRAY_LENGTH(res.err), __VA_ARGS__); \ + assert(n < Py_ARRAY_LENGTH(res.err)); \ + } while (0) +#endif + if (m == NULL) { if (!PyErr_Occurred()) { - PyErr_Format( - PyExc_SystemError, + SET_ERROR( "initialization of %s failed without raising an exception", name_buf); } goto error; } else if (PyErr_Occurred()) { - _PyErr_FormatFromCause( - PyExc_SystemError, - "initialization of %s raised unreported exception", - name_buf); + SET_ERROR("initialization of %s raised unreported exception", + name_buf); /* We would probably be correct to decref m here, * but we weren't doing so before, * so we stick with doing nothing. */ @@ -284,9 +310,8 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, * PyModuleDef_Init on it */ res.kind = _Py_ext_module_loader_result_INVALID; - PyErr_Format(PyExc_SystemError, - "init function of %s returned uninitialized object", - name_buf); + SET_ERROR("init function of %s returned uninitialized object", + name_buf); /* Likewise, decref'ing here makes sense. However, the original * code has a note about "prevent segfault in DECREF", * so we play it safe and leave it alone. */ @@ -303,12 +328,9 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, else if (info->hook_prefix == nonascii_prefix) { /* It should have been multi-phase init? */ /* Don't allow legacy init for non-ASCII module names. */ - PyErr_Format( - PyExc_SystemError, - "initialization of %s did not return PyModuleDef", - name_buf); - Py_DECREF(m); - return -1; + SET_ERROR("initialization of %s did not return PyModuleDef", + name_buf); + goto error; } else { /* single-phase init (legacy) */ @@ -324,19 +346,19 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, res.def = _PyModule_GetDef(m); if (res.def == NULL) { - PyErr_Format(PyExc_SystemError, - "initialization of %s did not return a valid extension " - "module", name_buf); + SET_ERROR("initialization of %s did not return a valid extension " + "module", name_buf); goto error; } } +#undef SET_ERROR assert(!PyErr_Occurred()); *p_res = res; return 0; error: - assert(PyErr_Occurred()); + assert(PyErr_Occurred() || res.err[0] != '\0'); Py_CLEAR(res.module); res.def = NULL; *p_res = res; From 9383ce1d66df8913b057691f9f3f984a7bf45de4 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 16 Apr 2024 14:52:27 -0600 Subject: [PATCH 04/23] fix error case --- Python/importdl.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Python/importdl.c b/Python/importdl.c index f39cadad4f117e..8994df41578a00 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -338,9 +338,8 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, res.module = m; if (!PyModule_Check(m)) { - PyErr_Format(PyExc_SystemError, - "initialization of %s did not return an extension " - "module", name_buf); + SET_ERROR("initialization of %s did not return an extension " + "module", name_buf); goto error; } From 62166ae5975bd5929f1cf1296e8deffa2495dc9c Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 18 Apr 2024 15:09:46 -0600 Subject: [PATCH 05/23] Add some asserts to _PyImport_RunModInitFunc(). --- Python/importdl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Python/importdl.c b/Python/importdl.c index 8994df41578a00..8075dad51784b1 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -345,6 +345,7 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, res.def = _PyModule_GetDef(m); if (res.def == NULL) { + PyErr_Clear(); SET_ERROR("initialization of %s did not return a valid extension " "module", name_buf); goto error; @@ -357,7 +358,7 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, return 0; error: - assert(PyErr_Occurred() || res.err[0] != '\0'); + assert((PyErr_Occurred() == NULL) != (res.err[0] == '\0')); Py_CLEAR(res.module); res.def = NULL; *p_res = res; From 48811196e3c9af445757057e0d2071497df45b1a Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 18 Apr 2024 16:24:25 -0600 Subject: [PATCH 06/23] Check the module returned by import_find_extension() to ensure single-phase init. --- Python/import.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/Python/import.c b/Python/import.c index ddc91feed53ee6..8d50b8285989dc 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1193,20 +1193,28 @@ is_core_module(PyInterpreterState *interp, PyObject *name, PyObject *path) } #ifndef NDEBUG -static bool -is_singlephase(PyModuleDef *def) +static enum _Py_ext_module_loader_result_kind +get_extension_kind(PyModuleDef *def) { + enum _Py_ext_module_loader_result_kind kind; if (def == NULL) { /* It must be a module created by reload_singlephase_extension() * from m_copy. Ideally we'd do away with this case. */ - return true; + kind = _Py_ext_module_loader_result_SINGLEPHASE; } else if (def->m_slots == NULL) { - return true; + kind = _Py_ext_module_loader_result_SINGLEPHASE; } else { - return false; + kind = _Py_ext_module_loader_result_MULTIPHASE; } + return kind; +} + +static bool +is_singlephase(PyModuleDef *def) +{ + return get_extension_kind(def) == _Py_ext_module_loader_result_SINGLEPHASE; } #endif @@ -1373,7 +1381,9 @@ import_find_extension(PyThreadState *tstate, return NULL; } assert(!PyErr_Occurred()); + assert(res.kind == _Py_ext_module_loader_result_SINGLEPHASE); mod = res.module; + // XXX __file__ doesn't get set! if (PyObject_SetItem(modules, info->name, mod) == -1) { Py_DECREF(mod); @@ -1416,7 +1426,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, def = res.def; assert(def != NULL); - if (mod == NULL) { + if (res.kind == _Py_ext_module_loader_result_MULTIPHASE) { //assert(!is_singlephase(def)); assert(mod == NULL); mod = PyModule_FromDefAndSpec(def, spec); @@ -1425,6 +1435,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, } } else { + assert(res.kind == _Py_ext_module_loader_result_SINGLEPHASE); assert(is_singlephase(def)); assert(PyModule_Check(mod)); From 9010c2cb78fe8942b95b023b6447a9180c64dd7e Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 23 Apr 2024 19:15:28 -0600 Subject: [PATCH 07/23] Relax the error check. --- Python/importdl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/importdl.c b/Python/importdl.c index 8075dad51784b1..f6b0ddf1f78a4d 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -358,7 +358,7 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, return 0; error: - assert((PyErr_Occurred() == NULL) != (res.err[0] == '\0')); + assert(PyErr_Occurred() || res.err[0] != '\0'); Py_CLEAR(res.module); res.def = NULL; *p_res = res; From b08c5f520e1ba085bee06c21282ebd548105541f Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 24 Apr 2024 13:49:31 -0600 Subject: [PATCH 08/23] Use an enum instead of storing an error string. --- Include/internal/pycore_importdl.h | 25 +++- Python/import.c | 22 +++- Python/importdl.c | 193 +++++++++++++++++++++++------ 3 files changed, 191 insertions(+), 49 deletions(-) diff --git a/Include/internal/pycore_importdl.h b/Include/internal/pycore_importdl.h index d6872c9ed00d9d..54c4279a8dd12d 100644 --- a/Include/internal/pycore_importdl.h +++ b/Include/internal/pycore_importdl.h @@ -15,8 +15,7 @@ extern "C" { extern const char *_PyImport_DynLoadFiletab[]; -typedef PyObject *(*PyModInitFunction)(void); - +/* Input for loading an extension module. */ struct _Py_ext_module_loader_info { PyObject *filename; #ifndef MS_WINDOWS @@ -43,6 +42,7 @@ extern int _Py_ext_module_loader_info_init_from_spec( struct _Py_ext_module_loader_info *info, PyObject *spec); +/* The result from running an extension module's init function. */ struct _Py_ext_module_loader_result { PyModuleDef *def; PyObject *module; @@ -52,11 +52,28 @@ struct _Py_ext_module_loader_result { _Py_ext_module_loader_result_MULTIPHASE = 2, _Py_ext_module_loader_result_INVALID = 3, } kind; - char err[200]; + struct _Py_ext_module_loader_result_error *err; + struct _Py_ext_module_loader_result_error { + enum _Py_ext_module_loader_result_error_kind { + _Py_ext_module_loader_result_EXCEPTION = 0, + _Py_ext_module_loader_result_ERR_MISSING = 1, + _Py_ext_module_loader_result_ERR_UNREPORTED_EXC = 2, + _Py_ext_module_loader_result_ERR_UNINITIALIZED = 3, + _Py_ext_module_loader_result_ERR_NONASCII_NOT_MULTIPHASE = 4, + _Py_ext_module_loader_result_ERR_NOT_MODULE = 5, + _Py_ext_module_loader_result_ERR_MISSING_DEF = 6, + } kind; + PyObject *exc; + } _err; }; -extern void _Py_ext_module_loader_result_apply_error( +extern void _Py_ext_module_loader_result_clear( struct _Py_ext_module_loader_result *res); +extern void _Py_ext_module_loader_result_apply_error( + struct _Py_ext_module_loader_result *res, + const char *name); +/* The module init function. */ +typedef PyObject *(*PyModInitFunction)(void); extern PyModInitFunction _PyImport_GetModInitFunc( struct _Py_ext_module_loader_info *info, FILE *fp); diff --git a/Python/import.c b/Python/import.c index 8d50b8285989dc..57426fee87bc49 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1377,14 +1377,21 @@ import_find_extension(PyThreadState *tstate, } struct _Py_ext_module_loader_result res; if (_PyImport_RunModInitFunc(def->m_base.m_init, info, &res) < 0) { - _Py_ext_module_loader_result_apply_error(&res); + _Py_ext_module_loader_result_apply_error(&res, name_buf); + _Py_ext_module_loader_result_clear(&res); return NULL; } - assert(!PyErr_Occurred()); + assert(!PyErr_Occurred() && res.err == NULL); assert(res.kind == _Py_ext_module_loader_result_SINGLEPHASE); mod = res.module; + /* Tchnically, the init function could return a different module def. + * Then we would probably need to update the global cache. + * However, we don't expect anyone to change the def. */ + assert(res.def == def); + _Py_ext_module_loader_result_clear(&res); // XXX __file__ doesn't get set! + if (PyObject_SetItem(modules, info->name, mod) == -1) { Py_DECREF(mod); return NULL; @@ -1411,15 +1418,16 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, { PyObject *mod = NULL; PyModuleDef *def = NULL; + const char *name_buf = PyBytes_AS_STRING(info->name_encoded); struct _Py_ext_module_loader_result res; if (_PyImport_RunModInitFunc(p0, info, &res) < 0) { /* We discard res.def. */ assert(res.module == NULL); - _Py_ext_module_loader_result_apply_error(&res); + _Py_ext_module_loader_result_apply_error(&res, name_buf); goto finally; } - assert(!PyErr_Occurred()); + assert(!PyErr_Occurred() && res.err == NULL); mod = res.module; res.module = NULL; @@ -1439,7 +1447,6 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, assert(is_singlephase(def)); assert(PyModule_Check(mod)); - const char *name_buf = PyBytes_AS_STRING(info->name_encoded); if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { Py_CLEAR(mod); goto finally; @@ -1448,13 +1455,14 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, /* Remember pointer to module init function. */ def->m_base.m_init = p0; + /* Remember the filename as the __file__ attribute */ if (info->filename != NULL) { - /* Remember the filename as the __file__ attribute */ if (PyModule_AddObjectRef(mod, "__file__", info->filename) < 0) { PyErr_Clear(); /* Not important enough to report */ } } + /* Update global import state. */ struct singlephase_global_update singlephase = {0}; // gh-88216: Extensions and def->m_base.m_copy can be updated // when the extension module doesn't support sub-interpreters. @@ -1471,6 +1479,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, goto finally; } + /* Update per-interpreter import state. */ PyObject *modules = get_modules_dict(tstate, true); if (finish_singlephase_extension( tstate, mod, def, info->name, modules) < 0) @@ -1481,6 +1490,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, } finally: + _Py_ext_module_loader_result_clear(&res); return mod; } diff --git a/Python/importdl.c b/Python/importdl.c index f6b0ddf1f78a4d..f9308a6b62f764 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -28,6 +28,11 @@ extern dl_funcptr _PyImport_FindSharedFuncptr(const char *prefix, const char *pathname, FILE *fp); #endif + +/***********************************/ +/* module info to use when loading */ +/***********************************/ + static const char * const ascii_only_prefix = "PyInit"; static const char * const nonascii_prefix = "PyInitU"; @@ -205,24 +210,142 @@ _Py_ext_module_loader_info_init_from_spec( } +/********************************/ +/* module init function results */ +/********************************/ + +void +_Py_ext_module_loader_result_clear(struct _Py_ext_module_loader_result *res) +{ + *res = (struct _Py_ext_module_loader_result){0}; +} + +static void +_Py_ext_module_loader_result_set_error( + struct _Py_ext_module_loader_result *res, + enum _Py_ext_module_loader_result_error_kind kind) +{ +#ifndef NDEBUG + switch (kind) { + case _Py_ext_module_loader_result_EXCEPTION: /* fall through */ + case _Py_ext_module_loader_result_ERR_UNREPORTED_EXC: + assert(PyErr_Occurred()); + break; + case _Py_ext_module_loader_result_ERR_MISSING: /* fall through */ + case _Py_ext_module_loader_result_ERR_UNINITIALIZED: /* fall through */ + case _Py_ext_module_loader_result_ERR_NONASCII_NOT_MULTIPHASE: /* fall through */ + case _Py_ext_module_loader_result_ERR_NOT_MODULE: /* fall through */ + case _Py_ext_module_loader_result_ERR_MISSING_DEF: + assert(!PyErr_Occurred()); + break; + default: + /* We added a new error kind but forgot to add it to this switch. */ + assert(0); + } +#endif + + assert(res->err == NULL && res->_err.exc == NULL); + res->err = &res->_err; + *res->err = (struct _Py_ext_module_loader_result_error){ + .kind=kind, + .exc=PyErr_GetRaisedException(), + }; + + /* For some kinds, we also set/check res->kind. */ + switch (kind) { + case _Py_ext_module_loader_result_ERR_UNINITIALIZED: + assert(res->kind == _Py_ext_module_loader_result_UNKNOWN); + res->kind = _Py_ext_module_loader_result_INVALID; + break; + /* None of the rest affect the result kind. */ + case _Py_ext_module_loader_result_EXCEPTION: /* fall through */ + case _Py_ext_module_loader_result_ERR_MISSING: /* fall through */ + case _Py_ext_module_loader_result_ERR_UNREPORTED_EXC: /* fall through */ + case _Py_ext_module_loader_result_ERR_NONASCII_NOT_MULTIPHASE: /* fall through */ + case _Py_ext_module_loader_result_ERR_NOT_MODULE: /* fall through */ + case _Py_ext_module_loader_result_ERR_MISSING_DEF: + break; + default: + /* We added a new error kind but forgot to add it to this switch. */ + assert(0); + } +} + void _Py_ext_module_loader_result_apply_error( - struct _Py_ext_module_loader_result *res) + struct _Py_ext_module_loader_result *res, + const char *name) { - if (res->err[0] != '\0') { - if (PyErr_Occurred()) { - _PyErr_FormatFromCause(PyExc_SystemError, res->err); - } - else { - PyErr_SetString(PyExc_SystemError, res->err); + assert(!PyErr_Occurred()); + struct _Py_ext_module_loader_result_error *err = res->err; + assert(err != NULL && err == &res->_err); + +#ifndef NDEBUG + switch (err->kind) { + case _Py_ext_module_loader_result_EXCEPTION: /* fall through */ + case _Py_ext_module_loader_result_ERR_UNREPORTED_EXC: + assert(err->exc != NULL); + break; + case _Py_ext_module_loader_result_ERR_MISSING: /* fall through */ + case _Py_ext_module_loader_result_ERR_UNINITIALIZED: /* fall through */ + case _Py_ext_module_loader_result_ERR_NONASCII_NOT_MULTIPHASE: /* fall through */ + case _Py_ext_module_loader_result_ERR_NOT_MODULE: /* fall through */ + case _Py_ext_module_loader_result_ERR_MISSING_DEF: + assert(err->exc == NULL); + break; + default: + /* We added a new error kind but forgot to add it to this switch. */ + assert(0); + } +#endif + + const char *msg = NULL; + switch (err->kind) { + case _Py_ext_module_loader_result_EXCEPTION: + break; + case _Py_ext_module_loader_result_ERR_MISSING: + msg = "initialization of %s failed without raising an exception"; + break; + case _Py_ext_module_loader_result_ERR_UNREPORTED_EXC: + msg = "initialization of %s raised unreported exception"; + break; + case _Py_ext_module_loader_result_ERR_UNINITIALIZED: + msg = "init function of %s returned uninitialized object"; + break; + case _Py_ext_module_loader_result_ERR_NONASCII_NOT_MULTIPHASE: + msg = "initialization of %s did not return PyModuleDef"; + break; + case _Py_ext_module_loader_result_ERR_NOT_MODULE: + msg = "initialization of %s did not return an extension module"; + break; + case _Py_ext_module_loader_result_ERR_MISSING_DEF: + msg = "initialization of %s did not return a valid extension module"; + break; + default: + /* We added a new error kind but forgot to add it to this switch. */ + assert(0); + PyErr_Format(PyExc_SystemError, + "loading %s failed due to init function", name); + return; + } + + if (err->exc != NULL) { + PyErr_SetRaisedException(err->exc); + if (msg != NULL) { + _PyErr_FormatFromCause(PyExc_SystemError, msg, name); } } else { - assert(PyErr_Occurred()); + assert(msg != NULL); + PyErr_Format(PyExc_SystemError, msg, name); } } +/********************************************/ +/* getting/running the module init function */ +/********************************************/ + PyModInitFunction _PyImport_GetModInitFunc(struct _Py_ext_module_loader_info *info, FILE *fp) @@ -266,7 +389,6 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, struct _Py_ext_module_loader_result res = { .kind=_Py_ext_module_loader_result_UNKNOWN, }; - const char *name_buf = PyBytes_AS_STRING(info->name_encoded); /* Call the module init function. */ @@ -277,27 +399,19 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, /* Validate the result (and populate "res". */ -#ifdef NDEBUG -# define SET_ERROR(...) \ - (void)snprintf(res.err, Py_ARRAY_LENGTH(res.err), __VA_ARGS__) -#else -# define SET_ERROR(...) \ - do { \ - int n = snprintf(res.err, Py_ARRAY_LENGTH(res.err), __VA_ARGS__); \ - assert(n < Py_ARRAY_LENGTH(res.err)); \ - } while (0) -#endif - if (m == NULL) { - if (!PyErr_Occurred()) { - SET_ERROR( - "initialization of %s failed without raising an exception", - name_buf); + if (PyErr_Occurred()) { + _Py_ext_module_loader_result_set_error( + &res, _Py_ext_module_loader_result_EXCEPTION); + } + else { + _Py_ext_module_loader_result_set_error( + &res, _Py_ext_module_loader_result_ERR_MISSING); } goto error; } else if (PyErr_Occurred()) { - SET_ERROR("initialization of %s raised unreported exception", - name_buf); + _Py_ext_module_loader_result_set_error( + &res, _Py_ext_module_loader_result_ERR_UNREPORTED_EXC); /* We would probably be correct to decref m here, * but we weren't doing so before, * so we stick with doing nothing. */ @@ -309,9 +423,8 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, /* This can happen when a PyModuleDef is returned without calling * PyModuleDef_Init on it */ - res.kind = _Py_ext_module_loader_result_INVALID; - SET_ERROR("init function of %s returned uninitialized object", - name_buf); + _Py_ext_module_loader_result_set_error( + &res, _Py_ext_module_loader_result_ERR_UNINITIALIZED); /* Likewise, decref'ing here makes sense. However, the original * code has a note about "prevent segfault in DECREF", * so we play it safe and leave it alone. */ @@ -326,10 +439,11 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, /* Run PyModule_FromDefAndSpec() to finish loading the module. */ } else if (info->hook_prefix == nonascii_prefix) { - /* It should have been multi-phase init? */ + /* Non-ASCII is only supported for multi-phase init. */ + res.kind = _Py_ext_module_loader_result_MULTIPHASE; /* Don't allow legacy init for non-ASCII module names. */ - SET_ERROR("initialization of %s did not return PyModuleDef", - name_buf); + _Py_ext_module_loader_result_set_error( + &res, _Py_ext_module_loader_result_ERR_NONASCII_NOT_MULTIPHASE); goto error; } else { @@ -338,30 +452,31 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, res.module = m; if (!PyModule_Check(m)) { - SET_ERROR("initialization of %s did not return an extension " - "module", name_buf); + _Py_ext_module_loader_result_set_error( + &res, _Py_ext_module_loader_result_ERR_NOT_MODULE); goto error; } res.def = _PyModule_GetDef(m); if (res.def == NULL) { PyErr_Clear(); - SET_ERROR("initialization of %s did not return a valid extension " - "module", name_buf); + _Py_ext_module_loader_result_set_error( + &res, _Py_ext_module_loader_result_ERR_MISSING_DEF); goto error; } } -#undef SET_ERROR - assert(!PyErr_Occurred()); + assert(!PyErr_Occurred() && res.err == NULL); *p_res = res; return 0; error: - assert(PyErr_Occurred() || res.err[0] != '\0'); + assert(!PyErr_Occurred()); + assert(res.err != NULL); Py_CLEAR(res.module); res.def = NULL; *p_res = res; + p_res->err = &p_res->_err; return -1; } From 8efc29fce9782db5bfdfb1238c38dc1a4594a147 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 29 Apr 2024 09:55:59 -0600 Subject: [PATCH 09/23] _Py_ext_module_loader_result_kind -> _Py_ext_module_kind --- Include/internal/pycore_importdl.h | 15 +++++++++------ Python/import.c | 18 +++++++++--------- Python/importdl.c | 12 ++++++------ 3 files changed, 24 insertions(+), 21 deletions(-) diff --git a/Include/internal/pycore_importdl.h b/Include/internal/pycore_importdl.h index 54c4279a8dd12d..9c70c7fb6f944c 100644 --- a/Include/internal/pycore_importdl.h +++ b/Include/internal/pycore_importdl.h @@ -15,6 +15,14 @@ extern "C" { extern const char *_PyImport_DynLoadFiletab[]; +enum _Py_ext_module_kind { + _Py_ext_module_kind_UNKNOWN = 0, + _Py_ext_module_kind_SINGLEPHASE = 1, + _Py_ext_module_kind_MULTIPHASE = 2, + _Py_ext_module_kind_INVALID = 3, +}; + + /* Input for loading an extension module. */ struct _Py_ext_module_loader_info { PyObject *filename; @@ -46,12 +54,7 @@ extern int _Py_ext_module_loader_info_init_from_spec( struct _Py_ext_module_loader_result { PyModuleDef *def; PyObject *module; - enum _Py_ext_module_loader_result_kind { - _Py_ext_module_loader_result_UNKNOWN = 0, - _Py_ext_module_loader_result_SINGLEPHASE = 1, - _Py_ext_module_loader_result_MULTIPHASE = 2, - _Py_ext_module_loader_result_INVALID = 3, - } kind; + enum _Py_ext_module_kind kind; struct _Py_ext_module_loader_result_error *err; struct _Py_ext_module_loader_result_error { enum _Py_ext_module_loader_result_error_kind { diff --git a/Python/import.c b/Python/import.c index 57426fee87bc49..9b69a5d835aa98 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1193,20 +1193,20 @@ is_core_module(PyInterpreterState *interp, PyObject *name, PyObject *path) } #ifndef NDEBUG -static enum _Py_ext_module_loader_result_kind +static enum _Py_ext_module_kind get_extension_kind(PyModuleDef *def) { - enum _Py_ext_module_loader_result_kind kind; + enum _Py_ext_module_kind kind; if (def == NULL) { /* It must be a module created by reload_singlephase_extension() * from m_copy. Ideally we'd do away with this case. */ - kind = _Py_ext_module_loader_result_SINGLEPHASE; + kind = _Py_ext_module_kind_SINGLEPHASE; } else if (def->m_slots == NULL) { - kind = _Py_ext_module_loader_result_SINGLEPHASE; + kind = _Py_ext_module_kind_SINGLEPHASE; } else { - kind = _Py_ext_module_loader_result_MULTIPHASE; + kind = _Py_ext_module_kind_MULTIPHASE; } return kind; } @@ -1214,7 +1214,7 @@ get_extension_kind(PyModuleDef *def) static bool is_singlephase(PyModuleDef *def) { - return get_extension_kind(def) == _Py_ext_module_loader_result_SINGLEPHASE; + return get_extension_kind(def) == _Py_ext_module_kind_SINGLEPHASE; } #endif @@ -1382,7 +1382,7 @@ import_find_extension(PyThreadState *tstate, return NULL; } assert(!PyErr_Occurred() && res.err == NULL); - assert(res.kind == _Py_ext_module_loader_result_SINGLEPHASE); + assert(res.kind == _Py_ext_module_kind_SINGLEPHASE); mod = res.module; /* Tchnically, the init function could return a different module def. * Then we would probably need to update the global cache. @@ -1434,7 +1434,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, def = res.def; assert(def != NULL); - if (res.kind == _Py_ext_module_loader_result_MULTIPHASE) { + if (res.kind == _Py_ext_module_kind_MULTIPHASE) { //assert(!is_singlephase(def)); assert(mod == NULL); mod = PyModule_FromDefAndSpec(def, spec); @@ -1443,7 +1443,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, } } else { - assert(res.kind == _Py_ext_module_loader_result_SINGLEPHASE); + assert(res.kind == _Py_ext_module_kind_SINGLEPHASE); assert(is_singlephase(def)); assert(PyModule_Check(mod)); diff --git a/Python/importdl.c b/Python/importdl.c index f9308a6b62f764..38a24c1b35844a 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -254,8 +254,8 @@ _Py_ext_module_loader_result_set_error( /* For some kinds, we also set/check res->kind. */ switch (kind) { case _Py_ext_module_loader_result_ERR_UNINITIALIZED: - assert(res->kind == _Py_ext_module_loader_result_UNKNOWN); - res->kind = _Py_ext_module_loader_result_INVALID; + assert(res->kind == _Py_ext_module_kind_UNKNOWN); + res->kind = _Py_ext_module_kind_INVALID; break; /* None of the rest affect the result kind. */ case _Py_ext_module_loader_result_EXCEPTION: /* fall through */ @@ -387,7 +387,7 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, struct _Py_ext_module_loader_result *p_res) { struct _Py_ext_module_loader_result res = { - .kind=_Py_ext_module_loader_result_UNKNOWN, + .kind=_Py_ext_module_kind_UNKNOWN, }; /* Call the module init function. */ @@ -434,13 +434,13 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, if (PyObject_TypeCheck(m, &PyModuleDef_Type)) { /* multi-phase init */ - res.kind = _Py_ext_module_loader_result_MULTIPHASE; + res.kind = _Py_ext_module_kind_MULTIPHASE; res.def = (PyModuleDef *)m; /* Run PyModule_FromDefAndSpec() to finish loading the module. */ } else if (info->hook_prefix == nonascii_prefix) { /* Non-ASCII is only supported for multi-phase init. */ - res.kind = _Py_ext_module_loader_result_MULTIPHASE; + res.kind = _Py_ext_module_kind_MULTIPHASE; /* Don't allow legacy init for non-ASCII module names. */ _Py_ext_module_loader_result_set_error( &res, _Py_ext_module_loader_result_ERR_NONASCII_NOT_MULTIPHASE); @@ -448,7 +448,7 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, } else { /* single-phase init (legacy) */ - res.kind = _Py_ext_module_loader_result_SINGLEPHASE; + res.kind = _Py_ext_module_kind_SINGLEPHASE; res.module = m; if (!PyModule_Check(m)) { From 0359bae00185473ee67b024c06da55ae1d194f4d Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 29 Apr 2024 09:57:58 -0600 Subject: [PATCH 10/23] Fix is_singlephase(). --- Python/import.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Python/import.c b/Python/import.c index 9b69a5d835aa98..3f53fcd537ae0e 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1202,11 +1202,11 @@ get_extension_kind(PyModuleDef *def) * from m_copy. Ideally we'd do away with this case. */ kind = _Py_ext_module_kind_SINGLEPHASE; } - else if (def->m_slots == NULL) { - kind = _Py_ext_module_kind_SINGLEPHASE; + else if (def->m_slots != NULL) { + kind = _Py_ext_module_kind_MULTIPHASE; } else { - kind = _Py_ext_module_kind_MULTIPHASE; + kind = _Py_ext_module_kind_SINGLEPHASE; } return kind; } From 87e6bbc40ce874b43f7298abb2e9d90128fb952c Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 29 Apr 2024 12:14:52 -0600 Subject: [PATCH 11/23] Return UNKNOWN from get_extension_kind(). --- Python/import.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/Python/import.c b/Python/import.c index 3f53fcd537ae0e..1f8d9f065cb3e4 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1206,15 +1206,19 @@ get_extension_kind(PyModuleDef *def) kind = _Py_ext_module_kind_MULTIPHASE; } else { - kind = _Py_ext_module_kind_SINGLEPHASE; + kind = _Py_ext_module_kind_UNKNOWN; } return kind; } static bool -is_singlephase(PyModuleDef *def) +is_singlephase(PyModuleDef *def, bool default_singlephase) { - return get_extension_kind(def) == _Py_ext_module_kind_SINGLEPHASE; + enum _Py_ext_module_kind kind = get_extension_kind(def); + if (kind == _Py_ext_module_kind_SINGLEPHASE) { + return true; + } + return default_singlephase && kind == _Py_ext_module_kind_UNKNOWN; } #endif @@ -1324,7 +1328,7 @@ import_find_extension(PyThreadState *tstate, if (def == NULL) { return NULL; } - assert(is_singlephase(def)); + assert(is_singlephase(def, true)); /* It may have been successfully imported previously in an interpreter that allows legacy modules @@ -1435,7 +1439,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, assert(def != NULL); if (res.kind == _Py_ext_module_kind_MULTIPHASE) { - //assert(!is_singlephase(def)); + assert(!is_singlephase(def, false)); assert(mod == NULL); mod = PyModule_FromDefAndSpec(def, spec); if (mod == NULL) { @@ -1444,7 +1448,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, } else { assert(res.kind == _Py_ext_module_kind_SINGLEPHASE); - assert(is_singlephase(def)); + assert(is_singlephase(def, true)); assert(PyModule_Check(mod)); if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { @@ -1554,7 +1558,7 @@ _PyImport_FixupBuiltin(PyThreadState *tstate, PyObject *mod, const char *name, * module state, but we also don't populate def->m_base.m_copy * for them. */ assert(is_core_module(tstate->interp, nameobj, nameobj)); - assert(is_singlephase(def)); + assert(is_singlephase(def, true)); assert(def->m_size == -1); assert(def->m_base.m_copy == NULL); @@ -1608,7 +1612,7 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) PyObject *mod = import_find_extension(tstate, &info); if (mod != NULL) { assert(!_PyErr_Occurred(tstate)); - assert(is_singlephase(_PyModule_GetDef(mod))); + assert(is_singlephase(_PyModule_GetDef(mod), true)); goto finally; } else if (_PyErr_Occurred(tstate)) { @@ -3962,7 +3966,7 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) mod = import_find_extension(tstate, &info); if (mod != NULL) { assert(!_PyErr_Occurred(tstate)); - assert(is_singlephase(_PyModule_GetDef(mod))); + assert(is_singlephase(_PyModule_GetDef(mod), true)); goto finally; } else if (_PyErr_Occurred(tstate)) { From 8622c28891606744241f09e046b30a4d5e6c9ab6 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 29 Apr 2024 12:19:37 -0600 Subject: [PATCH 12/23] Add a comment. --- Python/import.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Python/import.c b/Python/import.c index 1f8d9f065cb3e4..59b6560290b8a1 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1206,6 +1206,8 @@ get_extension_kind(PyModuleDef *def) kind = _Py_ext_module_kind_MULTIPHASE; } else { + // This is probably single-phase init, but a multi-phase + // module *can* have NULL m_slots. kind = _Py_ext_module_kind_UNKNOWN; } return kind; From 7c7e654cf3c13dfca880ee347a7aac50d6bc2661 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 29 Apr 2024 12:39:33 -0600 Subject: [PATCH 13/23] Set m_init in update_global_state_for_extension(). --- Python/import.c | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/Python/import.c b/Python/import.c index 59b6560290b8a1..b7f761f8251099 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1227,6 +1227,7 @@ is_singlephase(PyModuleDef *def, bool default_singlephase) struct singlephase_global_update { PyObject *m_dict; + PyModInitFunction m_init; }; static int @@ -1240,10 +1241,22 @@ update_global_state_for_extension(PyThreadState *tstate, assert(def->m_base.m_copy == NULL); } else { - assert(def->m_base.m_init != NULL - || is_core_module(tstate->interp, name, path)); - if (singlephase->m_dict == NULL) { + if (singlephase->m_init != NULL) { + assert(singlephase->m_dict == NULL); assert(def->m_base.m_copy == NULL); + assert(def->m_size >= 0); + /* Remember pointer to module init function. */ + // XXX Two modules should not share the same def->m_base. + //assert(def->m_base.m_init == NULL + // || def->m_base.m_init == singlephase->m_init); + def->m_base.m_init = singlephase->m_init; + } + else if (singlephase->m_dict == NULL) { + /* It must be a core builtin module. */ + assert(is_core_module(tstate->interp, name, path)); + assert(def->m_size == -1); + assert(def->m_base.m_copy == NULL); + assert(def->m_base.m_init == NULL); } else { assert(PyDict_Check(singlephase->m_dict)); @@ -1422,6 +1435,9 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, struct _Py_ext_module_loader_info *info, PyObject *spec, PyObject *modules) { + /* Core modules go through _PyImport_FixupBuiltin(). */ + assert(!is_core_module(tstate->interp, info->name, info->path)); + PyObject *mod = NULL; PyModuleDef *def = NULL; const char *name_buf = PyBytes_AS_STRING(info->name_encoded); @@ -1458,9 +1474,6 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, goto finally; } - /* Remember pointer to module init function. */ - def->m_base.m_init = p0; - /* Remember the filename as the __file__ attribute */ if (info->filename != NULL) { if (PyModule_AddObjectRef(mod, "__file__", info->filename) < 0) { @@ -1472,12 +1485,17 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, struct singlephase_global_update singlephase = {0}; // gh-88216: Extensions and def->m_base.m_copy can be updated // when the extension module doesn't support sub-interpreters. - if (def->m_size == -1 - && !is_core_module(tstate->interp, info->name, info->path)) - { + if (def->m_size == -1) { + /* We will reload from m_copy. */ + assert(def->m_base.m_init == NULL); singlephase.m_dict = PyModule_GetDict(mod); assert(singlephase.m_dict != NULL); } + else { + /* We will reload via the init function. */ + assert(def->m_size >= 0); + singlephase.m_init = p0; + } if (update_global_state_for_extension( tstate, info->path, info->name, def, &singlephase) < 0) { From 44061924b461a54669515135cb0fe7078129f54f Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 29 Apr 2024 12:47:18 -0600 Subject: [PATCH 14/23] Expand the cases in get_extension_kind(). --- Python/import.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Python/import.c b/Python/import.c index b7f761f8251099..07bd4976a53c10 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1194,7 +1194,7 @@ is_core_module(PyInterpreterState *interp, PyObject *name, PyObject *path) #ifndef NDEBUG static enum _Py_ext_module_kind -get_extension_kind(PyModuleDef *def) +get_extension_kind(PyModuleDef *def, bool check_size) { enum _Py_ext_module_kind kind; if (def == NULL) { @@ -1205,6 +1205,12 @@ get_extension_kind(PyModuleDef *def) else if (def->m_slots != NULL) { kind = _Py_ext_module_kind_MULTIPHASE; } + else if (check_size && def->m_size == -1) { + kind = _Py_ext_module_kind_SINGLEPHASE; + } + else if (def->m_base.m_init != NULL) { + kind = _Py_ext_module_kind_SINGLEPHASE; + } else { // This is probably single-phase init, but a multi-phase // module *can* have NULL m_slots. @@ -1216,7 +1222,7 @@ get_extension_kind(PyModuleDef *def) static bool is_singlephase(PyModuleDef *def, bool default_singlephase) { - enum _Py_ext_module_kind kind = get_extension_kind(def); + enum _Py_ext_module_kind kind = get_extension_kind(def, default_singlephase); if (kind == _Py_ext_module_kind_SINGLEPHASE) { return true; } From ae3e770263a3dea5d4701a9c6177e8db555615b4 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 29 Apr 2024 13:38:26 -0600 Subject: [PATCH 15/23] Add check_multiphase_preinit(). --- Python/import.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/Python/import.c b/Python/import.c index 07bd4976a53c10..5dcdfa92520a23 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1194,7 +1194,7 @@ is_core_module(PyInterpreterState *interp, PyObject *name, PyObject *path) #ifndef NDEBUG static enum _Py_ext_module_kind -get_extension_kind(PyModuleDef *def, bool check_size) +_get_extension_kind(PyModuleDef *def, bool check_size) { enum _Py_ext_module_kind kind; if (def == NULL) { @@ -1220,13 +1220,22 @@ get_extension_kind(PyModuleDef *def, bool check_size) } static bool -is_singlephase(PyModuleDef *def, bool default_singlephase) +check_multiphase_preinit(PyModuleDef *def) { - enum _Py_ext_module_kind kind = get_extension_kind(def, default_singlephase); - if (kind == _Py_ext_module_kind_SINGLEPHASE) { - return true; - } - return default_singlephase && kind == _Py_ext_module_kind_UNKNOWN; + /* PyModule_FromDefAndSpec() checks m_size so we skip m_size + * if the module hasn't been fully initialized yet. */ + enum _Py_ext_module_kind kind = _get_extension_kind(def, false); + return kind == _Py_ext_module_kind_MULTIPHASE + /* m_slots can be NULL. */ + || kind == _Py_ext_module_kind_UNKNOWN; +} + +static bool +check_singlephase(PyModuleDef *def) +{ + enum _Py_ext_module_kind kind = _get_extension_kind(def, true); + return kind == _Py_ext_module_kind_SINGLEPHASE + || kind == _Py_ext_module_kind_UNKNOWN; } #endif @@ -1349,7 +1358,7 @@ import_find_extension(PyThreadState *tstate, if (def == NULL) { return NULL; } - assert(is_singlephase(def, true)); + assert(check_singlephase(def)); /* It may have been successfully imported previously in an interpreter that allows legacy modules @@ -1463,7 +1472,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, assert(def != NULL); if (res.kind == _Py_ext_module_kind_MULTIPHASE) { - assert(!is_singlephase(def, false)); + assert(check_multiphase_preinit(def)); assert(mod == NULL); mod = PyModule_FromDefAndSpec(def, spec); if (mod == NULL) { @@ -1472,7 +1481,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, } else { assert(res.kind == _Py_ext_module_kind_SINGLEPHASE); - assert(is_singlephase(def, true)); + assert(check_singlephase(def)); assert(PyModule_Check(mod)); if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { @@ -1584,7 +1593,7 @@ _PyImport_FixupBuiltin(PyThreadState *tstate, PyObject *mod, const char *name, * module state, but we also don't populate def->m_base.m_copy * for them. */ assert(is_core_module(tstate->interp, nameobj, nameobj)); - assert(is_singlephase(def, true)); + assert(check_singlephase(def)); assert(def->m_size == -1); assert(def->m_base.m_copy == NULL); @@ -1638,7 +1647,7 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) PyObject *mod = import_find_extension(tstate, &info); if (mod != NULL) { assert(!_PyErr_Occurred(tstate)); - assert(is_singlephase(_PyModule_GetDef(mod), true)); + assert(check_singlephase(_PyModule_GetDef(mod))); goto finally; } else if (_PyErr_Occurred(tstate)) { @@ -3992,7 +4001,7 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) mod = import_find_extension(tstate, &info); if (mod != NULL) { assert(!_PyErr_Occurred(tstate)); - assert(is_singlephase(_PyModule_GetDef(mod), true)); + assert(check_singlephase(_PyModule_GetDef(mod))); goto finally; } else if (_PyErr_Occurred(tstate)) { From e94a082c22dc8d991575d03abfe3a03d8863a8df Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 1 May 2024 13:20:35 -0600 Subject: [PATCH 16/23] Add a _Py_ext_module_kind typedef. --- Include/internal/pycore_importdl.h | 6 +++--- Python/import.c | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Include/internal/pycore_importdl.h b/Include/internal/pycore_importdl.h index 9c70c7fb6f944c..b0af28da34e087 100644 --- a/Include/internal/pycore_importdl.h +++ b/Include/internal/pycore_importdl.h @@ -15,12 +15,12 @@ extern "C" { extern const char *_PyImport_DynLoadFiletab[]; -enum _Py_ext_module_kind { +typedef enum ext_module_kind { _Py_ext_module_kind_UNKNOWN = 0, _Py_ext_module_kind_SINGLEPHASE = 1, _Py_ext_module_kind_MULTIPHASE = 2, _Py_ext_module_kind_INVALID = 3, -}; +} _Py_ext_module_kind; /* Input for loading an extension module. */ @@ -54,7 +54,7 @@ extern int _Py_ext_module_loader_info_init_from_spec( struct _Py_ext_module_loader_result { PyModuleDef *def; PyObject *module; - enum _Py_ext_module_kind kind; + _Py_ext_module_kind kind; struct _Py_ext_module_loader_result_error *err; struct _Py_ext_module_loader_result_error { enum _Py_ext_module_loader_result_error_kind { diff --git a/Python/import.c b/Python/import.c index 5dcdfa92520a23..da16278631e803 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1193,10 +1193,10 @@ is_core_module(PyInterpreterState *interp, PyObject *name, PyObject *path) } #ifndef NDEBUG -static enum _Py_ext_module_kind +static _Py_ext_module_kind _get_extension_kind(PyModuleDef *def, bool check_size) { - enum _Py_ext_module_kind kind; + _Py_ext_module_kind kind; if (def == NULL) { /* It must be a module created by reload_singlephase_extension() * from m_copy. Ideally we'd do away with this case. */ @@ -1224,7 +1224,7 @@ check_multiphase_preinit(PyModuleDef *def) { /* PyModule_FromDefAndSpec() checks m_size so we skip m_size * if the module hasn't been fully initialized yet. */ - enum _Py_ext_module_kind kind = _get_extension_kind(def, false); + _Py_ext_module_kind kind = _get_extension_kind(def, false); return kind == _Py_ext_module_kind_MULTIPHASE /* m_slots can be NULL. */ || kind == _Py_ext_module_kind_UNKNOWN; @@ -1233,7 +1233,7 @@ check_multiphase_preinit(PyModuleDef *def) static bool check_singlephase(PyModuleDef *def) { - enum _Py_ext_module_kind kind = _get_extension_kind(def, true); + _Py_ext_module_kind kind = _get_extension_kind(def, true); return kind == _Py_ext_module_kind_SINGLEPHASE || kind == _Py_ext_module_kind_UNKNOWN; } From 4e1f308432ffe0414075196333d1d8cf34a892aa Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 1 May 2024 13:42:04 -0600 Subject: [PATCH 17/23] check_singlephase() -> assert_singlephase() --- Python/import.c | 55 ++++++++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/Python/import.c b/Python/import.c index da16278631e803..4bb0d39fb857eb 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1219,24 +1219,31 @@ _get_extension_kind(PyModuleDef *def, bool check_size) return kind; } -static bool -check_multiphase_preinit(PyModuleDef *def) -{ - /* PyModule_FromDefAndSpec() checks m_size so we skip m_size - * if the module hasn't been fully initialized yet. */ - _Py_ext_module_kind kind = _get_extension_kind(def, false); - return kind == _Py_ext_module_kind_MULTIPHASE - /* m_slots can be NULL. */ - || kind == _Py_ext_module_kind_UNKNOWN; -} - -static bool -check_singlephase(PyModuleDef *def) -{ - _Py_ext_module_kind kind = _get_extension_kind(def, true); - return kind == _Py_ext_module_kind_SINGLEPHASE - || kind == _Py_ext_module_kind_UNKNOWN; -} +/* The module might not be fully initialized yet + * and PyModule_FromDefAndSpec() checks m_size + * so we skip m_size. */ +#define assert_multiphase_def(def) \ + do { \ + _Py_ext_module_kind kind = _get_extension_kind(def, false); \ + assert(kind == _Py_ext_module_kind_MULTIPHASE \ + /* m_slots can be NULL. */ \ + || kind == _Py_ext_module_kind_UNKNOWN); \ + } while (0) + +#define assert_singlephase_def(def) \ + do { \ + _Py_ext_module_kind kind = _get_extension_kind(def, true); \ + assert(kind == _Py_ext_module_kind_SINGLEPHASE \ + || kind == _Py_ext_module_kind_UNKNOWN); \ + } while (0) + +#define assert_singlephase(def) \ + assert_singlephase_def(def) + +#else /* defined(NDEBUG) */ +#define assert_multiphase_def(def) +#define assert_singlephase_def(def) +#define assert_singlephase(def) #endif @@ -1358,7 +1365,7 @@ import_find_extension(PyThreadState *tstate, if (def == NULL) { return NULL; } - assert(check_singlephase(def)); + assert_singlephase(def); /* It may have been successfully imported previously in an interpreter that allows legacy modules @@ -1472,7 +1479,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, assert(def != NULL); if (res.kind == _Py_ext_module_kind_MULTIPHASE) { - assert(check_multiphase_preinit(def)); + assert_multiphase_def(def); assert(mod == NULL); mod = PyModule_FromDefAndSpec(def, spec); if (mod == NULL) { @@ -1481,7 +1488,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, } else { assert(res.kind == _Py_ext_module_kind_SINGLEPHASE); - assert(check_singlephase(def)); + assert_singlephase_def(def); assert(PyModule_Check(mod)); if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { @@ -1593,7 +1600,7 @@ _PyImport_FixupBuiltin(PyThreadState *tstate, PyObject *mod, const char *name, * module state, but we also don't populate def->m_base.m_copy * for them. */ assert(is_core_module(tstate->interp, nameobj, nameobj)); - assert(check_singlephase(def)); + assert_singlephase_def(def); assert(def->m_size == -1); assert(def->m_base.m_copy == NULL); @@ -1647,7 +1654,7 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) PyObject *mod = import_find_extension(tstate, &info); if (mod != NULL) { assert(!_PyErr_Occurred(tstate)); - assert(check_singlephase(_PyModule_GetDef(mod))); + assert_singlephase(_PyModule_GetDef(mod)); goto finally; } else if (_PyErr_Occurred(tstate)) { @@ -4001,7 +4008,7 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) mod = import_find_extension(tstate, &info); if (mod != NULL) { assert(!_PyErr_Occurred(tstate)); - assert(check_singlephase(_PyModule_GetDef(mod))); + assert_singlephase(_PyModule_GetDef(mod)); goto finally; } else if (_PyErr_Occurred(tstate)) { From aa1edc9d9ed9af7b6fbb9e663b1eca5d75428b51 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 1 May 2024 13:58:13 -0600 Subject: [PATCH 18/23] Split up an assert. --- Python/import.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Python/import.c b/Python/import.c index 4bb0d39fb857eb..242f07958cd544 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1422,7 +1422,8 @@ import_find_extension(PyThreadState *tstate, _Py_ext_module_loader_result_clear(&res); return NULL; } - assert(!PyErr_Occurred() && res.err == NULL); + assert(!PyErr_Occurred()); + assert(res.err == NULL); assert(res.kind == _Py_ext_module_kind_SINGLEPHASE); mod = res.module; /* Tchnically, the init function could return a different module def. From cd365ec4905db14b1a2b6747215a8a5455db9ba6 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 1 May 2024 14:18:48 -0600 Subject: [PATCH 19/23] Set __file__ in the reload case. --- Python/import.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Python/import.c b/Python/import.c index 242f07958cd544..53482628f1af0a 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1432,7 +1432,12 @@ import_find_extension(PyThreadState *tstate, assert(res.def == def); _Py_ext_module_loader_result_clear(&res); - // XXX __file__ doesn't get set! + /* Remember the filename as the __file__ attribute */ + if (info->filename != NULL) { + if (PyModule_AddObjectRef(mod, "__file__", info->filename) < 0) { + PyErr_Clear(); /* Not important enough to report */ + } + } if (PyObject_SetItem(modules, info->name, mod) == -1) { Py_DECREF(mod); From da1f91fdac698641b49228d7d2187cbef15f3e5f Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 1 May 2024 14:21:42 -0600 Subject: [PATCH 20/23] Split up other asserts. --- Python/import.c | 3 ++- Python/importdl.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Python/import.c b/Python/import.c index 53482628f1af0a..cbf5eb77b72e05 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1477,7 +1477,8 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, _Py_ext_module_loader_result_apply_error(&res, name_buf); goto finally; } - assert(!PyErr_Occurred() && res.err == NULL); + assert(!PyErr_Occurred()); + assert(res.err == NULL); mod = res.module; res.module = NULL; diff --git a/Python/importdl.c b/Python/importdl.c index 38a24c1b35844a..11aa9a732d91aa 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -466,7 +466,8 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, } } - assert(!PyErr_Occurred() && res.err == NULL); + assert(!PyErr_Occurred()); + assert(res.err == NULL); *p_res = res; return 0; From c2a687cc354467158103818756df41fc99f7d772 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 1 May 2024 14:24:11 -0600 Subject: [PATCH 21/23] Clear the stolen exception. --- Python/importdl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Python/importdl.c b/Python/importdl.c index 11aa9a732d91aa..2429458bb5b8f0 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -331,6 +331,7 @@ _Py_ext_module_loader_result_apply_error( if (err->exc != NULL) { PyErr_SetRaisedException(err->exc); + err->exc = NULL; /* SetRaisedException() stole our reference. */ if (msg != NULL) { _PyErr_FormatFromCause(PyExc_SystemError, msg, name); } From 211dab3f6e9edd018e1414c8cf222bfacc6144f2 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 1 May 2024 14:45:33 -0600 Subject: [PATCH 22/23] Call _Py_ext_module_loader_result_clear() in _Py_ext_module_loader_result_apply_error(). --- Python/import.c | 20 ++++++++++---------- Python/importdl.c | 25 ++++++++++++++++--------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/Python/import.c b/Python/import.c index cbf5eb77b72e05..cada91f5aa20ac 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1419,7 +1419,6 @@ import_find_extension(PyThreadState *tstate, struct _Py_ext_module_loader_result res; if (_PyImport_RunModInitFunc(def->m_base.m_init, info, &res) < 0) { _Py_ext_module_loader_result_apply_error(&res, name_buf); - _Py_ext_module_loader_result_clear(&res); return NULL; } assert(!PyErr_Occurred()); @@ -1475,7 +1474,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, /* We discard res.def. */ assert(res.module == NULL); _Py_ext_module_loader_result_apply_error(&res, name_buf); - goto finally; + return NULL; } assert(!PyErr_Occurred()); assert(res.err == NULL); @@ -1490,7 +1489,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, assert(mod == NULL); mod = PyModule_FromDefAndSpec(def, spec); if (mod == NULL) { - goto finally; + goto error; } } else { @@ -1499,8 +1498,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, assert(PyModule_Check(mod)); if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { - Py_CLEAR(mod); - goto finally; + goto error; } /* Remember the filename as the __file__ attribute */ @@ -1528,8 +1526,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, if (update_global_state_for_extension( tstate, info->path, info->name, def, &singlephase) < 0) { - Py_CLEAR(mod); - goto finally; + goto error; } /* Update per-interpreter import state. */ @@ -1537,14 +1534,17 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, if (finish_singlephase_extension( tstate, mod, def, info->name, modules) < 0) { - Py_CLEAR(mod); - goto finally; + goto error; } } -finally: _Py_ext_module_loader_result_clear(&res); return mod; + +error: + Py_XDECREF(mod); + _Py_ext_module_loader_result_clear(&res); + return NULL; } diff --git a/Python/importdl.c b/Python/importdl.c index 2429458bb5b8f0..a3091946bc94bf 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -217,6 +217,9 @@ _Py_ext_module_loader_info_init_from_spec( void _Py_ext_module_loader_result_clear(struct _Py_ext_module_loader_result *res) { + /* Instead, the caller should have called + * _Py_ext_module_loader_result_apply_error(). */ + assert(res->err == NULL); *res = (struct _Py_ext_module_loader_result){0}; } @@ -277,21 +280,25 @@ _Py_ext_module_loader_result_apply_error( const char *name) { assert(!PyErr_Occurred()); - struct _Py_ext_module_loader_result_error *err = res->err; - assert(err != NULL && err == &res->_err); + assert(res->err != NULL && res->err == &res->_err); + struct _Py_ext_module_loader_result_error err = *res->err; + res->err = NULL; + + /* We're otherwise done with the result at this point. */ + _Py_ext_module_loader_result_clear(res); #ifndef NDEBUG - switch (err->kind) { + switch (err.kind) { case _Py_ext_module_loader_result_EXCEPTION: /* fall through */ case _Py_ext_module_loader_result_ERR_UNREPORTED_EXC: - assert(err->exc != NULL); + assert(err.exc != NULL); break; case _Py_ext_module_loader_result_ERR_MISSING: /* fall through */ case _Py_ext_module_loader_result_ERR_UNINITIALIZED: /* fall through */ case _Py_ext_module_loader_result_ERR_NONASCII_NOT_MULTIPHASE: /* fall through */ case _Py_ext_module_loader_result_ERR_NOT_MODULE: /* fall through */ case _Py_ext_module_loader_result_ERR_MISSING_DEF: - assert(err->exc == NULL); + assert(err.exc == NULL); break; default: /* We added a new error kind but forgot to add it to this switch. */ @@ -300,7 +307,7 @@ _Py_ext_module_loader_result_apply_error( #endif const char *msg = NULL; - switch (err->kind) { + switch (err.kind) { case _Py_ext_module_loader_result_EXCEPTION: break; case _Py_ext_module_loader_result_ERR_MISSING: @@ -329,9 +336,9 @@ _Py_ext_module_loader_result_apply_error( return; } - if (err->exc != NULL) { - PyErr_SetRaisedException(err->exc); - err->exc = NULL; /* SetRaisedException() stole our reference. */ + if (err.exc != NULL) { + PyErr_SetRaisedException(err.exc); + err.exc = NULL; /* PyErr_SetRaisedException() stole our reference. */ if (msg != NULL) { _PyErr_FormatFromCause(PyExc_SystemError, msg, name); } From fca9857e2e3ddf6b92b37746a81dcc97703886ee Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 1 May 2024 14:51:53 -0600 Subject: [PATCH 23/23] Clarify the comment about modules sharing a def. --- Python/import.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Python/import.c b/Python/import.c index cada91f5aa20ac..0c51ffc6285a9c 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1268,9 +1268,11 @@ update_global_state_for_extension(PyThreadState *tstate, assert(def->m_base.m_copy == NULL); assert(def->m_size >= 0); /* Remember pointer to module init function. */ - // XXX Two modules should not share the same def->m_base. - //assert(def->m_base.m_init == NULL - // || def->m_base.m_init == singlephase->m_init); + // XXX If two modules share a def then def->m_base will + // reflect the last one added (here) to the global cache. + // We should prevent this somehow. The simplest solution + // is probably to store m_copy/m_init in the cache along + // with the def, rather than within the def. def->m_base.m_init = singlephase->m_init; } else if (singlephase->m_dict == NULL) {