bpo-42990: Refactor _PyFrame_New_NoTrack() (GH-24566)

* Refactor _PyFrame_New_NoTrack() and PyFunction_NewWithQualName()
  code.
* PyFrame_New() checks for _PyEval_BuiltinsFromGlobals() failure.
* Fix a ref leak in _PyEval_BuiltinsFromGlobals() error path.
* Complete PyFunction_GetModule() documentation: it returns a
  borrowed reference and it can return NULL.
* Move _PyEval_BuiltinsFromGlobals() definition to the internal C
  API.
* PyFunction_NewWithQualName() uses _Py_IDENTIFIER() API for the
  "__name__" string to make it compatible with subinterpreters.

Doc/c-api/function.rst

@@ -61,9 +61,11 @@ There are a few functions specific to Python functions.
 
 .. c:function:: PyObject* PyFunction_GetModule(PyObject *op)
 
-   Return the *__module__* attribute of the function object *op*. This is normally
+   Return a :term:`borrowed reference` to the *__module__* attribute of the
-   a string containing the module name, but can be set to any other object by
+   function object *op*. It can be *NULL*.
-   Python code.
+
+   This is normally a string containing the module name, but can be set to any
+   other object by Python code.
 
 
 .. c:function:: PyObject* PyFunction_GetDefaults(PyObject *op)

Include/cpython/frameobject.h

@@ -92,5 +92,3 @@ PyAPI_FUNC(void) PyFrame_FastToLocals(PyFrameObject *);
 PyAPI_FUNC(void) _PyFrame_DebugMallocStats(FILE *out);
 
 PyAPI_FUNC(PyFrameObject *) PyFrame_GetBack(PyFrameObject *frame);
-
-PyObject *_PyEval_BuiltinsFromGlobals(PyObject *globals);

Include/internal/pycore_ceval.h

@@ -31,9 +31,12 @@ PyAPI_FUNC(void) _PyEval_SetCoroutineOriginTrackingDepth(
     PyThreadState *tstate,
     int new_depth);
 
-/* Private function */
 void _PyEval_Fini(void);
 
