GH-109369: Exit tier 2 if executor is invalid (GH-111657)

Author: markshannon

Include/internal/pycore_opcode_metadata.h

@@ -1790,3 +1790,28 @@ def test_func(x):
         test_func(1000)
         sys.monitoring.set_local_events(TEST_TOOL, code, 0)
         self.assertEqual(sys.monitoring.get_local_events(TEST_TOOL, code), 0)
+
+class TestTier2Optimizer(CheckEvents):
+
+    def test_monitoring_already_opimized_loop(self):
+        def test_func(recorder):
+            set_events = sys.monitoring.set_events
+            line = E.LINE
+            i = 0
+            for i in range(551):
+                # Turn on events without branching once i reaches 500.
+                set_events(TEST_TOOL, line * int(i >= 500))
+                pass
+                pass
+                pass
+
+        self.assertEqual(sys.monitoring._all_events(), {})
+        events = []
+        recorder = LineRecorder(events)
+        sys.monitoring.register_callback(TEST_TOOL, E.LINE, recorder)
+        try:
+            test_func(recorder)
+        finally:
+            sys.monitoring.register_callback(TEST_TOOL, E.LINE, None)
+            sys.monitoring.set_events(TEST_TOOL, 0)
+        self.assertGreater(len(events), 250)

Lib/test/test_monitoring.py

@@ -0,0 +1 @@
+Make sure that tier 2 traces are de-optimized if the code is instrumented

Misc/NEWS.d/next/Core and Builtins/2023-11-03-22-48-29.gh-issue-109369.ELYaxJ.rst

@@ -4021,6 +4021,11 @@ dummy_func(
             memmove(&stack_pointer[-1 - oparg], &stack_pointer[-oparg], oparg * sizeof(stack_pointer[0]));
         }
 
+        op(_CHECK_VALIDITY, (--)) {
+            TIER_TWO_ONLY
+            DEOPT_IF(!current_executor->base.vm_data.valid);
+        }
+
 
 // END BYTECODES //
 

Python/abstract_interp_cases.c.h

@@ -1057,7 +1057,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
 deoptimize:
     // On DEOPT_IF we just repeat the last instruction.
     // This presumes nothing was popped from the stack (nor pushed).
-    DPRINTF(2, "DEOPT: [Opcode %d, operand %" PRIu64 "]\n", opcode, operand);
+    DPRINTF(2, "DEOPT: [Opcode %d, operand %" PRIu64 " @ %d]\n", opcode, operand, (int)(next_uop-current_executor->trace-1));
     OPT_HIST(trace_uop_execution_counter, trace_run_length_hist);
     UOP_STAT_INC(opcode, miss);
     frame->return_offset = 0;  // Dispatch to frame->instr_ptr

Python/bytecodes.c

@@ -472,8 +472,8 @@ translate_bytecode_to_trace(
     } \
     reserved = (n);  // Keep ADD_TO_TRACE / ADD_TO_STUB honest
 
-// Reserve space for main+stub uops, plus 2 for _SET_IP and _EXIT_TRACE
-#define RESERVE(main, stub) RESERVE_RAW((main) + (stub) + 2, uop_name(opcode))
+// Reserve space for main+stub uops, plus 3 for _SET_IP, _CHECK_VALIDITY and _EXIT_TRACE
+#define RESERVE(main, stub) RESERVE_RAW((main) + (stub) + 3, uop_name(opcode))
 
 // Trace stack operations (used by _PUSH_FRAME, _POP_FRAME)
 #define TRACE_STACK_PUSH() \
@@ -503,8 +503,9 @@ translate_bytecode_to_trace(
 
 top:  // Jump here after _PUSH_FRAME or likely branches
     for (;;) {
-        RESERVE_RAW(2, "epilogue");  // Always need space for _SET_IP and _EXIT_TRACE
+        RESERVE_RAW(3, "epilogue");  // Always need space for _SET_IP, _CHECK_VALIDITY and _EXIT_TRACE
         ADD_TO_TRACE(_SET_IP, INSTR_IP(instr, code), 0);
+        ADD_TO_TRACE(_CHECK_VALIDITY, 0, 0);
 
         uint32_t opcode = instr->op.code;
         uint32_t oparg = instr->op.arg;

Python/ceval.c

@@ -12,13 +12,13 @@
 #include <stddef.h>
 #include "pycore_optimizer.h"
 
-
 static void
 remove_unneeded_uops(_PyUOpInstruction *buffer, int buffer_size)
 {
     // Note that we don't enter stubs, those SET_IPs are needed.
     int last_set_ip = -1;
     bool need_ip = true;
+    bool maybe_invalid = false;
     for (int pc = 0; pc < buffer_size; pc++) {
         int opcode = buffer[pc].opcode;
         if (opcode == _SET_IP) {
@@ -28,6 +28,16 @@ remove_unneeded_uops(_PyUOpInstruction *buffer, int buffer_size)
             need_ip = false;
             last_set_ip = pc;
         }
+        else if (opcode == _CHECK_VALIDITY) {
+            if (maybe_invalid) {
+                /* Exiting the trace requires that IP is correct */
+                need_ip = true;
+                maybe_invalid = false;
+            }
+            else {
+                buffer[pc].opcode = NOP;
+            }
+        }
         else if (opcode == _JUMP_TO_TOP || opcode == _EXIT_TRACE) {
             break;
         }
@@ -36,6 +46,9 @@ remove_unneeded_uops(_PyUOpInstruction *buffer, int buffer_size)
             if (_PyOpcode_opcode_metadata[opcode].flags & (HAS_ERROR_FLAG | HAS_DEOPT_FLAG) || opcode == _PUSH_FRAME) {
                 need_ip = true;
             }
+            if (_PyOpcode_opcode_metadata[opcode].flags & HAS_ESCAPES_FLAG) {
+                maybe_invalid = true;
+            }
         }
     }
 }

Python/executor_cases.c.h

@@ -390,7 +390,7 @@ def analyze_pseudo(self, pseudo: parsing.Pseudo) -> PseudoInstruction:
             else:
                 targets.append(self.macro_instrs[target_name])
         assert targets
-        ignored_flags = {"HAS_EVAL_BREAK_FLAG", "HAS_DEOPT_FLAG", "HAS_ERROR_FLAG"}
+        ignored_flags = {"HAS_EVAL_BREAK_FLAG", "HAS_DEOPT_FLAG", "HAS_ERROR_FLAG", "HAS_ESCAPES_FLAG"}
         assert len({t.instr_flags.bitmap(ignore=ignored_flags) for t in targets}) == 1
         return PseudoInstruction(pseudo.name, targets, targets[0].instr_flags)