Repository: python/cpython
Pull request: gh-100720: refactor calculation of number of frame slots for a code object into the new function _PyFrame_NumSlotsForCodeObject
Fixes #100720.



<!-- gh-issue-number: gh-100720 -->
* Issue: gh-100720
<!-- /gh-issue-number -->


Problem statement:
Add _PyFrame_NumSlotsForCodeObject()
The calculation of how many slots need to be in the frame of a given code object is repeated in a number of places in the codebase, which makes it error prone to try and change the frame layout.  It should be refactored into a single place.

(lessons learnt from the register machine experiment).

<!-- gh-linked-prs -->
### Linked PRs
* gh-100722
<!-- /gh-linked-prs -->

Hints from issue discussion:

Commit message: add comments that deepfreeze.py and Objects/codeobject.c need to be kept in sync

Patch to review:
diff --git a/Include/internal/pycore_code.h b/Include/internal/pycore_code.h
index 9e59fc98bf3d57..8d1bb8c52e26b0 100644
--- a/Include/internal/pycore_code.h
+++ b/Include/internal/pycore_code.h
@@ -199,6 +199,8 @@ extern PyObject* _PyCode_GetCellvars(PyCodeObject *);
 extern PyObject* _PyCode_GetFreevars(PyCodeObject *);
 extern PyObject* _PyCode_GetCode(PyCodeObject *);
 
+extern int _PyCode_GetNumFrameSlots(PyCodeObject *);
+
 /** API for initializing the line number tables. */
 extern int _PyCode_InitAddressRange(PyCodeObject* co, PyCodeAddressRange *bounds);
 
diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-01-03-16-50-42.gh-issue-100720.UhE7P-.rst b/Misc/NEWS.d/next/Core and Builtins/2023-01-03-16-50-42.gh-issue-100720.UhE7P-.rst
new file mode 100644
index 00000000000000..ba2d843df52e7b
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2023-01-03-16-50-42.gh-issue-100720.UhE7P-.rst	
@@ -0,0 +1 @@
+Added ``_PyCode_GetNumFrameSlots``, which returns the number of slots needed in a frame for a given code object.
diff --git a/Objects/codeobject.c b/Objects/codeobject.c
index e174c6fee9cc24..106f8f9cfe39d0 100644
--- a/Objects/codeobject.c
+++ b/Objects/codeobject.c
@@ -1520,6 +1520,14 @@ PyCode_GetFreevars(PyCodeObject *code)
     return _PyCode_GetFreevars(code);
 }
 
+int
+_PyCode_GetNumFrameSlots(PyCodeObject *code)
+{
+    /* This function needs to remain in sync with Tools/build/deepfreeze.py */
+    assert(code->co_framesize >= (int)FRAME_SPECIALS_SIZE);
+    return code->co_framesize - FRAME_SPECIALS_SIZE;
+}
+
 static void
 deopt_code(_Py_CODEUNIT *instructions, Py_ssize_t len)
 {
diff --git a/Objects/frameobject.c b/Objects/frameobject.c
index eab85c08fc0165..2780357b015291 100644
--- a/Objects/frameobject.c
+++ b/Objects/frameobject.c
@@ -946,7 +946,7 @@ frame_sizeof(PyFrameObject *f, PyObject *Py_UNUSED(ignored))
     Py_ssize_t res;
     res = offsetof(PyFrameObject, _f_frame_data) + offsetof(_PyInterpreterFrame, localsplus);
     PyCodeObject *code = f->f_frame->f_code;
-    res += (code->co_nlocalsplus+code->co_stacksize) * sizeof(PyObject *);
+    res += _PyCode_GetNumFrameSlots(code) * sizeof(PyObject *);
     return PyLong_FromSsize_t(res);
 }
 
diff --git a/Objects/genobject.c b/Objects/genobject.c
index c006f1af2177f9..26aee2b3864cb9 100644
--- a/Objects/genobject.c
+++ b/Objects/genobject.c
@@ -769,7 +769,7 @@ gen_sizeof(PyGenObject *gen, PyObject *Py_UNUSED(ignored))
     Py_ssize_t res;
     res = offsetof(PyGenObject, gi_iframe) + offsetof(_PyInterpreterFrame, localsplus);
     PyCodeObject *code = gen->gi_code;
-    res += (code->co_nlocalsplus+code->co_stacksize) * sizeof(PyObject *);
+    res += _PyCode_GetNumFrameSlots(code) * sizeof(PyObject *);
     return PyLong_FromSsize_t(res);
 }
 
@@ -850,7 +850,7 @@ static PyObject *
 make_gen(PyTypeObject *type, PyFunctionObject *func)
 {
     PyCodeObject *code = (PyCodeObject *)func->func_code;
-    int slots = code->co_nlocalsplus + code->co_stacksize;
+    int slots = _PyCode_GetNumFrameSlots(code);
     PyGenObject *gen = PyObject_GC_NewVar(PyGenObject, type, slots);
     if (gen == NULL) {
         return NULL;
diff --git a/Tools/build/deepfreeze.py b/Tools/build/deepfreeze.py
index 7f4e24280133f2..ca3d004e6fd979 100644
--- a/Tools/build/deepfreeze.py
+++ b/Tools/build/deepfreeze.py
@@ -262,6 +262,7 @@ def generate_code(self, name: str, code: types.CodeType) -> str:
             self.field(code, "co_argcount")
             self.field(code, "co_posonlyargcount")
             self.field(code, "co_kwonlyargcount")
+            # The following should remain in sync with _PyCode_GetNumFrameSlots
             self.write(f".co_framesize = {code.co_stacksize + len(localsplusnames)} + FRAME_SPECIALS_SIZE,")
             self.field(code, "co_stacksize")
             self.field(code, "co_firstlineno")


Code review comments:
diff_hunk: @@ -1520,6 +1520,14 @@ PyCode_GetFreevars(PyCodeObject *code)
     return _PyCode_GetFreevars(code);
 }
 
+int
+_PyCode_GetNumFrameSlots(PyCodeObject *code)
+{
+    /* This function needs to remain in sync with Tools/build/deepfreeze.py */
+    assert(code->co_framesize >= (int)FRAME_SPECIALS_SIZE);
+    return code->co_framesize - FRAME_SPECIALS_SIZE;
path: Objects/codeobject.c
text: @user2:
Odd that one line needs an `(int)` cast and the other doesn't. This actually makes me think that the cast belongs in the definition of `FRAME_SPECIALS_SIZE`. @user1 ?

@user1:
Adding the cast to `FRAME_SPECIALS_SIZE` makes sense.
It is an oddity of the C compiler that it doesn't treat `sizeof(int)` as `4` but as a `size_t` value which needs to be cast.

@user2:
I know. My question was about why apparently `int >= size_t` is a problem (which we solve with a cast) but `int - size_t` isn't. I'm guessing that the `>=` converts the `int` to `size_t` and (regardless of whether that does sign extension) negative ints will appear to be larger than zero, while `-` converts the `size_t` to `int` and all's well?