From e0f24592396000c7eafb99f46d8c1f6c46790e51 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Wed, 19 Oct 2022 23:45:07 -0700 Subject: [PATCH 1/6] Just assign None instead of changing co_code --- Lib/test/test_peepholer.py | 31 +++++++---- Objects/frameobject.c | 111 ++++++++++++++++--------------------- 2 files changed, 68 insertions(+), 74 deletions(-) diff --git a/Lib/test/test_peepholer.py b/Lib/test/test_peepholer.py index 7363452f5e132f..d166c5086f2cef 100644 --- a/Lib/test/test_peepholer.py +++ b/Lib/test/test_peepholer.py @@ -819,8 +819,8 @@ def test_setting_lineno_adds_check(self): code = textwrap.dedent("""\ def f(): x = 2 - L = 3 - L = 4 + if not x: + return 4 for i in range(55): x + 6 del x @@ -832,15 +832,22 @@ def f(): exec(code, ns) f = ns['f'] self.assertInBytecode(f, "LOAD_FAST") + self.assertNotInBytecode(f, "LOAD_FAST_CHECK") + co_code = f.__code__.co_code def trace(frame, event, arg): if event == 'line' and frame.f_lineno == 9: - frame.f_lineno = 2 + frame.f_lineno = 3 sys.settrace(None) return None return trace - sys.settrace(trace) - f() - self.assertNotInBytecode(f, "LOAD_FAST") + e = r"cannot leave local name 'x' unbound \(assigning None\)" + with self.assertWarnsRegex(RuntimeWarning, e): + sys.settrace(trace) + result = f() + self.assertEqual(result, 4) + self.assertInBytecode(f, "LOAD_FAST") + self.assertNotInBytecode(f, "LOAD_FAST_CHECK") + self.assertEqual(f.__code__.co_code, co_code) def make_function_with_no_checks(self): code = textwrap.dedent("""\ @@ -862,16 +869,20 @@ def f(): def test_deleting_local_adds_check(self): f = self.make_function_with_no_checks() + co_code = f.__code__.co_code def trace(frame, event, arg): if event == 'line' and frame.f_lineno == 4: del frame.f_locals["x"] sys.settrace(None) return None return trace - sys.settrace(trace) - f() - self.assertNotInBytecode(f, "LOAD_FAST") - self.assertInBytecode(f, "LOAD_FAST_CHECK") + e = r"cannot leave local name 'x' unbound \(assigning None\)" + with self.assertWarnsRegex(RuntimeWarning, e): + sys.settrace(trace) + f() + self.assertInBytecode(f, "LOAD_FAST") + self.assertNotInBytecode(f, "LOAD_FAST_CHECK") + self.assertEqual(f.__code__.co_code, co_code) def test_modifying_local_does_not_add_check(self): f = self.make_function_with_no_checks() diff --git a/Objects/frameobject.c b/Objects/frameobject.c index 1f1228b7e8022d..8c8f61f177aa0a 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -620,39 +620,46 @@ _PyFrame_GetState(PyFrameObject *frame) Py_UNREACHABLE(); } -static void -add_load_fast_null_checks(PyCodeObject *co) +static bool +op_exists(PyCodeObject *co, int opcode, int oparg, int start, int stop) { - int changed = 0; + // This only works when opcode is a non-quickened form: + assert(_PyOpcode_Deopt[opcode] == opcode); + int check_oparg = 0; _Py_CODEUNIT *instructions = _PyCode_CODE(co); - for (Py_ssize_t i = 0; i < Py_SIZE(co); i++) { - int opcode = _Py_OPCODE(instructions[i]); - switch (opcode) { - case LOAD_FAST: - case LOAD_FAST__LOAD_FAST: - case LOAD_FAST__LOAD_CONST: - changed = 1; - _Py_SET_OPCODE(instructions[i], LOAD_FAST_CHECK); - break; - case LOAD_CONST__LOAD_FAST: - changed = 1; - _Py_SET_OPCODE(instructions[i], LOAD_CONST); - break; - case STORE_FAST__LOAD_FAST: - changed = 1; - _Py_SET_OPCODE(instructions[i], STORE_FAST); - break; + while (start < stop) { + int check_opcode = _PyOpcode_Deopt[_Py_OPCODE(instructions[start])]; + check_oparg |= _Py_OPARG(instructions[start]); + if (check_opcode == opcode && check_oparg == oparg) { + return true; + } + if (check_opcode == EXTENDED_ARG) { + check_oparg <<= 8; } - i += _PyOpcode_Caches[_PyOpcode_Deopt[opcode]]; + else { + check_oparg = 0; + } + start += 1 + _PyOpcode_Caches[check_opcode]; } - if (changed && co->_co_cached != NULL) { - // invalidate cached co_code object - Py_CLEAR(co->_co_cached->_co_code); - Py_CLEAR(co->_co_cached->_co_cellvars); - Py_CLEAR(co->_co_cached->_co_freevars); - Py_CLEAR(co->_co_cached->_co_varnames); - PyMem_Free(co->_co_cached); - co->_co_cached = NULL; + return false; +} + +static void +maybe_bind_fast_local(_PyInterpreterFrame *f, int i) +{ + PyCodeObject *co = f->f_code; + if (f->localsplus[i] == NULL && op_exists(co, LOAD_FAST, i, 0, Py_SIZE(co))) + { + // We're being overly-cautious here (it's too expensive to run a full + // flow analysis), but at least one instruction expects i to be bound. + // Rather than crashing (or changing co_code), just bind None instead: + const char *e = "cannot leave local name %R unbound (assigning None)"; + PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); + if (PyErr_WarnFormat(PyExc_RuntimeWarning, 0, e, name)) { + // It's okay if frame_obj is NULL, just try anyways: + PyErr_WriteUnraisable((PyObject *)f->frame_obj); + } + f->localsplus[i] = Py_NewRef(Py_None); } } @@ -745,8 +752,6 @@ frame_setlineno(PyFrameObject *f, PyObject* p_new_lineno, void *Py_UNUSED(ignore return -1; } - add_load_fast_null_checks(f->f_frame->f_code); - /* PyCode_NewWithPosOnlyArgs limits co_code to be under INT_MAX so this * should never overflow. */ int len = (int)Py_SIZE(f->f_frame->f_code); @@ -826,6 +831,10 @@ frame_setlineno(PyFrameObject *f, PyObject* p_new_lineno, void *Py_UNUSED(ignore } start_stack = pop_value(start_stack); } + // Populate any NULL locals that might be needed: + for (int i = 0; i < f->f_frame->f_code->co_nlocalsplus; i++) { + maybe_bind_fast_local(f->f_frame, i); + } /* Finally set the new lasti and return OK. */ f->f_lineno = 0; f->f_frame->prev_instr = _PyCode_CODE(f->f_frame->f_code) + best_addr; @@ -1108,31 +1117,6 @@ PyFrame_New(PyThreadState *tstate, PyCodeObject *code, return f; } -static int -_PyFrame_OpAlreadyRan(_PyInterpreterFrame *frame, int opcode, int oparg) -{ - // This only works when opcode is a non-quickened form: - assert(_PyOpcode_Deopt[opcode] == opcode); - int check_oparg = 0; - for (_Py_CODEUNIT *instruction = _PyCode_CODE(frame->f_code); - instruction < frame->prev_instr; instruction++) - { - int check_opcode = _PyOpcode_Deopt[_Py_OPCODE(*instruction)]; - check_oparg |= _Py_OPARG(*instruction); - if (check_opcode == opcode && check_oparg == oparg) { - return 1; - } - if (check_opcode == EXTENDED_ARG) { - check_oparg <<= 8; - } - else { - check_oparg = 0; - } - instruction += _PyOpcode_Caches[check_opcode]; - } - return 0; -} - int _PyFrame_FastToLocalsWithError(_PyInterpreterFrame *frame) { /* Merge fast locals into f->f_locals */ @@ -1195,7 +1179,8 @@ _PyFrame_FastToLocalsWithError(_PyInterpreterFrame *frame) { // run yet. if (value != NULL) { if (PyCell_Check(value) && - _PyFrame_OpAlreadyRan(frame, MAKE_CELL, i)) { + op_exists(co, MAKE_CELL, i, 0, lasti)) + { // (likely) MAKE_CELL must have executed already. value = PyCell_GET(value); } @@ -1269,7 +1254,7 @@ _PyFrame_LocalsToFast(_PyInterpreterFrame *frame, int clear) } fast = _PyFrame_GetLocalsArray(frame); co = frame->f_code; - bool added_null_checks = false; + int lasti = _PyInterpreterFrame_LASTI(frame); PyErr_Fetch(&error_type, &error_value, &error_traceback); for (int i = 0; i < co->co_nlocalsplus; i++) { @@ -1289,10 +1274,6 @@ _PyFrame_LocalsToFast(_PyInterpreterFrame *frame, int clear) } } PyObject *oldvalue = fast[i]; - if (!added_null_checks && oldvalue != NULL && value == NULL) { - add_load_fast_null_checks(co); - added_null_checks = true; - } PyObject *cell = NULL; if (kind == CO_FAST_FREE) { // The cell was set when the frame was created from @@ -1302,8 +1283,8 @@ _PyFrame_LocalsToFast(_PyInterpreterFrame *frame, int clear) } else if (kind & CO_FAST_CELL && oldvalue != NULL) { /* Same test as in PyFrame_FastToLocals() above. */ - if (PyCell_Check(oldvalue) && - _PyFrame_OpAlreadyRan(frame, MAKE_CELL, i)) { + if (PyCell_Check(oldvalue) && op_exists(co, MAKE_CELL, i, 0, lasti)) + { // (likely) MAKE_CELL must have executed already. cell = oldvalue; } @@ -1321,6 +1302,8 @@ _PyFrame_LocalsToFast(_PyInterpreterFrame *frame, int clear) else if (value != oldvalue) { Py_XINCREF(value); Py_XSETREF(fast[i], value); + // Populate any NULL locals that might be needed: + maybe_bind_fast_local(frame, i); } Py_XDECREF(value); } From 6f68d329dcb1cdbc79bd55f260190bee943bc1b2 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Wed, 19 Oct 2022 23:55:47 -0700 Subject: [PATCH 2/6] blurb add --- .../2022-10-19-23-54-43.gh-issue-93143.1wCYub.rst | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2022-10-19-23-54-43.gh-issue-93143.1wCYub.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-10-19-23-54-43.gh-issue-93143.1wCYub.rst b/Misc/NEWS.d/next/Core and Builtins/2022-10-19-23-54-43.gh-issue-93143.1wCYub.rst new file mode 100644 index 00000000000000..934596876380e8 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2022-10-19-23-54-43.gh-issue-93143.1wCYub.rst @@ -0,0 +1,4 @@ +Rather than changing :attr:`~types.CodeType.co_code`, the interpreter will +now display a :exc:`RuntimeWarning` and assign :const:`None` to any fast +locals that are incorrectly left unbound after jumps or :keyword:`del` +statements executed while tracing. From dd52da542cc149a005a37ddde1030e08f1541227 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 20 Oct 2022 14:35:58 -0700 Subject: [PATCH 3/6] Feedback from code review --- Lib/test/test_peepholer.py | 78 +++++++++++++++++++++++-- Objects/frameobject.c | 113 ++++++++++++++++++++----------------- 2 files changed, 133 insertions(+), 58 deletions(-) diff --git a/Lib/test/test_peepholer.py b/Lib/test/test_peepholer.py index d166c5086f2cef..aa79f6d2cafc54 100644 --- a/Lib/test/test_peepholer.py +++ b/Lib/test/test_peepholer.py @@ -815,10 +815,42 @@ def f(): self.assertInBytecode(f, 'LOAD_FAST_CHECK', "a73") self.assertInBytecode(f, 'LOAD_FAST', "a73") - def test_setting_lineno_adds_check(self): - code = textwrap.dedent("""\ + def test_setting_lineno_warns_and_assigns_none_0(self): + code = textwrap.dedent(f"""\ def f(): - x = 2 + x = y = 2 + if not x: + return 4 + for i in range(55): + x + 6 + L = 7 + L = 8 + L = 9 + L = 10 + """) + ns = {} + exec(code, ns) + f = ns['f'] + self.assertInBytecode(f, "LOAD_FAST") + self.assertNotInBytecode(f, "LOAD_FAST_CHECK") + co_code = f.__code__.co_code + def trace(frame, event, arg): + if event == 'line' and frame.f_lineno == 9: + frame.f_lineno = 3 + sys.settrace(None) + return None + return trace + sys.settrace(trace) + result = f() + self.assertIsNone(result) + self.assertInBytecode(f, "LOAD_FAST") + self.assertNotInBytecode(f, "LOAD_FAST_CHECK") + self.assertEqual(f.__code__.co_code, co_code) + + def test_setting_lineno_warns_and_assigns_none_1(self): + code = textwrap.dedent(f"""\ + def f(): + x = y = 2 if not x: return 4 for i in range(55): @@ -840,7 +872,41 @@ def trace(frame, event, arg): sys.settrace(None) return None return trace - e = r"cannot leave local name 'x' unbound \(assigning None\)" + e = r"assigning None to 1 unbound local" + with self.assertWarnsRegex(RuntimeWarning, e): + sys.settrace(trace) + result = f() + self.assertEqual(result, 4) + self.assertInBytecode(f, "LOAD_FAST") + self.assertNotInBytecode(f, "LOAD_FAST_CHECK") + self.assertEqual(f.__code__.co_code, co_code) + + def test_setting_lineno_warns_and_assigns_none_2(self): + code = textwrap.dedent(f"""\ + def f(): + x = y = 2 + if not x: + return 4 + for i in range(55): + x + 6 + del x, y + L = 8 + L = 9 + L = 10 + """) + ns = {} + exec(code, ns) + f = ns['f'] + self.assertInBytecode(f, "LOAD_FAST") + self.assertNotInBytecode(f, "LOAD_FAST_CHECK") + co_code = f.__code__.co_code + def trace(frame, event, arg): + if event == 'line' and frame.f_lineno == 9: + frame.f_lineno = 3 + sys.settrace(None) + return None + return trace + e = r"assigning None to 2 unbound locals" with self.assertWarnsRegex(RuntimeWarning, e): sys.settrace(trace) result = f() @@ -867,7 +933,7 @@ def f(): self.assertNotInBytecode(f, "LOAD_FAST_CHECK") return f - def test_deleting_local_adds_check(self): + def test_deleting_local_warns_and_assigns_none(self): f = self.make_function_with_no_checks() co_code = f.__code__.co_code def trace(frame, event, arg): @@ -876,7 +942,7 @@ def trace(frame, event, arg): sys.settrace(None) return None return trace - e = r"cannot leave local name 'x' unbound \(assigning None\)" + e = r"assigning None to unbound local 'x'" with self.assertWarnsRegex(RuntimeWarning, e): sys.settrace(trace) f() diff --git a/Objects/frameobject.c b/Objects/frameobject.c index 8c8f61f177aa0a..0d0c1d268e845e 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -620,49 +620,6 @@ _PyFrame_GetState(PyFrameObject *frame) Py_UNREACHABLE(); } -static bool -op_exists(PyCodeObject *co, int opcode, int oparg, int start, int stop) -{ - // This only works when opcode is a non-quickened form: - assert(_PyOpcode_Deopt[opcode] == opcode); - int check_oparg = 0; - _Py_CODEUNIT *instructions = _PyCode_CODE(co); - while (start < stop) { - int check_opcode = _PyOpcode_Deopt[_Py_OPCODE(instructions[start])]; - check_oparg |= _Py_OPARG(instructions[start]); - if (check_opcode == opcode && check_oparg == oparg) { - return true; - } - if (check_opcode == EXTENDED_ARG) { - check_oparg <<= 8; - } - else { - check_oparg = 0; - } - start += 1 + _PyOpcode_Caches[check_opcode]; - } - return false; -} - -static void -maybe_bind_fast_local(_PyInterpreterFrame *f, int i) -{ - PyCodeObject *co = f->f_code; - if (f->localsplus[i] == NULL && op_exists(co, LOAD_FAST, i, 0, Py_SIZE(co))) - { - // We're being overly-cautious here (it's too expensive to run a full - // flow analysis), but at least one instruction expects i to be bound. - // Rather than crashing (or changing co_code), just bind None instead: - const char *e = "cannot leave local name %R unbound (assigning None)"; - PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); - if (PyErr_WarnFormat(PyExc_RuntimeWarning, 0, e, name)) { - // It's okay if frame_obj is NULL, just try anyways: - PyErr_WriteUnraisable((PyObject *)f->frame_obj); - } - f->localsplus[i] = Py_NewRef(Py_None); - } -} - /* Setter for f_lineno - you can set f_lineno from within a trace function in * order to jump to a given line of code, subject to some restrictions. Most * lines are OK to jump to because they don't make any assumptions about the @@ -831,9 +788,30 @@ frame_setlineno(PyFrameObject *f, PyObject* p_new_lineno, void *Py_UNUSED(ignore } start_stack = pop_value(start_stack); } - // Populate any NULL locals that might be needed: + // Populate any NULL locals that the compiler might have "proven" to exist + // in the new location. Rather than crashing or changing co_code, just bind + // None instead: + int empty = 0; for (int i = 0; i < f->f_frame->f_code->co_nlocalsplus; i++) { - maybe_bind_fast_local(f->f_frame, i); + // Counting every unbound local is overly-cautious, but a full flow + // analysis (like we do in the compiler) is probably too expensive: + empty += f->f_frame->localsplus[i] == NULL; + } + if (empty) { + const char *e = "assigning None to %d unbound local%s"; + const char *s = (empty == 1) ? "" : "s"; + if (PyErr_WarnFormat(PyExc_RuntimeWarning, 0, e, empty, s)) { + return -1; + } + // Do this in a second pass to avoid writing a bunch of Nones when + // warnings are being treated as errors and the previous bit raises: + for (int i = 0; i < f->f_frame->f_code->co_nlocalsplus; i++) { + if (f->f_frame->localsplus[i] == NULL) { + f->f_frame->localsplus[i] = Py_NewRef(Py_None); + empty--; + } + } + assert(empty == 0); } /* Finally set the new lasti and return OK. */ f->f_lineno = 0; @@ -1117,6 +1095,31 @@ PyFrame_New(PyThreadState *tstate, PyCodeObject *code, return f; } +static int +_PyFrame_OpAlreadyRan(_PyInterpreterFrame *frame, int opcode, int oparg) +{ + // This only works when opcode is a non-quickened form: + assert(_PyOpcode_Deopt[opcode] == opcode); + int check_oparg = 0; + for (_Py_CODEUNIT *instruction = _PyCode_CODE(frame->f_code); + instruction < frame->prev_instr; instruction++) + { + int check_opcode = _PyOpcode_Deopt[_Py_OPCODE(*instruction)]; + check_oparg |= _Py_OPARG(*instruction); + if (check_opcode == opcode && check_oparg == oparg) { + return 1; + } + if (check_opcode == EXTENDED_ARG) { + check_oparg <<= 8; + } + else { + check_oparg = 0; + } + instruction += _PyOpcode_Caches[check_opcode]; + } + return 0; +} + int _PyFrame_FastToLocalsWithError(_PyInterpreterFrame *frame) { /* Merge fast locals into f->f_locals */ @@ -1179,8 +1182,7 @@ _PyFrame_FastToLocalsWithError(_PyInterpreterFrame *frame) { // run yet. if (value != NULL) { if (PyCell_Check(value) && - op_exists(co, MAKE_CELL, i, 0, lasti)) - { + _PyFrame_OpAlreadyRan(frame, MAKE_CELL, i)) { // (likely) MAKE_CELL must have executed already. value = PyCell_GET(value); } @@ -1254,7 +1256,6 @@ _PyFrame_LocalsToFast(_PyInterpreterFrame *frame, int clear) } fast = _PyFrame_GetLocalsArray(frame); co = frame->f_code; - int lasti = _PyInterpreterFrame_LASTI(frame); PyErr_Fetch(&error_type, &error_value, &error_traceback); for (int i = 0; i < co->co_nlocalsplus; i++) { @@ -1283,8 +1284,8 @@ _PyFrame_LocalsToFast(_PyInterpreterFrame *frame, int clear) } else if (kind & CO_FAST_CELL && oldvalue != NULL) { /* Same test as in PyFrame_FastToLocals() above. */ - if (PyCell_Check(oldvalue) && op_exists(co, MAKE_CELL, i, 0, lasti)) - { + if (PyCell_Check(oldvalue) && + _PyFrame_OpAlreadyRan(frame, MAKE_CELL, i)) { // (likely) MAKE_CELL must have executed already. cell = oldvalue; } @@ -1300,10 +1301,18 @@ _PyFrame_LocalsToFast(_PyInterpreterFrame *frame, int clear) } } else if (value != oldvalue) { + if (value == NULL) { + // Probably can't delete this, since the compiler's flow + // analysis may have already "proved" that it exists here: + const char *e = "assigning None to unbound local %R"; + if (PyErr_WarnFormat(PyExc_RuntimeWarning, 0, e, name)) { + // It's okay if frame_obj is NULL, just try anyways: + PyErr_WriteUnraisable((PyObject *)frame->frame_obj); + } + value = Py_NewRef(Py_None); + } Py_XINCREF(value); Py_XSETREF(fast[i], value); - // Populate any NULL locals that might be needed: - maybe_bind_fast_local(frame, i); } Py_XDECREF(value); } From da0723066dee232b106be98c39e2cc58f0d0850f Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 20 Oct 2022 14:39:58 -0700 Subject: [PATCH 4/6] fixup --- Objects/frameobject.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Objects/frameobject.c b/Objects/frameobject.c index 0d0c1d268e845e..4902fe13b0c021 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -1182,7 +1182,7 @@ _PyFrame_FastToLocalsWithError(_PyInterpreterFrame *frame) { // run yet. if (value != NULL) { if (PyCell_Check(value) && - _PyFrame_OpAlreadyRan(frame, MAKE_CELL, i)) { + _PyFrame_OpAlreadyRan(frame, MAKE_CELL, i)) { // (likely) MAKE_CELL must have executed already. value = PyCell_GET(value); } @@ -1303,7 +1303,7 @@ _PyFrame_LocalsToFast(_PyInterpreterFrame *frame, int clear) else if (value != oldvalue) { if (value == NULL) { // Probably can't delete this, since the compiler's flow - // analysis may have already "proved" that it exists here: + // analysis may have already "proven" that it exists here: const char *e = "assigning None to unbound local %R"; if (PyErr_WarnFormat(PyExc_RuntimeWarning, 0, e, name)) { // It's okay if frame_obj is NULL, just try anyways: From 555d23ed6860f2593db166f2ed9358cae693d8b0 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 20 Oct 2022 14:58:35 -0700 Subject: [PATCH 5/6] Perform assignments *before* adjusting the stack --- Objects/frameobject.c | 50 +++++++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/Objects/frameobject.c b/Objects/frameobject.c index 4902fe13b0c021..02d4ea0e527ab5 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -767,6 +767,31 @@ frame_setlineno(PyFrameObject *f, PyObject* p_new_lineno, void *Py_UNUSED(ignore PyErr_SetString(PyExc_ValueError, msg); return -1; } + // Populate any NULL locals that the compiler might have "proven" to exist + // in the new location. Rather than crashing or changing co_code, just bind + // None instead: + int unbound = 0; + for (int i = 0; i < f->f_frame->f_code->co_nlocalsplus; i++) { + // Counting every unbound local is overly-cautious, but a full flow + // analysis (like we do in the compiler) is probably too expensive: + unbound += f->f_frame->localsplus[i] == NULL; + } + if (unbound) { + const char *e = "assigning None to %d unbound local%s"; + const char *s = (unbound == 1) ? "" : "s"; + if (PyErr_WarnFormat(PyExc_RuntimeWarning, 0, e, unbound, s)) { + return -1; + } + // Do this in a second pass to avoid writing a bunch of Nones when + // warnings are being treated as errors and the previous bit raises: + for (int i = 0; i < f->f_frame->f_code->co_nlocalsplus; i++) { + if (f->f_frame->localsplus[i] == NULL) { + f->f_frame->localsplus[i] = Py_NewRef(Py_None); + unbound--; + } + } + assert(unbound == 0); + } if (state == FRAME_SUSPENDED) { /* Account for value popped by yield */ start_stack = pop_value(start_stack); @@ -788,31 +813,6 @@ frame_setlineno(PyFrameObject *f, PyObject* p_new_lineno, void *Py_UNUSED(ignore } start_stack = pop_value(start_stack); } - // Populate any NULL locals that the compiler might have "proven" to exist - // in the new location. Rather than crashing or changing co_code, just bind - // None instead: - int empty = 0; - for (int i = 0; i < f->f_frame->f_code->co_nlocalsplus; i++) { - // Counting every unbound local is overly-cautious, but a full flow - // analysis (like we do in the compiler) is probably too expensive: - empty += f->f_frame->localsplus[i] == NULL; - } - if (empty) { - const char *e = "assigning None to %d unbound local%s"; - const char *s = (empty == 1) ? "" : "s"; - if (PyErr_WarnFormat(PyExc_RuntimeWarning, 0, e, empty, s)) { - return -1; - } - // Do this in a second pass to avoid writing a bunch of Nones when - // warnings are being treated as errors and the previous bit raises: - for (int i = 0; i < f->f_frame->f_code->co_nlocalsplus; i++) { - if (f->f_frame->localsplus[i] == NULL) { - f->f_frame->localsplus[i] = Py_NewRef(Py_None); - empty--; - } - } - assert(empty == 0); - } /* Finally set the new lasti and return OK. */ f->f_lineno = 0; f->f_frame->prev_instr = _PyCode_CODE(f->f_frame->f_code) + best_addr; From 4e597b398730f1960b5a33fd6ae0b32142c288d9 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 20 Oct 2022 20:40:13 -0700 Subject: [PATCH 6/6] Address code review --- Lib/test/test_peepholer.py | 6 +++--- .../2022-10-19-23-54-43.gh-issue-93143.1wCYub.rst | 2 +- Objects/frameobject.c | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_peepholer.py b/Lib/test/test_peepholer.py index aa79f6d2cafc54..0d398fc3030948 100644 --- a/Lib/test/test_peepholer.py +++ b/Lib/test/test_peepholer.py @@ -815,7 +815,7 @@ def f(): self.assertInBytecode(f, 'LOAD_FAST_CHECK', "a73") self.assertInBytecode(f, 'LOAD_FAST', "a73") - def test_setting_lineno_warns_and_assigns_none_0(self): + def test_setting_lineno_no_undefined(self): code = textwrap.dedent(f"""\ def f(): x = y = 2 @@ -847,7 +847,7 @@ def trace(frame, event, arg): self.assertNotInBytecode(f, "LOAD_FAST_CHECK") self.assertEqual(f.__code__.co_code, co_code) - def test_setting_lineno_warns_and_assigns_none_1(self): + def test_setting_lineno_one_undefined(self): code = textwrap.dedent(f"""\ def f(): x = y = 2 @@ -881,7 +881,7 @@ def trace(frame, event, arg): self.assertNotInBytecode(f, "LOAD_FAST_CHECK") self.assertEqual(f.__code__.co_code, co_code) - def test_setting_lineno_warns_and_assigns_none_2(self): + def test_setting_lineno_two_undefined(self): code = textwrap.dedent(f"""\ def f(): x = y = 2 diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-10-19-23-54-43.gh-issue-93143.1wCYub.rst b/Misc/NEWS.d/next/Core and Builtins/2022-10-19-23-54-43.gh-issue-93143.1wCYub.rst index 934596876380e8..779ff6a543ec80 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2022-10-19-23-54-43.gh-issue-93143.1wCYub.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2022-10-19-23-54-43.gh-issue-93143.1wCYub.rst @@ -1,4 +1,4 @@ Rather than changing :attr:`~types.CodeType.co_code`, the interpreter will now display a :exc:`RuntimeWarning` and assign :const:`None` to any fast -locals that are incorrectly left unbound after jumps or :keyword:`del` +locals that are left unbound after jumps or :keyword:`del` statements executed while tracing. diff --git a/Objects/frameobject.c b/Objects/frameobject.c index 02d4ea0e527ab5..4b4be382d94366 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -1311,7 +1311,7 @@ _PyFrame_LocalsToFast(_PyInterpreterFrame *frame, int clear) } value = Py_NewRef(Py_None); } - Py_XINCREF(value); + Py_INCREF(value); Py_XSETREF(fast[i], value); } Py_XDECREF(value);