+
+extern PyObject *_PyEval_BuiltinsFromGlobals(PyObject *globals);
+
+
 static inline PyObject*
 _PyEval_EvalFrame(PyThreadState *tstate, PyFrameObject *f, int throwflag)
 {

Objects/frameobject.c

@@ -1,12 +1,11 @@
 /* Frame object implementation */
 
 #include "Python.h"
-#include "pycore_object.h"
+#include "pycore_ceval.h"         // _PyEval_BuiltinsFromGlobals()
-#include "pycore_gc.h"       // _PyObject_GC_IS_TRACKED()
+#include "pycore_object.h"        // _PyObject_GC_UNTRACK()
 
-#include "code.h"
+#include "frameobject.h"          // PyFrameObject
-#include "frameobject.h"
+#include "opcode.h"               // EXTENDED_ARG
-#include "opcode.h"
 #include "structmember.h"         // PyMemberDef
 
 #define OFF(x) offsetof(PyFrameObject, x)
@@ -762,9 +761,7 @@ _Py_IDENTIFIER(__builtins__);
 static inline PyFrameObject*
 frame_alloc(PyCodeObject *code)
 {
-    PyFrameObject *f;
+    PyFrameObject *f = code->co_zombieframe;
-
-    f = code->co_zombieframe;
     if (f != NULL) {
         code->co_zombieframe = NULL;
         _Py_NewReference((PyObject *)f);
@@ -803,14 +800,11 @@ frame_alloc(PyCodeObject *code)
         _Py_NewReference((PyObject *)f);
     }
 
-    f->f_code = code;
     extras = code->co_nlocals + ncells + nfrees;
     f->f_valuestack = f->f_localsplus + extras;
-    for (Py_ssize_t i=0; i<extras; i++) {
+    for (Py_ssize_t i=0; i < extras; i++) {
         f->f_localsplus[i] = NULL;
     }
-    f->f_locals = NULL;
-    f->f_trace = NULL;
     return f;
 }
 
@@ -818,42 +812,33 @@ frame_alloc(PyCodeObject *code)
 PyFrameObject* _Py_HOT_FUNCTION
 _PyFrame_New_NoTrack(PyThreadState *tstate, PyFrameConstructor *con, PyObject *locals)
 {
-#ifdef Py_DEBUG
+    assert(con != NULL);
-    if (con == NULL || con->fc_code == NULL ||
+    assert(con->fc_globals != NULL);
-        (locals != NULL && !PyMapping_Check(locals))) {
+    assert(con->fc_builtins != NULL);
-        PyErr_BadInternalCall();
+    assert(con->fc_code != NULL);
-        return NULL;
+    assert(locals == NULL || PyMapping_Check(locals));
-    }
-#endif
-
-    PyFrameObject *back = tstate->frame;
 
     PyFrameObject *f = frame_alloc((PyCodeObject *)con->fc_code);
     if (f == NULL) {
         return NULL;
     }
 
+    f->f_back = (PyFrameObject*)Py_XNewRef(tstate->frame);
+    f->f_code = (PyCodeObject *)Py_NewRef(con->fc_code);
+    f->f_builtins = Py_NewRef(con->fc_builtins);
+    f->f_globals = Py_NewRef(con->fc_globals);
+    f->f_locals = Py_XNewRef(locals);
+    // f_valuestack initialized by frame_alloc()
+    f->f_trace = NULL;
     f->f_stackdepth = 0;
-    Py_INCREF(con->fc_builtins);
+    f->f_trace_lines = 1;
-    f->f_builtins = con->fc_builtins;
+    f->f_trace_opcodes = 0;
-    Py_XINCREF(back);
+    f->f_gen = NULL;
-    f->f_back = back;
-    Py_INCREF(con->fc_code);
-    Py_INCREF(con->fc_globals);
-    f->f_globals = con->fc_globals;
-    Py_XINCREF(locals);
-    f->f_locals = locals;
-
     f->f_lasti = -1;
     f->f_lineno = 0;
     f->f_iblock = 0;
     f->f_state = FRAME_CREATED;
-    f->f_gen = NULL;
+    // f_blockstack and f_localsplus initialized by frame_alloc()
-    f->f_trace_opcodes = 0;
-    f->f_trace_lines = 1;
-
-    assert(f->f_code != NULL);
-
     return f;
 }
 
@@ -863,6 +848,9 @@ PyFrame_New(PyThreadState *tstate, PyCodeObject *code,
             PyObject *globals, PyObject *locals)
 {
     PyObject *builtins = _PyEval_BuiltinsFromGlobals(globals);
+    if (builtins == NULL) {
+        return NULL;
+    }
     PyFrameConstructor desc = {
         .fc_globals = globals,
         .fc_builtins = builtins,
@@ -875,8 +863,9 @@ PyFrame_New(PyThreadState *tstate, PyCodeObject *code,
     };
     PyFrameObject *f = _PyFrame_New_NoTrack(tstate, &desc, locals);
     Py_DECREF(builtins);
-    if (f)
+    if (f) {
         _PyObject_GC_TRACK(f);
+    }
     return f;
 }
 
@@ -1173,27 +1162,30 @@ PyFrame_GetBack(PyFrameObject *frame)
     return back;
 }
 
-PyObject *_PyEval_BuiltinsFromGlobals(PyObject *globals) {
+PyObject*
+_PyEval_BuiltinsFromGlobals(PyObject *globals)
+{
     PyObject *builtins = _PyDict_GetItemIdWithError(globals, &PyId___builtins__);
     if (builtins) {
         if (PyModule_Check(builtins)) {
             builtins = PyModule_GetDict(builtins);
             assert(builtins != NULL);
         }
+        return Py_NewRef(builtins);
     }
+
+    if (PyErr_Occurred()) {
+        return NULL;
+    }
+
+    /* No builtins! Make up a minimal one. Give them 'None', at least. */
+    builtins = PyDict_New();
     if (builtins == NULL) {
-        if (PyErr_Occurred()) {
+        return NULL;
-            return NULL;
+    }
-        }
+    if (PyDict_SetItemString(builtins, "None", Py_None) < 0) {
-        /* No builtins!              Make up a minimal one
+        Py_DECREF(builtins);
-            Give them 'None', at least. */
+        return NULL;
-        builtins = PyDict_New();
-        if (builtins == NULL ||
-            PyDict_SetItemString(
-                builtins, "None", Py_None) < 0)
-            return NULL;
     }
-    else
-        Py_INCREF(builtins);
     return builtins;
 }

Objects/funcobject.c

@@ -2,83 +2,90 @@
 /* Function object implementation */
 
 #include "Python.h"
-#include "pycore_object.h"
+#include "pycore_ceval.h"         // _PyEval_BuiltinsFromGlobals()
-#include "frameobject.h"
+#include "pycore_object.h"        // _PyObject_GC_UNTRACK()
-#include "code.h"
 #include "structmember.h"         // PyMemberDef
 
 PyObject *
 PyFunction_NewWithQualName(PyObject *code, PyObject *globals, PyObject *qualname)
 {
-    PyFunctionObject *op;
+    assert(globals != NULL);
-    PyObject *doc, *consts, *module;
+    assert(PyDict_Check(globals));
-    static PyObject *__name__ = NULL;
+    Py_INCREF(globals);
 
-    if (__name__ == NULL) {
+    PyCodeObject *code_obj = (PyCodeObject *)code;
-        __name__ = PyUnicode_InternFromString("__name__");
+    Py_INCREF(code_obj);
-        if (__name__ == NULL)
+
-            return NULL;
+    PyObject *name = code_obj->co_name;
+    assert(name != NULL);
+    Py_INCREF(name);
+    if (!qualname) {
+        qualname = name;
+    }
+    Py_INCREF(qualname);
+
+    PyObject *consts = code_obj->co_consts;
+    assert(PyTuple_Check(consts));
+    PyObject *doc;
+    if (PyTuple_Size(consts) >= 1) {
+        doc = PyTuple_GetItem(consts, 0);
+        if (!PyUnicode_Check(doc)) {
+            doc = Py_None;
+        }
+    }
+    else {
+        doc = Py_None;
+    }
+    Py_INCREF(doc);
+
+    // __module__: Use globals['__name__'] if it exists, or NULL.
+    _Py_IDENTIFIER(__name__);
+    PyObject *module = _PyDict_GetItemIdWithError(globals, &PyId___name__);
+    PyObject *builtins = NULL;
+    if (module == NULL && PyErr_Occurred()) {
+        goto error;
+    }
+    Py_XINCREF(module);
+
+    builtins = _PyEval_BuiltinsFromGlobals(globals);
+    if (builtins == NULL) {
+        goto error;
     }
 
-    /* __module__: If module name is in globals, use it.
+    PyFunctionObject *op = PyObject_GC_New(PyFunctionObject, &PyFunction_Type);
-       Otherwise, use None. */
-    module = PyDict_GetItemWithError(globals, __name__);
-    if (module) {
-        Py_INCREF(module);
-    }
-    else if (PyErr_Occurred()) {
-        return NULL;
-    }
-
-    op = PyObject_GC_New(PyFunctionObject, &PyFunction_Type);
     if (op == NULL) {
-        Py_XDECREF(module);
+        goto error;
-        return NULL;
     }
     /* Note: No failures from this point on, since func_dealloc() does not
        expect a partially-created object. */
 
-    op->func_weakreflist = NULL;
-    Py_INCREF(code);
-    op->func_code = code;
-    assert(globals != NULL);
-    Py_INCREF(globals);
     op->func_globals = globals;
-    PyObject *builtins = _PyEval_BuiltinsFromGlobals(globals);
-    if (builtins == NULL) {
-        return NULL;
-    }
     op->func_builtins = builtins;
-    op->func_name = ((PyCodeObject *)code)->co_name;
+    op->func_name = name;
-    Py_INCREF(op->func_name);
+    op->func_qualname = qualname;
-    op->func_defaults = NULL; /* No default arguments */
+    op->func_code = (PyObject*)code_obj;
-    op->func_kwdefaults = NULL; /* No keyword only defaults */
+    op->func_defaults = NULL;    // No default positional arguments
+    op->func_kwdefaults = NULL;  // No default keyword arguments
     op->func_closure = NULL;
-    op->vectorcall = _PyFunction_Vectorcall;
-    op->func_module = module;
-
-    consts = ((PyCodeObject *)code)->co_consts;
-    if (PyTuple_Size(consts) >= 1) {
-        doc = PyTuple_GetItem(consts, 0);
-        if (!PyUnicode_Check(doc))
-            doc = Py_None;
-    }
-    else
-        doc = Py_None;
-    Py_INCREF(doc);
     op->func_doc = doc;
-
     op->func_dict = NULL;
+    op->func_weakreflist = NULL;
+    op->func_module = module;
     op->func_annotations = NULL;
-
+    op->vectorcall = _PyFunction_Vectorcall;
-    if (qualname)
-        op->func_qualname = qualname;
-    else
-        op->func_qualname = op->func_name;
-    Py_INCREF(op->func_qualname);
 
     _PyObject_GC_TRACK(op);
     return (PyObject *)op;
+
+error:
+    Py_DECREF(globals);
+    Py_DECREF(code_obj);
+    Py_DECREF(name);
+    Py_DECREF(qualname);
+    Py_DECREF(doc);
+    Py_XDECREF(module);
+    Py_XDECREF(builtins);
+    return NULL;
 }
 
 PyObject *

Python/ceval.c

@@ -908,7 +908,7 @@ PyEval_EvalCode(PyObject *co, PyObject *globals, PyObject *locals)
         .fc_closure = NULL
     };
     PyThreadState *tstate = PyThreadState_GET();
-    PyObject *res =_PyEval_Vector(tstate, &desc, locals, NULL, 0, NULL);
+    PyObject *res = _PyEval_Vector(tstate, &desc, locals, NULL, 0, NULL);
     Py_DECREF(builtins);
     return res;
 }
@@ -4800,8 +4800,8 @@ PyEval_EvalCodeEx(PyObject *_co, PyObject *globals, PyObject *locals,
     };
     PyThreadState *tstate = _PyThreadState_GET();
     res = _PyEval_Vector(tstate, &constr, locals,
-                                    allargs, argcount,
+                         allargs, argcount,
-                                    kwnames);
+                         kwnames);
     if (kwcount) {
         Py_DECREF(kwnames);
         PyMem_Free(newargs);