-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-40941: Unify implicit and explicit state in the frame and generator objects into a single value. #20803
bpo-40941: Unify implicit and explicit state in the frame and generator objects into a single value. #20803
Changes from all commits
0d4d7e4
a7e7f75
fe00513
eeb0e4d
286f281
39d0996
15a965e
a22412c
38792f2
21a8f2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -300,17 +300,20 @@ first_line_not_before(int *lines, int len, int line) | |
static void | ||
frame_stack_pop(PyFrameObject *f) | ||
{ | ||
PyObject *v = (*--f->f_stacktop); | ||
assert(f->f_stackdepth >= 0); | ||
f->f_stackdepth--; | ||
PyObject *v = f->f_valuestack[f->f_stackdepth]; | ||
Py_DECREF(v); | ||
} | ||
|
||
static void | ||
frame_block_unwind(PyFrameObject *f) | ||
{ | ||
assert(f->f_stackdepth >= 0); | ||
assert(f->f_iblock > 0); | ||
f->f_iblock--; | ||
PyTryBlock *b = &f->f_blockstack[f->f_iblock]; | ||
intptr_t delta = (f->f_stacktop - f->f_valuestack) - b->b_level; | ||
intptr_t delta = f->f_stackdepth - b->b_level; | ||
while (delta > 0) { | ||
frame_stack_pop(f); | ||
delta--; | ||
|
@@ -352,33 +355,36 @@ frame_setlineno(PyFrameObject *f, PyObject* p_new_lineno, void *Py_UNUSED(ignore | |
return -1; | ||
} | ||
|
||
/* Upon the 'call' trace event of a new frame, f->f_lasti is -1 and | ||
* f->f_trace is NULL, check first on the first condition. | ||
* Forbidding jumps from the 'call' event of a new frame is a side effect | ||
* of allowing to set f_lineno only from trace functions. */ | ||
if (f->f_lasti == -1) { | ||
PyErr_Format(PyExc_ValueError, | ||
/* | ||
* This code preserves the historical restrictions on | ||
* setting the line number of a frame. | ||
* Jumps are forbidden on a 'return' trace event (except after a yield). | ||
* Jumps from 'call' trace events are also forbidden. | ||
* In addition, jumps are forbidden when not tracing, | ||
* as this is a debugging feature. | ||
*/ | ||
switch(f->f_state) { | ||
case FRAME_CREATED: | ||
PyErr_Format(PyExc_ValueError, | ||
"can't jump from the 'call' trace event of a new frame"); | ||
return -1; | ||
} | ||
|
||
/* You can only do this from within a trace function, not via | ||
* _getframe or similar hackery. */ | ||
if (!f->f_trace) { | ||
PyErr_Format(PyExc_ValueError, | ||
"f_lineno can only be set by a trace function"); | ||
return -1; | ||
} | ||
|
||
/* Forbid jumps upon a 'return' trace event (except after executing a | ||
* YIELD_VALUE or YIELD_FROM opcode, f_stacktop is not NULL in that case) | ||
* and upon an 'exception' trace event. | ||
* Jumps from 'call' trace events have already been forbidden above for new | ||
* frames, so this check does not change anything for 'call' events. */ | ||
if (f->f_stacktop == NULL) { | ||
PyErr_SetString(PyExc_ValueError, | ||
return -1; | ||
case FRAME_RETURNED: | ||
case FRAME_UNWINDING: | ||
case FRAME_RAISED: | ||
case FRAME_CLEARED: | ||
PyErr_SetString(PyExc_ValueError, | ||
"can only jump from a 'line' trace event"); | ||
return -1; | ||
return -1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is a "call" trace also a "line" trace? If not, it would make sense to combine the 4 invalid states into a single error message, and it might make sense to include the invalid state as an exception attribute, or even as part of the string. if that is much easier. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No a "call" trace is not a "line" trace. |
||
case FRAME_EXECUTING: | ||
case FRAME_SUSPENDED: | ||
/* You can only do this from within a trace function, not via | ||
* _getframe or similar hackery. */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this restriction? Is it just because "better safe than sorry, and we couldn't add the restriction later"? Is it an intentional limitation to be kind to PyPy and optimization? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It a pre-existing restriction. There is no change to behavior here. |
||
if (!f->f_trace) { | ||
PyErr_Format(PyExc_ValueError, | ||
"f_lineno can only be set by a trace function"); | ||
return -1; | ||
} | ||
break; | ||
} | ||
|
||
int new_lineno; | ||
|
@@ -585,11 +591,10 @@ frame_dealloc(PyFrameObject *f) | |
} | ||
|
||
/* Free stack */ | ||
if (f->f_stacktop != NULL) { | ||
for (PyObject **p = valuestack; p < f->f_stacktop; p++) { | ||
Py_XDECREF(*p); | ||
} | ||
for (int i = 0; i < f->f_stackdepth; i++) { | ||
Py_XDECREF(f->f_valuestack[i]); | ||
} | ||
f->f_stackdepth = 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I almost want to change this to take advantage of f_stackdepth already being an integer count of the number of times to loop, but maybe that is too clever for debugging, and should be left to the compiler. while (f->f_stackdepth--) Py_XDRECREF(f->f_valuestack[f->f_stackdepth]) ; seems not worse, but if I missed something by not testing, that argues for not making similar changes. :{ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we should leave optimizing the loop to the compiler, although using idiomatic forms usually helps the compiler. |
||
|
||
Py_XDECREF(f->f_back); | ||
Py_DECREF(f->f_builtins); | ||
|
@@ -647,10 +652,8 @@ frame_traverse(PyFrameObject *f, visitproc visit, void *arg) | |
} | ||
|
||
/* stack */ | ||
if (f->f_stacktop != NULL) { | ||
for (PyObject **p = f->f_valuestack; p < f->f_stacktop; p++) { | ||
Py_VISIT(*p); | ||
} | ||
for (int i = 0; i < f->f_stackdepth; i++) { | ||
Py_VISIT(f->f_valuestack[i]); | ||
} | ||
return 0; | ||
} | ||
|
@@ -663,9 +666,7 @@ frame_tp_clear(PyFrameObject *f) | |
* frame may also point to this frame, believe itself to still be | ||
* active, and try cleaning up this frame again. | ||
*/ | ||
PyObject **oldtop = f->f_stacktop; | ||
f->f_stacktop = NULL; | ||
f->f_executing = 0; | ||
f->f_state = FRAME_CLEARED; | ||
|
||
Py_CLEAR(f->f_trace); | ||
|
||
|
@@ -676,18 +677,17 @@ frame_tp_clear(PyFrameObject *f) | |
} | ||
|
||
/* stack */ | ||
if (oldtop != NULL) { | ||
for (PyObject **p = f->f_valuestack; p < oldtop; p++) { | ||
Py_CLEAR(*p); | ||
} | ||
for (int i = 0; i < f->f_stackdepth; i++) { | ||
Py_CLEAR(f->f_valuestack[i]); | ||
} | ||
f->f_stackdepth = 0; | ||
return 0; | ||
} | ||
|
||
static PyObject * | ||
frame_clear(PyFrameObject *f, PyObject *Py_UNUSED(ignored)) | ||
{ | ||
if (f->f_executing) { | ||
if (_PyFrame_IsExecuting(f)) { | ||
PyErr_SetString(PyExc_RuntimeError, | ||
"cannot clear an executing frame"); | ||
return NULL; | ||
|
@@ -898,7 +898,7 @@ _PyFrame_New_NoTrack(PyThreadState *tstate, PyCodeObject *code, | |
return NULL; | ||
} | ||
|
||
f->f_stacktop = f->f_valuestack; | ||
f->f_stackdepth = 0; | ||
f->f_builtins = builtins; | ||
Py_XINCREF(back); | ||
f->f_back = back; | ||
|
@@ -927,7 +927,7 @@ _PyFrame_New_NoTrack(PyThreadState *tstate, PyCodeObject *code, | |
f->f_lasti = -1; | ||
f->f_lineno = code->co_firstlineno; | ||
f->f_iblock = 0; | ||
f->f_executing = 0; | ||
f->f_state = FRAME_CREATED; | ||
f->f_gen = NULL; | ||
f->f_trace_opcodes = 0; | ||
f->f_trace_lines = 1; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just something to be vary about here...there was recently https://bugs.python.org/issue39542#msg372962 where a
static inline
function failed to inline on mac, albeit it was through a more complicated call chain.Not sure if you have the capability to test this but it'd be nice if someone can benchmark this on mac since this is such a critical code path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should trust the big three compilers to do their jobs without constantly verifying it.
Clang will inline this very simple function, probably even if we don't tell it to.