GH-108976. Keep monitoring data structures valid during de-optimization during callback. (GH-109131)

Author: markshannon

Lib/test/test_monitoring.py

@@ -1719,6 +1719,14 @@ def make_foo_optimized_then_set_event():
         finally:
             sys.monitoring.set_events(TEST_TOOL, 0)
 
+    def test_gh108976(self):
+        sys.monitoring.use_tool_id(0, "test")
+        sys.monitoring.set_events(0, 0)
+        sys.monitoring.register_callback(0, E.LINE, lambda *args: sys.monitoring.set_events(0, 0))
+        sys.monitoring.register_callback(0, E.INSTRUCTION, lambda *args: 0)
+        sys.monitoring.set_events(0, E.LINE | E.INSTRUCTION)
+        sys.monitoring.set_events(0, 0)
+
 
 class TestOptimizer(MonitoringTestBase, unittest.TestCase):
 

Lib/test/test_pdb.py

@@ -2297,6 +2297,24 @@ def test_pdb_issue_gh_101517():
     (Pdb) continue
     """
 
+def test_pdb_issue_gh_108976():
+    """See GH-108976
+    Make sure setting f_trace_opcodes = True won't crash pdb
+    >>> def test_function():
+    ...     import sys
+    ...     sys._getframe().f_trace_opcodes = True
+    ...     import pdb; pdb.Pdb(nosigint=True, readrc=False).set_trace()
+    ...     a = 1
+    >>> with PdbTestInput([  # doctest: +NORMALIZE_WHITESPACE
+    ...     'continue'
+    ... ]):
+    ...    test_function()
+    bdb.Bdb.dispatch: unknown debugging event: 'opcode'
+    > <doctest test.test_pdb.test_pdb_issue_gh_108976[0]>(5)test_function()
+    -> a = 1
+    (Pdb) continue
+    """
+
 def test_pdb_ambiguous_statements():
     """See GH-104301
 

Misc/NEWS.d/next/Core and Builtins/2023-09-06-22-50-25.gh-issue-108976.MUKaIJ.rst

@@ -0,0 +1,2 @@
+Fix crash that occurs after de-instrumenting a code object in a monitoring
+callback.

Python/instrumentation.c

@@ -364,45 +364,21 @@ dump_instrumentation_data_per_instruction(PyCodeObject *code, _PyCoMonitoringDat
 }
 
 static void
-dump_monitors(const char *prefix, _Py_Monitors monitors, FILE*out)
+dump_global_monitors(const char *prefix, _Py_GlobalMonitors monitors, FILE*out)
 {
     fprintf(out, "%s monitors:\n", prefix);
     for (int event = 0; event < _PY_MONITORING_UNGROUPED_EVENTS; event++) {
         fprintf(out, "    Event %d: Tools %x\n", event, monitors.tools[event]);
     }
 }
 
-/* Like _Py_GetBaseOpcode but without asserts.
- * Does its best to give the right answer, but won't abort
- * if something is wrong */
-static int
-get_base_opcode_best_attempt(PyCodeObject *code, int offset)
+static void
+dump_local_monitors(const char *prefix, _Py_LocalMonitors monitors, FILE*out)
 {
-    int opcode = _Py_OPCODE(_PyCode_CODE(code)[offset]);
-    if (INSTRUMENTED_OPCODES[opcode] != opcode) {
-        /* Not instrumented */
-        return _PyOpcode_Deopt[opcode] == 0 ? opcode : _PyOpcode_Deopt[opcode];
-    }
-    if (opcode == INSTRUMENTED_INSTRUCTION) {
-        if (code->_co_monitoring->per_instruction_opcodes[offset] == 0) {
-            return opcode;
-        }
-        opcode = code->_co_monitoring->per_instruction_opcodes[offset];
-    }
-    if (opcode == INSTRUMENTED_LINE) {
-        if (code->_co_monitoring->lines[offset].original_opcode == 0) {
-            return opcode;
-        }
-        opcode = code->_co_monitoring->lines[offset].original_opcode;
-    }
-    int deinstrumented = DE_INSTRUMENT[opcode];
-    if (deinstrumented) {
-        return deinstrumented;
-    }
-    if (_PyOpcode_Deopt[opcode] == 0) {
-        return opcode;
+    fprintf(out, "%s monitors:\n", prefix);
+    for (int event = 0; event < _PY_MONITORING_LOCAL_EVENTS; event++) {
+        fprintf(out, "    Event %d: Tools %x\n", event, monitors.tools[event]);
     }
-    return _PyOpcode_Deopt[opcode];
 }
 
 /* No error checking -- Don't use this for anything but experimental debugging */
@@ -417,9 +393,9 @@ dump_instrumentation_data(PyCodeObject *code, int star, FILE*out)
         fprintf(out, "NULL\n");
         return;
     }
