From dd17ce9e098c3d641829c16c0a3f41964abbf7b8 Mon Sep 17 00:00:00 2001 From: Fidget-Spinner <28750310+Fidget-Spinner@users.noreply.github.com> Date: Sun, 23 Jan 2022 21:01:58 +0800 Subject: [PATCH 1/4] check eval breaker in specialized CALL opcodes --- Lib/test/support/__init__.py | 17 +++++++++++++++++ Lib/unittest/test/test_break.py | 5 +++-- .../2022-01-23-21-01-28.bpo-46465.z25Vbj.rst | 2 ++ Python/ceval.c | 9 +++++++++ 4 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2022-01-23-21-01-28.bpo-46465.z25Vbj.rst diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py index ca903d302bdd31..75ba0a924d81fb 100644 --- a/Lib/test/support/__init__.py +++ b/Lib/test/support/__init__.py @@ -56,6 +56,7 @@ "run_with_tz", "PGO", "missing_compiler_executable", "ALWAYS_EQ", "NEVER_EQ", "LARGEST", "SMALLEST", "LOOPBACK_TIMEOUT", "INTERNET_TIMEOUT", "SHORT_TIMEOUT", "LONG_TIMEOUT", + "cpython_warmup", ] @@ -2118,3 +2119,19 @@ def clear_ignored_deprecations(*tokens: object) -> None: if warnings.filters != new_filters: warnings.filters[:] = new_filters warnings._filters_mutated() + + +# This number should be CPython internal "hot" count times 2. +CPYTHON_WARMUP_COUNT = 8*2 + + +def cpython_warmup(f): + """Makes a test "hot" so that PEP 659 adaptive interpreter opcodes will + execute instead of normal opcodes. + """ + @functools.wraps(f) + def wrapper(self): + for i in range(CPYTHON_WARMUP_COUNT): + with self.subTest(i=i): + return f(self) + return wrapper diff --git a/Lib/unittest/test/test_break.py b/Lib/unittest/test/test_break.py index eebd2b610ce11f..9599e8dfa0784b 100644 --- a/Lib/unittest/test/test_break.py +++ b/Lib/unittest/test/test_break.py @@ -6,6 +6,7 @@ import weakref import unittest +from test.support import cpython_warmup @unittest.skipUnless(hasattr(os, 'kill'), "Test requires os.kill") @@ -23,7 +24,7 @@ def tearDown(self): unittest.signals._results = weakref.WeakKeyDictionary() unittest.signals._interrupt_handler = None - + @cpython_warmup def testInstallHandler(self): default_handler = signal.getsignal(signal.SIGINT) unittest.installHandler() @@ -121,7 +122,7 @@ def test(result): self.assertTrue(result2.shouldStop) self.assertFalse(result3.shouldStop) - + @cpython_warmup def testHandlerReplacedButCalled(self): # Can't use skipIf decorator because the signal handler may have # been changed after defining this method. diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-01-23-21-01-28.bpo-46465.z25Vbj.rst b/Misc/NEWS.d/next/Core and Builtins/2022-01-23-21-01-28.bpo-46465.z25Vbj.rst new file mode 100644 index 00000000000000..d0313cb78351f8 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2022-01-23-21-01-28.bpo-46465.z25Vbj.rst @@ -0,0 +1,2 @@ +Properly check for eval loop breaker in specialized ``CALL`` opcodes. Patch +by Victor Stinner and Ken Jin. diff --git a/Python/ceval.c b/Python/ceval.c index 9aaddd99edacf7..12644489a3f39c 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4731,6 +4731,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr Py_DECREF(callable); Py_DECREF(obj); SET_TOP(res); + CHECK_EVAL_BREAKER(); DISPATCH(); } @@ -4752,6 +4753,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr if (res == NULL) { goto error; } + CHECK_EVAL_BREAKER(); DISPATCH(); } @@ -4783,6 +4785,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr if (res == NULL) { goto error; } + CHECK_EVAL_BREAKER(); DISPATCH(); } @@ -4822,6 +4825,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr */ goto error; } + CHECK_EVAL_BREAKER(); DISPATCH(); } @@ -4851,6 +4855,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr if (res == NULL) { goto error; } + CHECK_EVAL_BREAKER(); DISPATCH(); } @@ -4881,6 +4886,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr if (res == NULL) { goto error; } + CHECK_EVAL_BREAKER(); DISPATCH(); } @@ -4906,6 +4912,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr STACK_SHRINK(2); SET_TOP(Py_None); Py_DECREF(callable); + CHECK_EVAL_BREAKER(); DISPATCH(); } @@ -4939,6 +4946,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr if (res == NULL) { goto error; } + CHECK_EVAL_BREAKER(); DISPATCH(); } @@ -4972,6 +4980,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr if (res == NULL) { goto error; } + CHECK_EVAL_BREAKER(); DISPATCH(); } From 055713441a0866c47ed8f9caf01bb392e2ca25dc Mon Sep 17 00:00:00 2001 From: Fidget-Spinner <28750310+Fidget-Spinner@users.noreply.github.com> Date: Sun, 23 Jan 2022 21:24:49 +0800 Subject: [PATCH 2/4] reference the C macro warmup count --- Lib/test/support/__init__.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py index 75ba0a924d81fb..249761682eeae5 100644 --- a/Lib/test/support/__init__.py +++ b/Lib/test/support/__init__.py @@ -2121,7 +2121,8 @@ def clear_ignored_deprecations(*tokens: object) -> None: warnings._filters_mutated() -# This number should be CPython internal "hot" count times 2. +# This number should be CPython internal "hot" (C macro QUICKENING_WARMUP_DELAY) +# count times 2. CPYTHON_WARMUP_COUNT = 8*2 From 2ca3d9e32d37108b0c9dd394f23dde512201042d Mon Sep 17 00:00:00 2001 From: Fidget-Spinner <28750310+Fidget-Spinner@users.noreply.github.com> Date: Sun, 23 Jan 2022 21:43:39 +0800 Subject: [PATCH 3/4] Address code review Co-Authored-By: Victor Stinner --- Lib/test/support/__init__.py | 31 +++++++++++++++++++------------ Lib/unittest/test/test_break.py | 10 +++++++--- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py index 249761682eeae5..a1db6bdf214679 100644 --- a/Lib/test/support/__init__.py +++ b/Lib/test/support/__init__.py @@ -56,7 +56,7 @@ "run_with_tz", "PGO", "missing_compiler_executable", "ALWAYS_EQ", "NEVER_EQ", "LARGEST", "SMALLEST", "LOOPBACK_TIMEOUT", "INTERNET_TIMEOUT", "SHORT_TIMEOUT", "LONG_TIMEOUT", - "cpython_warmup", + "repeat_cpython_adaptative", ] @@ -2122,17 +2122,24 @@ def clear_ignored_deprecations(*tokens: object) -> None: # This number should be CPython internal "hot" (C macro QUICKENING_WARMUP_DELAY) -# count times 2. -CPYTHON_WARMUP_COUNT = 8*2 +# count + 1. +CPYTHON_WARMUP_COUNT = 8+1 -def cpython_warmup(f): - """Makes a test "hot" so that PEP 659 adaptive interpreter opcodes will - execute instead of normal opcodes. +def repeat_cpython_adaptative(f): + """Runs a test multiple times. First with PEP 659 adaptive opcodes, then + with specialized opcodes once the opcodes turn "hot". This catches bugs + related to specialized opcodes. + + See bpo-46465 for an example bug affecting only specialized opcodes, + due to a missing CHECK_EVAL_BREAKER. """ - @functools.wraps(f) - def wrapper(self): - for i in range(CPYTHON_WARMUP_COUNT): - with self.subTest(i=i): - return f(self) - return wrapper + if check_impl_detail(cpython=True): + @functools.wraps(f) + def wrapper(self): + for i in range(CPYTHON_WARMUP_COUNT): + with self.subTest(cpython_warmup_iter=i): + return f(self) + return wrapper + else: + return f diff --git a/Lib/unittest/test/test_break.py b/Lib/unittest/test/test_break.py index 9599e8dfa0784b..195466a2ec9a47 100644 --- a/Lib/unittest/test/test_break.py +++ b/Lib/unittest/test/test_break.py @@ -6,7 +6,7 @@ import weakref import unittest -from test.support import cpython_warmup +from test.support import repeat_cpython_adaptative @unittest.skipUnless(hasattr(os, 'kill'), "Test requires os.kill") @@ -24,7 +24,9 @@ def tearDown(self): unittest.signals._results = weakref.WeakKeyDictionary() unittest.signals._interrupt_handler = None - @cpython_warmup + # Tests both adaptive and specialized opcodes for proper + # CHECK_EVAL_BREAKER(). See bpo-46465 for an example bug. + @repeat_cpython_adaptative def testInstallHandler(self): default_handler = signal.getsignal(signal.SIGINT) unittest.installHandler() @@ -122,7 +124,9 @@ def test(result): self.assertTrue(result2.shouldStop) self.assertFalse(result3.shouldStop) - @cpython_warmup + # Tests both adaptive and specialized opcodes for proper + # CHECK_EVAL_BREAKER(). See bpo-46465 for an example bug. + @repeat_cpython_adaptative def testHandlerReplacedButCalled(self): # Can't use skipIf decorator because the signal handler may have # been changed after defining this method. From d905928e87898fca711e286b454d27f2a499003b Mon Sep 17 00:00:00 2001 From: Fidget-Spinner <28750310+Fidget-Spinner@users.noreply.github.com> Date: Mon, 24 Jan 2022 23:38:04 +0800 Subject: [PATCH 4/4] Address Mark's reviews Co-Authored-By: Mark Shannon --- Lib/test/support/__init__.py | 18 +++++++++--------- Objects/codeobject.c | 12 ++++++++++++ Python/ceval.c | 3 --- 3 files changed, 21 insertions(+), 12 deletions(-) diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py index a1db6bdf214679..91cfca9f4b6226 100644 --- a/Lib/test/support/__init__.py +++ b/Lib/test/support/__init__.py @@ -2121,11 +2121,6 @@ def clear_ignored_deprecations(*tokens: object) -> None: warnings._filters_mutated() -# This number should be CPython internal "hot" (C macro QUICKENING_WARMUP_DELAY) -# count + 1. -CPYTHON_WARMUP_COUNT = 8+1 - - def repeat_cpython_adaptative(f): """Runs a test multiple times. First with PEP 659 adaptive opcodes, then with specialized opcodes once the opcodes turn "hot". This catches bugs @@ -2134,12 +2129,17 @@ def repeat_cpython_adaptative(f): See bpo-46465 for an example bug affecting only specialized opcodes, due to a missing CHECK_EVAL_BREAKER. """ - if check_impl_detail(cpython=True): + # _co_warmedup is only available in CPython + if check_impl_detail(cpython=True) and hasattr(f.__code__, "_co_warmedup"): @functools.wraps(f) def wrapper(self): - for i in range(CPYTHON_WARMUP_COUNT): - with self.subTest(cpython_warmup_iter=i): - return f(self) + done = False + while not done: + is_warmedup = f.__code__._co_warmedup + if is_warmedup: + done = True + with self.subTest(cpython_is_warmedup=is_warmedup): + f(self) return wrapper else: return f diff --git a/Objects/codeobject.c b/Objects/codeobject.c index a413b183be8edc..863ddd908a9d77 100644 --- a/Objects/codeobject.c +++ b/Objects/codeobject.c @@ -1542,6 +1542,17 @@ code_getfreevars(PyCodeObject *code, void *closure) return _PyCode_GetFreevars(code); } +static PyObject * +code_iswarmedup(PyCodeObject *code, void *closure) +{ + if (code->co_quickened != NULL) { + Py_RETURN_TRUE; + } + else { + Py_RETURN_FALSE; + } +} + static PyGetSetDef code_getsetlist[] = { {"co_lnotab", (getter)code_getlnotab, NULL, NULL}, // The following old names are kept for backward compatibility. @@ -1549,6 +1560,7 @@ static PyGetSetDef code_getsetlist[] = { {"co_varnames", (getter)code_getvarnames, NULL, NULL}, {"co_cellvars", (getter)code_getcellvars, NULL, NULL}, {"co_freevars", (getter)code_getfreevars, NULL, NULL}, + {"_co_warmedup", (getter)code_iswarmedup, NULL, NULL}, {0} }; diff --git a/Python/ceval.c b/Python/ceval.c index 12644489a3f39c..6299212ae79406 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4731,7 +4731,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr Py_DECREF(callable); Py_DECREF(obj); SET_TOP(res); - CHECK_EVAL_BREAKER(); DISPATCH(); } @@ -4855,7 +4854,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr if (res == NULL) { goto error; } - CHECK_EVAL_BREAKER(); DISPATCH(); } @@ -4912,7 +4910,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr STACK_SHRINK(2); SET_TOP(Py_None); Py_DECREF(callable); - CHECK_EVAL_BREAKER(); DISPATCH(); }