Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork34.1k
Comments
GH-116090: Fix test and clarify behavior for exception events when exhausting a generator.#120697
Conversation
markshannon commentedJun 18, 2024
Skipping news as nothing has changed here. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
brandtbucher commentedJul 26, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@markshannon, this changed test is failing on tier two builds: Traceback (most recent call last): File"/Users/brandtbucher/cpython/Lib/test/test_monitoring.py", line864, intest_implicit_stop_iterationself.check_events(implicit_stop_iteration, expected,recorders=recorders)~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File"/Users/brandtbucher/cpython/Lib/test/test_monitoring.py", line749, incheck_eventsself.assertEqual(events, expected)~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^AssertionError:Lists differ: [] != [('raise', <class 'StopIteration'>)] If I understand correctly, when events are set globally, we correctly invalidate all executors. However, we don't prevent the creation and entry of new ones. This patch fixes the issue for me... does it look correct? diff --git a/Python/optimizer.c b/Python/optimizer.cindex f0793b8c8f2..433e04b382e 100644--- a/Python/optimizer.c+++ b/Python/optimizer.c@@ -181,6 +181,9 @@ _PyOptimizer_Optimize( PyCodeObject *code = _PyFrame_GetCode(frame); assert(PyCode_Check(code)); PyInterpreterState *interp = _PyInterpreterState_GET();+ if (_PyCode_HAS_INSTRUMENTATION(code)) {+ return 0;+ } if (!has_space_for_executor(code, start)) { return 0; } I'm not sure, but Ithink that we should also be invalidating some executors when local events are set. It's not needed to fix the test, just something that I noticed while poking around the instrumentation code. Something like this? diff --git a/Python/instrumentation.c b/Python/instrumentation.cindex ae790a1441b..a4269055777 100644--- a/Python/instrumentation.c+++ b/Python/instrumentation.c@@ -1990,6 +1990,9 @@ _PyMonitoring_SetLocalEvents(PyCodeObject *code, int tool_id, _PyMonitoringEvent goto done; } set_local_events(local, tool_id, events);+#ifdef _Py_TIER2+ _Py_Executors_InvalidateDependency(interp, code, 1);+#endif res = force_instrument_lock_held(code, interp); |
markshannon commentedJul 29, 2024
We want to optimize code around instrumentation, so just because a code object has some instrumentation should prevent us from executing other parts of it in tier 2. This is important for breakpoints, which might sit on non-optimized paths. Adding instrumentation should invalidate an executors covering that instrumentation. So, to answer your question: BTW, don't use |
brandtbucher commentedJul 29, 2024
The second patch isn't sufficient to fix the failing test (though I still think we should do it). The problem is that in this case, diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.hindex fac4a4d2280..52d3f992f96 100644--- a/Include/internal/pycore_ceval.h+++ b/Include/internal/pycore_ceval.h@@ -269,6 +269,7 @@ PyAPI_FUNC(PyObject **) _PyObjectArray_FromStackRefArray(_PyStackRef *input, Py_ PyAPI_FUNC(void) _PyObjectArray_Free(PyObject **array, PyObject **scratch);+PyAPI_FUNC(void) monitor_raise(PyThreadState *tstate, _PyInterpreterFrame *frame, _Py_CODEUNIT *instr); /* Bits that can be set in PyThreadState.eval_breaker */ #define _PY_GIL_DROP_REQUEST_BIT (1U << 0)diff --git a/Python/bytecodes.c b/Python/bytecodes.cindex d74f2aae048..9b513911ce1 100644--- a/Python/bytecodes.c+++ b/Python/bytecodes.c@@ -2849,6 +2849,7 @@ dummy_func( if (!_PyErr_ExceptionMatches(tstate, PyExc_StopIteration)) { ERROR_NO_POP(); }+ monitor_raise(tstate, frame, frame->instr_ptr); _PyErr_Clear(tstate); } /* iterator ended normally */diff --git a/Python/ceval.c b/Python/ceval.cindex c0074c45b27..9890e1dc813 100644--- a/Python/ceval.c+++ b/Python/ceval.c@@ -225,9 +225,6 @@ maybe_lltrace_resume_frame(_PyInterpreterFrame *frame, _PyInterpreterFrame *skip #endif-static void monitor_raise(PyThreadState *tstate,- _PyInterpreterFrame *frame,- _Py_CODEUNIT *instr); static void monitor_reraise(PyThreadState *tstate, _PyInterpreterFrame *frame, _Py_CODEUNIT *instr);@@ -2200,7 +2197,7 @@ no_tools_for_local_event(PyThreadState *tstate, _PyInterpreterFrame *frame, int } }-static void+void monitor_raise(PyThreadState *tstate, _PyInterpreterFrame *frame, _Py_CODEUNIT *instr) {diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.hindex 6e3f6cc62fe..455ca713707 100644--- a/Python/executor_cases.c.h+++ b/Python/executor_cases.c.h@@ -3173,6 +3173,7 @@ if (!_PyErr_ExceptionMatches(tstate, PyExc_StopIteration)) { JUMP_TO_ERROR(); }+ monitor_raise(tstate, frame, frame->instr_ptr); _PyErr_Clear(tstate); } /* iterator ended normally */ The reason I did I'd really like to get this fixed because it's blocking all of my tier two work right now. |
Uh oh!
There was an error while loading.Please reload this page.
STOP_ITERATIONmonitoring event is not triggered by unspecialized bytecode #116090📚 Documentation preview 📚:https://cpython-previews--120697.org.readthedocs.build/