GH-116090: Fix test and clarify behavior for exception events when exhausting a generator. #120697
Conversation
|
Skipping news as nothing has changed here. |
|
@markshannon, this changed test is failing on tier two builds: Traceback (most recent call last):
File "/Users/brandtbucher/cpython/Lib/test/test_monitoring.py", line 864, in test_implicit_stop_iteration
self.check_events(implicit_stop_iteration, expected, recorders=recorders)
~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/brandtbucher/cpython/Lib/test/test_monitoring.py", line 749, in check_events
self.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.c
index 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 I think 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.c
index 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); |
|
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 |
|
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.h
index 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)
8000
;
+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.c
index 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.c
index 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.h
index 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. |
STOP_ITERATIONmonitoring event is not triggered by unspecialized bytecode #116090📚 Documentation preview 📚: https://cpython-previews--120697.org.readthedocs.build/