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.

1 files changed, 43 insertions, 51 deletions

diff --git a/Objects/frameobject.c b/Objects/frameobject.c
index 57105e1..5f7fa40 100644
--- a/Objects/frameobject.c
+++ b/Objects/frameobject.c
@@ -1,12 +1,11 @@
 /* Frame object implementation */
 
 #include "Python.h"
-#include "pycore_object.h"
-#include "pycore_gc.h"       // _PyObject_GC_IS_TRACKED()
+#include "pycore_ceval.h"         // _PyEval_BuiltinsFromGlobals()
+#include "pycore_object.h"        // _PyObject_GC_UNTRACK()
 
-#include "code.h"
-#include "frameobject.h"
-#include "opcode.h"
+#include "frameobject.h"          // PyFrameObject
+#include "opcode.h"               // EXTENDED_ARG
 #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;
-
-    f = code->co_zombieframe;
+    PyFrameObject *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
-    if (con == NULL || con->fc_code == NULL ||
-        (locals != NULL && !PyMapping_Check(locals))) {
-        PyErr_BadInternalCall();
-        return NULL;
-    }
-#endif
-
-    PyFrameObject *back = tstate->frame;
+    assert(con != NULL);
+    assert(con->fc_globals != NULL);
+    assert(con->fc_builtins != NULL);
+    assert(con->fc_code != NULL);
+    assert(locals == NULL || PyMapping_Check(locals));
 
     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_builtins = con->fc_builtins;
-    Py_XINCREF(back);
-    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_trace_lines = 1;
+    f->f_trace_opcodes = 0;
+    f->f_gen = NULL;
     f->f_lasti = -1;
     f->f_lineno = 0;
     f->f_iblock = 0;
     f->f_state = FRAME_CREATED;
-    f->f_gen = NULL;
-    f->f_trace_opcodes = 0;
-    f->f_trace_lines = 1;
-
-    assert(f->f_code != NULL);
-
+    // f_blockstack and f_localsplus initialized by frame_alloc()
     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;
-        }
-        /* No builtins!              Make up a minimal one
-            Give them 'None', at least. */
-        builtins = PyDict_New();
-        if (builtins == NULL ||
-            PyDict_SetItemString(
-                builtins, "None", Py_None) < 0)
-            return NULL;
+        return NULL;
+    }
+    if (PyDict_SetItemString(builtins, "None", Py_None) < 0) {
+        Py_DECREF(builtins);
+        return NULL;
     }
-    else
-        Py_INCREF(builtins);
     return builtins;
 }