From c44d5ebde825250f9afc9e4be53845470119dfcd Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Sun, 17 Mar 2024 23:22:18 +0100 Subject: [PATCH 1/3] WIP: C API PyArg_Parser --- Include/Python.h | 1 + Include/cpython/argparser.h | 37 +++++ Include/internal/pycore_pylifecycle.h | 1 + Makefile.pre.in | 2 + Modules/_testcapimodule.c | 13 ++ PCbuild/pythoncore.vcxproj | 2 + PCbuild/pythoncore.vcxproj.filters | 3 + Python/argparser.c | 197 ++++++++++++++++++++++++++ Python/pylifecycle.c | 4 + 9 files changed, 260 insertions(+) create mode 100644 Include/cpython/argparser.h create mode 100644 Python/argparser.c diff --git a/Include/Python.h b/Include/Python.h index 01fc45137a17bb..e721e17b6c2970 100644 --- a/Include/Python.h +++ b/Include/Python.h @@ -103,6 +103,7 @@ #include "pythread.h" #include "cpython/context.h" #include "modsupport.h" +#include "cpython/argparser.h" #include "compile.h" #include "pythonrun.h" #include "pylifecycle.h" diff --git a/Include/cpython/argparser.h b/Include/cpython/argparser.h new file mode 100644 index 00000000000000..fa2c166b6336cd --- /dev/null +++ b/Include/cpython/argparser.h @@ -0,0 +1,37 @@ +#ifndef Py_LIMITED_API +#ifndef Py_ARGPARSER_H +#define Py_ARGPARSER_H +#ifdef __cplusplus +extern "C" { +#endif + +// PyArgSpec.arg_kind values +#define PyArgKind_POS_ONLY 0 +#define PyArgKind_POS_OR_KW 1 +#define PyArgKind_KW_ONLY 2 +#define PyArgKind_VARARGS 3 +#define PyArgKind_KWARGS 4 + +#define PyArgType_OBJECT 0 + +typedef struct PyArgSpec { + int arg_kind; + int arg_type; + // FIXME: default value +} PyArgSpec; + + +typedef struct PyArgParser { + const char *func_name_utf8; + PyObject *func_name; + Py_ssize_t nspec; + PyArgSpec *specs; + // FIXME: keyword names + void *impl; +} PyArgParser; + +#ifdef __cplusplus +} +#endif +#endif /* !Py_ARGPARSER_H */ +#endif /* !Py_LIMITED_API */ diff --git a/Include/internal/pycore_pylifecycle.h b/Include/internal/pycore_pylifecycle.h index c675098685764c..2bc85b079abd16 100644 --- a/Include/internal/pycore_pylifecycle.h +++ b/Include/internal/pycore_pylifecycle.h @@ -60,6 +60,7 @@ extern void _PyAtExit_Fini(PyInterpreterState *interp); extern void _PyThread_FiniType(PyInterpreterState *interp); extern void _Py_Deepfreeze_Fini(void); extern void _PyArg_Fini(void); +extern void PyArgParser_Fini(void); extern void _Py_FinalizeAllocatedBlocks(_PyRuntimeState *); extern PyStatus _PyGILState_Init(PyInterpreterState *interp); diff --git a/Makefile.pre.in b/Makefile.pre.in index 3cf4de08a0c842..be9b8c0ce962da 100644 --- a/Makefile.pre.in +++ b/Makefile.pre.in @@ -401,6 +401,7 @@ PYTHON_OBJS= \ Python/_warnings.o \ Python/Python-ast.o \ Python/Python-tokenize.o \ + Python/argparser.o \ Python/asdl.o \ Python/assemble.o \ Python/ast.o \ @@ -1058,6 +1059,7 @@ PYTHON_HEADERS= \ $(PARSER_HEADERS) \ \ $(srcdir)/Include/cpython/abstract.h \ + $(srcdir)/Include/cpython/argparser.h \ $(srcdir)/Include/cpython/bytearrayobject.h \ $(srcdir)/Include/cpython/bytesobject.h \ $(srcdir)/Include/cpython/cellobject.h \ diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 7928cd7d6fe1ae..188c9cf64dce98 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -3241,6 +3241,18 @@ test_weakref_capi(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args)) } +static PyObject * +pyargparser_test(PyObject *Py_UNUSED(module), PyObject *args) +{ + extern int PyArgParser_Test(PyObject *args); + + if (PyArgParser_Test(args) < 0) { + return NULL; + } + Py_RETURN_NONE; +} + + static PyMethodDef TestMethods[] = { {"set_errno", set_errno, METH_VARARGS}, {"test_config", test_config, METH_NOARGS}, @@ -3381,6 +3393,7 @@ static PyMethodDef TestMethods[] = { {"function_set_kw_defaults", function_set_kw_defaults, METH_VARARGS, NULL}, {"check_pyimport_addmodule", check_pyimport_addmodule, METH_VARARGS}, {"test_weakref_capi", test_weakref_capi, METH_NOARGS}, + {"PyArgParser_Test", pyargparser_test, METH_VARARGS}, {NULL, NULL} /* sentinel */ }; diff --git a/PCbuild/pythoncore.vcxproj b/PCbuild/pythoncore.vcxproj index 88a4a7c9564309..0643ca6852276e 100644 --- a/PCbuild/pythoncore.vcxproj +++ b/PCbuild/pythoncore.vcxproj @@ -135,6 +135,7 @@ + @@ -549,6 +550,7 @@ + diff --git a/PCbuild/pythoncore.vcxproj.filters b/PCbuild/pythoncore.vcxproj.filters index 27bd1121663398..94713a1b345a5c 100644 --- a/PCbuild/pythoncore.vcxproj.filters +++ b/PCbuild/pythoncore.vcxproj.filters @@ -1244,6 +1244,9 @@ Python + + Python + Python diff --git a/Python/argparser.c b/Python/argparser.c new file mode 100644 index 00000000000000..8452b59aeea024 --- /dev/null +++ b/Python/argparser.c @@ -0,0 +1,197 @@ +#include "Python.h" +#include "pycore_tuple.h" // _PyTuple_ITEMS() +#include // offsetof() + + +typedef struct { + int arg_type; +} PyArgSpecImpl; + +typedef struct PyArgParserImpl { + struct PyArgParserImpl *next; + + // pyargparser_dealloc() clears parser->impl + PyArgParser *parser; + + PyObject *func_name; // can be NULL + + Py_ssize_t nspec; + PyArgSpecImpl specs[1]; +} PyArgParserImpl; + + +static PyArgParserImpl *parser_head = NULL; // FIXME: make it per-interpreter? + + +static void pyargparser_dealloc(PyArgParserImpl *parser) +{ + parser->parser->impl = NULL; + Py_XDECREF(parser->func_name); + PyMem_Free(parser); +} + + +static PyArgParserImpl* pyargparser_create(PyArgParser *parser) +{ + size_t size = offsetof(PyArgParserImpl, specs); + if ((size_t)parser->nspec > ((size_t)PY_SSIZE_T_MAX - size) + / sizeof(PyArgSpecImpl)) { + PyErr_NoMemory(); + return NULL; + } + size += sizeof(PyArgSpecImpl) * parser->nspec; + + + PyArgParserImpl *impl = PyMem_Malloc(size); + if (impl == NULL) { + PyErr_NoMemory(); + return NULL; + } + + impl->parser = parser; + if (parser->func_name != NULL) { + impl->func_name = Py_NewRef(parser->func_name); + } + else if (parser->func_name_utf8 != NULL) { + impl->func_name = PyUnicode_FromString(parser->func_name_utf8); + if (impl->func_name == NULL) { + goto error; + } + } + else { + impl->func_name = NULL; + } + + impl->nspec = parser->nspec; + for (Py_ssize_t i=0; i < parser->nspec; i++) { + PyArgSpecImpl *spec_impl = &impl->specs[i]; + const PyArgSpec *spec = &parser->specs[i]; + spec_impl->arg_type = spec->arg_type; + } + + impl->next = parser_head; + parser_head = impl; + return impl; + +error: + pyargparser_dealloc(impl); + return NULL; +} + + +static int pyargparser_get_impl(PyArgParser *parser, PyArgParserImpl **result) +{ + PyArgParserImpl *impl = parser->impl; + if (impl != NULL) { + *result = impl; + return 0; + } + + impl = pyargparser_create(parser); + *result = impl; + if (impl == NULL) { + return -1; + } + parser->impl = impl; + return 0; +} + + +static int +pyargparser_tuple(PyArgParserImpl *parser, Py_ssize_t nargs, PyObject **args, + va_list va) +{ + Py_ssize_t spec_index = 0; + Py_ssize_t arg_index = 0; + + for (; spec_index < parser->nspec; spec_index++) { + PyArgSpecImpl *spec = &parser->specs[spec_index]; + + assert(spec->arg_type == PyArgType_OBJECT); + + if (arg_index >= nargs) { + PyErr_Format(PyExc_TypeError, "expect X args, got %zd", nargs); + return -1; + } + + PyObject **pobj = va_arg(va, PyObject **); + + *pobj = args[arg_index]; + arg_index++; + } + + if (spec_index != parser->nspec) { + PyErr_Format(PyExc_TypeError, "expect X args, got %zd", nargs); + return -1; + } + if (arg_index != nargs) { + PyErr_Format(PyExc_TypeError, "expect X args, got %zd", nargs); + return -1; + } + return 0; +} + + +int PyArgParser_Tuple(PyArgParser *parser, PyObject *args, ...) +{ + if (!PyTuple_Check(args)) { + PyErr_Format(PyExc_TypeError, "expect args tuple, got %T", args); + return -1; + } + + PyArgParserImpl *impl; + if (pyargparser_get_impl(parser, &impl) < 0) { + return -1; + } + + va_list va; + va_start(va, args); + int res = pyargparser_tuple(impl, + PyTuple_GET_SIZE(args), + _PyTuple_ITEMS(args), + va); + va_end(va); + return res; +} + + +void PyArgParser_Fini(void) +{ + PyArgParserImpl *parser = parser_head; + parser_head = NULL; + + while (parser != NULL) { + PyArgParserImpl *next = parser->next; + pyargparser_dealloc(parser); + parser = next; + } +} + +static PyArgSpec test_specs[] = { + { + .arg_kind = PyArgKind_POS_ONLY, + .arg_type = PyArgType_OBJECT, + }, + { + .arg_kind = PyArgKind_POS_ONLY, + .arg_type = PyArgType_OBJECT, + }, +}; + +static PyArgParser test_parser = { + .func_name_utf8 = "myfunc", + .nspec = Py_ARRAY_LENGTH(test_specs), + .specs = test_specs, +}; + +PyAPI_FUNC(int) PyArgParser_Test(PyObject *args) +{ + PyObject *x = NULL; + PyObject *y = NULL; + if (PyArgParser_Tuple(&test_parser, args, &x, &y) < 0) { + return -1; + } + _PyObject_Dump(x); + _PyObject_Dump(y); + return 0; +} diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 3a2c0a450ac9d9..df93f42cbf1abf 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1837,6 +1837,10 @@ finalize_interp_clear(PyThreadState *tstate) finalize_interp_types(tstate->interp); + if (is_main_interp) { + PyArgParser_Fini(); + } + /* Free any delayed free requests immediately */ _PyMem_FiniDelayed(tstate->interp); From 845207aab88745c693266ad5b6f97ffa7d4bf6c0 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Mon, 18 Mar 2024 00:39:39 +0100 Subject: [PATCH 2/3] FIXME --- Include/cpython/argparser.h | 9 ++++++++- Python/argparser.c | 7 +++++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/Include/cpython/argparser.h b/Include/cpython/argparser.h index fa2c166b6336cd..81dbeb80a760c4 100644 --- a/Include/cpython/argparser.h +++ b/Include/cpython/argparser.h @@ -13,17 +13,24 @@ extern "C" { #define PyArgKind_KWARGS 4 #define PyArgType_OBJECT 0 +// FIXME: support more types +// FIXME: support converter API typedef struct PyArgSpec { + // FIXME: add name for keyword argument + int arg_kind; int arg_type; - // FIXME: default value + + // FIXME: default value for optional argument } PyArgSpec; typedef struct PyArgParser { + // Name used in error message const char *func_name_utf8; PyObject *func_name; + Py_ssize_t nspec; PyArgSpec *specs; // FIXME: keyword names diff --git a/Python/argparser.c b/Python/argparser.c index 8452b59aeea024..d5742aeab6a685 100644 --- a/Python/argparser.c +++ b/Python/argparser.c @@ -20,7 +20,10 @@ typedef struct PyArgParserImpl { } PyArgParserImpl; -static PyArgParserImpl *parser_head = NULL; // FIXME: make it per-interpreter? +// FIXME: make it per-interpreter? +// Or pyargparser_create() must allocate static immortal str objects on the +// heap memory! +static PyArgParserImpl *parser_head = NULL; static void pyargparser_dealloc(PyArgParserImpl *parser) @@ -179,7 +182,7 @@ static PyArgSpec test_specs[] = { }; static PyArgParser test_parser = { - .func_name_utf8 = "myfunc", + .func_name_utf8 = "PyArgParser_Test", .nspec = Py_ARRAY_LENGTH(test_specs), .specs = test_specs, }; From b95019880394d33b0aba020171f193859da20486 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Mon, 18 Mar 2024 01:03:27 +0100 Subject: [PATCH 3/3] use _Py_atomic API --- Python/argparser.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/Python/argparser.c b/Python/argparser.c index d5742aeab6a685..cd0f0b07dceae4 100644 --- a/Python/argparser.c +++ b/Python/argparser.c @@ -28,7 +28,9 @@ static PyArgParserImpl *parser_head = NULL; static void pyargparser_dealloc(PyArgParserImpl *parser) { - parser->parser->impl = NULL; + if (parser->parser != NULL) { + parser->parser->impl = NULL; + } Py_XDECREF(parser->func_name); PyMem_Free(parser); } @@ -44,7 +46,6 @@ static PyArgParserImpl* pyargparser_create(PyArgParser *parser) } size += sizeof(PyArgSpecImpl) * parser->nspec; - PyArgParserImpl *impl = PyMem_Malloc(size); if (impl == NULL) { PyErr_NoMemory(); @@ -72,8 +73,25 @@ static PyArgParserImpl* pyargparser_create(PyArgParser *parser) spec_impl->arg_type = spec->arg_type; } - impl->next = parser_head; - parser_head = impl; + + PyArgParserImpl *other_impl = NULL; + if (!_Py_atomic_compare_exchange_ptr(&parser->impl, &other_impl, impl)) { + // Concurrent call: use the race winner, delete our copy. + impl->parser = NULL; + pyargparser_dealloc(impl); + return other_impl; + } + + // FIXME: use a regular mutex instead? + while (1) { + PyArgParserImpl *next = NULL; + if (_Py_atomic_compare_exchange_ptr(&parser_head, &next, impl)) { + impl->next = next; + break; + } + // lost race, retry + } + return impl; error: @@ -95,7 +113,6 @@ static int pyargparser_get_impl(PyArgParser *parser, PyArgParserImpl **result) if (impl == NULL) { return -1; } - parser->impl = impl; return 0; }