Emit the exception from a pending call

- adds a new trace event type ("opcode") for
  more reliable injection of the error
- moves the error injection into a C helper
  function so it actually happens in the frame
  of interest immediately after the offset of
  interest

Author: ncoghlan

Lib/test/test_with_signal_safety.py

@@ -2,6 +2,7 @@
 """
 
 from test.support import cpython_only, verbose
+from _testcapi import install_error_injection_hook
 import asyncio
 import dis
 import sys
@@ -11,24 +12,23 @@ class InjectedException(Exception):
     """Exception injected into a running frame via a trace function"""
     pass
 
-def raise_after_instruction(target_function, target_instruction):
+def raise_after_offset(target_function, target_offset):
     """Sets a trace function to inject an exception into given function
 
     Relies on the ability to request that a trace function be called for
     every executed opcode, not just every line
     """
     target_code = target_function.__code__
-    def inject_exception(frame, event, arg):
-        if frame.f_code is not target_code:
-            return
-        frame.f_trace_opcodes = True
-        if frame.f_lasti >= target_instruction:
-            if frame.f_lasti > frame.f_pendingi:
-                raise InjectedException(f"Failing after {frame.f_lasti}")
-        return inject_exception
-    sys.settrace(inject_exception)
-
-# TODO: Add a test case that ensures raise_after_instruction is working
+    def inject_exception():
+        print("Raising injected exception")
+        raise InjectedException(f"Failing after {target_offset}")
+    # This installs a trace hook that's implemented in C, and hence won't
+    # trigger any of the per-bytecode processing in the eval loop
+    # This means it can register the pending call that raises the exception and
+    # the pending call won't be processed until after the trace hook returns
+    install_error_injection_hook(target_code, target_offset, inject_exception)
+
+# TODO: Add a test case that ensures raise_after_offset is working
 # properly (otherwise there's a risk the tests will pass due to the
 # exception not being injected properly)
 
@@ -51,11 +51,11 @@ def setUp(self):
         self.addCleanup(sys.settrace, old_trace)
         sys.settrace(None)
 
-    def assert_cm_exited(self, tracking_cm, target_instruction, traced_operation):
+    def assert_cm_exited(self, tracking_cm, target_offset, traced_operation):
         if tracking_cm.enter_without_exit:
             msg = ("Context manager entered without exit due to "
-            f"exception injected at offset {target_instruction} in:\n"
-            f"{dis.Bytecode(traced_operation).dis()}")
+                  f"exception injected at offset {target_offset} in:\n"
+                  f"{dis.Bytecode(traced_operation).dis()}")
             self.fail(msg)
 
     def test_synchronous_cm(self):
@@ -71,21 +71,21 @@ def traced_function():
             with tracking_cm:
                 1 + 1
             return
-        target_instruction = -1
-        num_instructions = len(traced_function.__code__.co_code) - 2
-        while target_instruction < num_instructions:
-            target_instruction += 1
-            raise_after_instruction(traced_function, target_instruction)
+        target_offset = -1
+        max_offset = len(traced_function.__code__.co_code) - 2
+        while target_offset < max_offset:
+            target_offset += 1
+            raise_after_offset(traced_function, target_offset)
             try:
                 traced_function()
             except InjectedException:
                 # key invariant: if we entered the CM, we exited it
-                self.assert_cm_exited(tracking_cm, target_instruction, traced_function)
+                self.assert_cm_exited(tracking_cm, target_offset, traced_function)
             else:
-                self.fail(f"Exception wasn't raised @{target_instruction}")
+                self.fail(f"Exception wasn't raised @{target_offset}")
 
 
-    def test_asynchronous_cm(self):
+    def _test_asynchronous_cm(self):
         class AsyncTrackingCM():
             def __init__(self):
                 self.enter_without_exit = None
@@ -98,19 +98,19 @@ async def traced_coroutine():
             async with tracking_cm:
                 1 + 1
             return
-        target_instruction = -1
-        num_instructions = len(traced_coroutine.__code__.co_code) - 2
+        target_offset = -1
+        max_offset = len(traced_coroutine.__code__.co_code) - 2
         loop = asyncio.get_event_loop()
-        while target_instruction < num_instructions:
-            target_instruction += 1
-            raise_after_instruction(traced_coroutine, target_instruction)
+        while target_offset < max_offset:
+            target_offset += 1
+            raise_after_offset(traced_coroutine, target_offset)
             try:
                 loop.run_until_complete(traced_coroutine())
             except InjectedException:
                 # key invariant: if we entered the CM, we exited it
-                self.assert_cm_exited(tracking_cm, target_instruction, traced_coroutine)
+                self.assert_cm_exited(tracking_cm, target_offset, traced_coroutine)
             else:
-                self.fail(f"Exception wasn't raised @{target_instruction}")
+                self.fail(f"Exception wasn't raised @{target_offset}")
 
 
 if __name__ == '__main__':

Modules/_testcapimodule.c

@@ -12,6 +12,7 @@
 #include "structmember.h"
 #include "datetime.h"
 #include "marshal.h"
+#include "frameobject.h"
 #include <signal.h>
 
 #ifdef MS_WINDOWS
@@ -2345,6 +2346,58 @@ PyObject *pending_threadfunc(PyObject *self, PyObject *arg)
     Py_RETURN_TRUE;
 }
 
+/* Helper for test_with_signal_safety that injects errors into the eval loop's
+ * pending call handling at a designated bytecode offset
+ *
+ * The hook args indicate the code object where the pending call should be
+ * injected, the offset where it should be registered, and the callback itself
+ */
+static int
+error_injection_trace(PyObject *hook_args, PyFrameObject *frame,
+                      int what, PyObject *event_arg)
+{
+    PyObject *target_code, *callback;
+    int target_offset;
+
+    if (!PyArg_ParseTuple(hook_args, "OiO:error_injection_trace",
+                          &target_code, &target_offset, &callback)) {
+        PyEval_SetTrace(NULL, NULL);
+        return -1;
+    }
+
+    if (((PyObject *) frame->f_code) == target_code) {
+        printf("Tracing frame of interest\n");
+        frame->f_trace_opcodes = 1;
+        if (what == PyTrace_OPCODE && frame->f_lasti > target_offset) {
+            printf("Adding pending call after %d\n", frame->f_lasti);
+            Py_INCREF(callback);
+            if (Py_AddPendingCall(&_pending_callback, callback) < 0) {
+                printf("Failed to add pending call\n");
+                Py_DECREF(callback);
+                PyEval_SetTrace(NULL, NULL);
+                return -1;
+            }
+            PyEval_SetTrace(NULL, NULL);
+        }
+    }
+    return 0;
+}
+
+PyObject *install_error_injection_hook(PyObject *self, PyObject *args)
+{
+    PyObject *target_code, *target_offset, *callback;
+
+    /* Check the args are as expected */
+    if (!PyArg_UnpackTuple(args, "install_error_injection_hook", 3, 3,
+                           &target_code, &target_offset, &callback)) {
+        return NULL;
+    }
+    printf("Registering trace hook\n");
+
+    PyEval_SetTrace(error_injection_trace, args);
+    Py_RETURN_NONE;
+}
+
 /* Some tests of PyUnicode_FromFormat().  This needs more tests. */
 static PyObject *
 test_string_from_format(PyObject *self, PyObject *args)
@@ -4415,6 +4468,7 @@ static PyMethodDef TestMethods[] = {
     {"unicode_legacy_string",   unicode_legacy_string,           METH_VARARGS},
     {"_test_thread_state",      test_thread_state,               METH_VARARGS},
     {"_pending_threadfunc",     pending_threadfunc,              METH_VARARGS},
+    {"install_error_injection_hook", install_error_injection_hook, METH_VARARGS},
 #ifdef HAVE_GETTIMEOFDAY
     {"profile_int",             profile_int,                     METH_NOARGS},
 #endif
@@ -4933,5 +4987,6 @@ PyInit__testcapi(void)
     TestError = PyErr_NewException("_testcapi.error", NULL, NULL);
     Py_INCREF(TestError);
     PyModule_AddObject(m, "error", TestError);
+
     return m;
 }

Python/ceval.c

@@ -981,12 +981,18 @@ _PyEval_EvalFrameDefault(PyFrameObject *f, int throwflag)
                 */
                 goto fast_next_opcode;
             }
-            if ((next_instr > handle_pending_after) &&
-                    _Py_atomic_load_relaxed(&pendingcalls_to_do)) {
-                if (Py_MakePendingCalls() < 0)
-                    goto error;
+            if (handle_pending_after > first_instr) {
+                printf("Pending calls deferred until %lu\n", DEFER_OFFSET());
+            }
+            if (next_instr >= handle_pending_after) {
+                printf("Checking for pending calls: %lu > %lu?\n", INSTR_OFFSET(), DEFER_OFFSET());
                 /* Allow for subsequent jumps backwards in the bytecode */
                 handle_pending_after = first_instr;
+                if (_Py_atomic_load_relaxed(&pendingcalls_to_do)) {
+                    printf("  Processing pending calls\n");
+                    if (Py_MakePendingCalls() < 0)
+                        goto error;
+                }
             }
             if (_Py_atomic_load_relaxed(&gil_drop_request)) {
                 /* Give another thread a chance */