-    dump_monitors("Global", _PyInterpreterState_GET()->monitors, out);
-    dump_monitors("Code", data->local_monitors, out);
-    dump_monitors("Active", data->active_monitors, out);
+    dump_global_monitors("Global", _PyInterpreterState_GET()->monitors, out);
+    dump_local_monitors("Code", data->local_monitors, out);
+    dump_local_monitors("Active", data->active_monitors, out);
     int code_len = (int)Py_SIZE(code);
     bool starred = false;
     for (int i = 0; i < code_len; i += _PyInstruction_GetLength(code, i)) {
@@ -458,6 +434,9 @@ dump_instrumentation_data(PyCodeObject *code, int star, FILE*out)
 static bool
 valid_opcode(int opcode)
 {
+    if (opcode == INSTRUMENTED_LINE) {
+        return true;
+    }
     if (IS_VALID_OPCODE(opcode) &&
         opcode != CACHE &&
         opcode != RESERVED &&
@@ -475,18 +454,23 @@ sanity_check_instrumentation(PyCodeObject *code)
     if (data == NULL) {
         return;
     }
-    _Py_GlobalMonitors active_monitors = _PyInterpreterState_GET()->monitors;
+    _Py_GlobalMonitors global_monitors = _PyInterpreterState_GET()->monitors;
+    _Py_LocalMonitors active_monitors;
     if (code->_co_monitoring) {
-        _Py_Monitors local_monitors = code->_co_monitoring->local_monitors;
-        active_monitors = local_union(active_monitors, local_monitors);
+        _Py_LocalMonitors local_monitors = code->_co_monitoring->local_monitors;
+        active_monitors = local_union(global_monitors, local_monitors);
+    }
+    else {
+        _Py_LocalMonitors empty = (_Py_LocalMonitors) { 0 };
+        active_monitors = local_union(global_monitors, empty);
     }
     assert(monitors_equals(
         code->_co_monitoring->active_monitors,
-        active_monitors)
-    );
+        active_monitors));
     int code_len = (int)Py_SIZE(code);
     for (int i = 0; i < code_len;) {
-        int opcode = _PyCode_CODE(code)[i].op.code;
+        _Py_CODEUNIT *instr = &_PyCode_CODE(code)[i];
+        int opcode = instr->op.code;
         int base_opcode = _Py_GetBaseOpcode(code, i);
         CHECK(valid_opcode(opcode));
         CHECK(valid_opcode(base_opcode));
@@ -506,23 +490,30 @@ sanity_check_instrumentation(PyCodeObject *code)
             opcode = data->lines[i].original_opcode;
             CHECK(opcode != END_FOR);
             CHECK(opcode != RESUME);
+            CHECK(opcode != RESUME_CHECK);
             CHECK(opcode != INSTRUMENTED_RESUME);
             if (!is_instrumented(opcode)) {
                 CHECK(_PyOpcode_Deopt[opcode] == opcode);
             }
             CHECK(opcode != INSTRUMENTED_LINE);
         }
-        else if (data->lines && !is_instrumented(opcode)) {
-            CHECK(data->lines[i].original_opcode == 0 ||
-                  data->lines[i].original_opcode == base_opcode ||
-                  DE_INSTRUMENT[data->lines[i].original_opcode] == base_opcode);
+        else if (data->lines) {
+            /* If original_opcode is INSTRUMENTED_INSTRUCTION
+             * *and* we are executing a INSTRUMENTED_LINE instruction
+             * that has de-instrumented itself, then we will execute
+             * an invalid INSTRUMENTED_INSTRUCTION */
+            CHECK(data->lines[i].original_opcode != INSTRUMENTED_INSTRUCTION);
+        }
+        if (opcode == INSTRUMENTED_INSTRUCTION) {
+            CHECK(data->per_instruction_opcodes[i] != 0);
+            opcode = data->per_instruction_opcodes[i];
         }
         if (is_instrumented(opcode)) {
             CHECK(DE_INSTRUMENT[opcode] == base_opcode);
             int event = EVENT_FOR_OPCODE[DE_INSTRUMENT[opcode]];
             if (event < 0) {
                 /* RESUME fixup */
-                event = _PyCode_CODE(code)[i].op.arg;
+                event = instr->op.arg ? 1: 0;
             }
             CHECK(active_monitors.tools[event] != 0);
         }
@@ -608,30 +599,30 @@ static void
 de_instrument_line(PyCodeObject *code, int i)
 {
     _Py_CODEUNIT *instr = &_PyCode_CODE(code)[i];
-    uint8_t *opcode_ptr = &instr->op.code;
-    int opcode =*opcode_ptr;
+    int opcode = instr->op.code;
     if (opcode != INSTRUMENTED_LINE) {
         return;
     }
     _PyCoLineInstrumentationData *lines = &code->_co_monitoring->lines[i];
     int original_opcode = lines->original_opcode;
+    if (original_opcode == INSTRUMENTED_INSTRUCTION) {
+        lines->original_opcode = code->_co_monitoring->per_instruction_opcodes[i];
+    }
     CHECK(original_opcode != 0);
     CHECK(original_opcode == _PyOpcode_Deopt[original_opcode]);
-    *opcode_ptr = instr->op.code = original_opcode;
+    instr->op.code = original_opcode;
     if (_PyOpcode_Caches[original_opcode]) {
         instr[1].cache = adaptive_counter_warmup();
     }
-    assert(*opcode_ptr != INSTRUMENTED_LINE);
     assert(instr->op.code != INSTRUMENTED_LINE);
 }
 
-
 static void
 de_instrument_per_instruction(PyCodeObject *code, int i)
 {
     _Py_CODEUNIT *instr = &_PyCode_CODE(code)[i];
     uint8_t *opcode_ptr = &instr->op.code;
-    int opcode =*opcode_ptr;
+    int opcode = *opcode_ptr;
     if (opcode == INSTRUMENTED_LINE) {
         opcode_ptr = &code->_co_monitoring->lines[i].original_opcode;
         opcode = *opcode_ptr;
@@ -642,10 +633,11 @@ de_instrument_per_instruction(PyCodeObject *code, int i)
     int original_opcode = code->_co_monitoring->per_instruction_opcodes[i];
     CHECK(original_opcode != 0);
     CHECK(original_opcode == _PyOpcode_Deopt[original_opcode]);
-    instr->op.code = original_opcode;
+    *opcode_ptr = original_opcode;
     if (_PyOpcode_Caches[original_opcode]) {
         instr[1].cache = adaptive_counter_warmup();
     }
+    assert(*opcode_ptr != INSTRUMENTED_INSTRUCTION);
     assert(instr->op.code != INSTRUMENTED_INSTRUCTION);
     /* Keep things clean for sanity check */
     code->_co_monitoring->per_instruction_opcodes[i] = 0;
@@ -685,7 +677,7 @@ static void
 instrument_line(PyCodeObject *code, int i)
 {
     uint8_t *opcode_ptr = &_PyCode_CODE(code)[i].op.code;
-    int opcode =*opcode_ptr;
+    int opcode = *opcode_ptr;
     if (opcode == INSTRUMENTED_LINE) {
         return;
     }
@@ -700,13 +692,14 @@ instrument_per_instruction(PyCodeObject *code, int i)
 {
     _Py_CODEUNIT *instr = &_PyCode_CODE(code)[i];
     uint8_t *opcode_ptr = &instr->op.code;
-    int opcode =*opcode_ptr;
+    int opcode = *opcode_ptr;
     if (opcode == INSTRUMENTED_LINE) {
         _PyCoLineInstrumentationData *lines = &code->_co_monitoring->lines[i];
         opcode_ptr = &lines->original_opcode;
         opcode = *opcode_ptr;
     }
     if (opcode == INSTRUMENTED_INSTRUCTION) {
+        assert(code->_co_monitoring->per_instruction_opcodes[i] > 0);
         return;
     }
     CHECK(opcode != 0);
@@ -1129,7 +1122,6 @@ _Py_call_instrumentation_line(PyThreadState *tstate, _PyInterpreterFrame* frame,
 
     _PyCoMonitoringData *monitoring = code->_co_monitoring;
     _PyCoLineInstrumentationData *line_data = &monitoring->lines[i];
-    uint8_t original_opcode = line_data->original_opcode;
     if (tstate->tracing) {
         goto done;
     }
@@ -1212,7 +1204,9 @@ _Py_call_instrumentation_line(PyThreadState *tstate, _PyInterpreterFrame* frame,
         }
     } while (tools);
     Py_DECREF(line_obj);
+    uint8_t original_opcode;
 done:
+    original_opcode = line_data->original_opcode;
     assert(original_opcode != 0);
     assert(original_opcode != INSTRUMENTED_LINE);
     assert(_PyOpcode_Deopt[original_opcode] == original_opcode);
@@ -1655,7 +1649,9 @@ _Py_Instrument(PyCodeObject *code, PyInterpreterState *interp)
             i += _PyInstruction_GetLength(code, i);
         }
     }
-
+#ifdef INSTRUMENT_DEBUG
+    sanity_check_instrumentation(code);
+#endif
     uint8_t new_line_tools = new_events.tools[PY_MONITORING_EVENT_LINE];
     uint8_t new_per_instruction_tools = new_events.tools[PY_MONITORING_EVENT_INSTRUCTION];