gh-116088: Insert bottom checks after all sym_set_...() calls (#116089)

This changes the `sym_set_...()` functions to return a `bool` which is `false`
when the symbol is `bottom` after the operation.

All calls to such functions now check this result and go to `hit_bottom`,
a special error label that prints a different message and then reports
that it wasn't able to optimize the trace. No executor will be produced
in this case.

diff --git a/Include/internal/pycore_optimizer.h b/Include/internal/pycore_optimizer.h
index 4894f613b5fb9188820ac3423922b6adfbc80e2b..d32e6c0174f68084d078f7892db380840756e63c 100644 (file)
--- a/Include/internal/pycore_optimizer.h
+++ b/Include/internal/pycore_optimizer.h
@@ -91,10 +91,10 @@ extern _Py_UopsSymbol *_Py_uop_sym_new_type(
 extern _Py_UopsSymbol *_Py_uop_sym_new_const(_Py_UOpsContext *ctx, PyObject *const_val);
 extern _Py_UopsSymbol *_Py_uop_sym_new_null(_Py_UOpsContext *ctx);
 extern bool _Py_uop_sym_matches_type(_Py_UopsSymbol *sym, PyTypeObject *typ);
-extern void _Py_uop_sym_set_null(_Py_UopsSymbol *sym);
-extern void _Py_uop_sym_set_non_null(_Py_UopsSymbol *sym);
-extern void _Py_uop_sym_set_type(_Py_UopsSymbol *sym, PyTypeObject *typ);
-extern void _Py_uop_sym_set_const(_Py_UopsSymbol *sym, PyObject *const_val);
+extern bool _Py_uop_sym_set_null(_Py_UopsSymbol *sym);
+extern bool _Py_uop_sym_set_non_null(_Py_UopsSymbol *sym);
+extern bool _Py_uop_sym_set_type(_Py_UopsSymbol *sym, PyTypeObject *typ);
+extern bool _Py_uop_sym_set_const(_Py_UopsSymbol *sym, PyObject *const_val);
 extern bool _Py_uop_sym_is_bottom(_Py_UopsSymbol *sym);
 
 

diff --git a/Lib/test/test_capi/test_opt.py b/Lib/test/test_capi/test_opt.py
index a43726f05a448d39a2e1114b965f232bb53dea62..d21765307f0f096db201cca4b6e8ad16f3cc32a3 100644 (file)
--- a/Lib/test/test_capi/test_opt.py
+++ b/Lib/test/test_capi/test_opt.py
@@ -894,23 +894,33 @@ class TestUopsOptimization(unittest.TestCase):
 
     def test_type_inconsistency(self):
         ns = {}
-        exec(textwrap.dedent("""
+        src = textwrap.dedent("""
             def testfunc(n):
                 for i in range(n):
                     x = _test_global + _test_global
-        """), globals(), ns)
+        """)
+        exec(src, ns, ns)
         testfunc = ns['testfunc']
-        # Must be a real global else it won't be optimized to _LOAD_CONST_INLINE
-        global _test_global
-        _test_global = 0
+        ns['_test_global'] = 0
         _, ex = self._run_with_optimizer(testfunc, 16)
         self.assertIsNone(ex)
-        _test_global = 1.2
+        ns['_test_global'] = 1
         _, ex = self._run_with_optimizer(testfunc, 16)
         self.assertIsNotNone(ex)
         uops = get_opnames(ex)
-        self.assertIn("_GUARD_BOTH_INT", uops)
+        self.assertNotIn("_GUARD_BOTH_INT", uops)
         self.assertIn("_BINARY_OP_ADD_INT", uops)
+        # Try again, but between the runs, set the global to a float.
+        # This should result in no executor the second time.
+        ns = {}
+        exec(src, ns, ns)
+        testfunc = ns['testfunc']
+        ns['_test_global'] = 0
+        _, ex = self._run_with_optimizer(testfunc, 16)
+        self.assertIsNone(ex)
+        ns['_test_global'] = 3.14
+        _, ex = self._run_with_optimizer(testfunc, 16)
+        self.assertIsNone(ex)
 
 
 if __name__ == "__main__":

diff --git a/Python/optimizer_analysis.c b/Python/optimizer_analysis.c
index 2a7ef4ec919eebe615bf393529003a8858c337b8..a326e2249bb4dec5965d4292cca16028423af6e1 100644 (file)
--- a/Python/optimizer_analysis.c
+++ b/Python/optimizer_analysis.c
@@ -363,6 +363,15 @@ error:
     DPRINTF(1, "Encountered error in abstract interpreter\n");
     _Py_uop_abstractcontext_fini(ctx);
     return 0;
+
+hit_bottom:
+    // Attempted to push a "bottom" (contradition) symbol onto the stack.
+    // This means that the abstract interpreter has hit unreachable code.
+    // We *could* generate an _EXIT_TRACE or _FATAL_ERROR here, but it's
+    // simpler to just admit failure and not create the executor.
+    DPRINTF(1, "Hit bottom in abstract interpreter\n");
+    _Py_uop_abstractcontext_fini(ctx);
+    return 0;
 }
 
 

diff --git a/Python/optimizer_bytecodes.c b/Python/optimizer_bytecodes.c
index 928c22da16b99982ce2c84286509d1d830d604db..aa19a50f09ff71b06b5e22adfbeea3ea80a0a98d 100644 (file)
--- a/Python/optimizer_bytecodes.c
+++ b/Python/optimizer_bytecodes.c
@@ -85,8 +85,12 @@ dummy_func(void) {
             sym_matches_type(right, &PyLong_Type)) {
             REPLACE_OP(this_instr, _NOP, 0, 0);
         }
-        sym_set_type(left, &PyLong_Type);
-        sym_set_type(right, &PyLong_Type);
+        if (!sym_set_type(left, &PyLong_Type)) {
+            goto hit_bottom;
+        }
+        if (!sym_set_type(right, &PyLong_Type)) {
+            goto hit_bottom;
+        }
     }
 
     op(_GUARD_BOTH_FLOAT, (left, right -- left, right)) {
@@ -94,8 +98,12 @@ dummy_func(void) {
             sym_matches_type(right, &PyFloat_Type)) {
             REPLACE_OP(this_instr, _NOP, 0 ,0);
         }
-        sym_set_type(left, &PyFloat_Type);
-        sym_set_type(right, &PyFloat_Type);
+        if (!sym_set_type(left, &PyFloat_Type)) {
+            goto hit_bottom;
+        }
+        if (!sym_set_type(right, &PyFloat_Type)) {
+            goto hit_bottom;
+        }
     }
 
     op(_GUARD_BOTH_UNICODE, (left, right -- left, right)) {
@@ -103,8 +111,12 @@ dummy_func(void) {
             sym_matches_type(right, &PyUnicode_Type)) {
             REPLACE_OP(this_instr, _NOP, 0 ,0);
         }
-        sym_set_type(left, &PyUnicode_Type);
-        sym_set_type(right, &PyUnicode_Type);
+        if (!sym_set_type(left, &PyUnicode_Type)) {
+            goto hit_bottom;
+        }
+        if (!sym_set_type(right, &PyUnicode_Type)) {
+            goto hit_bottom;
+        }
     }
 
     op(_BINARY_OP_ADD_INT, (left, right -- res)) {
@@ -365,14 +377,20 @@ dummy_func(void) {
 
 
     op(_CHECK_FUNCTION_EXACT_ARGS, (func_version/2, callable, self_or_null, unused[oparg] -- callable, self_or_null, unused[oparg])) {
-        sym_set_type(callable, &PyFunction_Type);
+        if (!sym_set_type(callable, &PyFunction_Type)) {
+            goto hit_bottom;
+        }
         (void)self_or_null;
         (void)func_version;
     }
 
     op(_CHECK_CALL_BOUND_METHOD_EXACT_ARGS, (callable, null, unused[oparg] -- callable, null, unused[oparg])) {
-        sym_set_null(null);
-        sym_set_type(callable, &PyMethod_Type);
+        if (!sym_set_null(null)) {
+            goto hit_bottom;
+        }
+        if (!sym_set_type(callable, &PyMethod_Type)) {
+            goto hit_bottom;
+        }
     }
 
     op(_INIT_CALL_PY_EXACT_ARGS, (callable, self_or_null, args[oparg] -- new_frame: _Py_UOpsAbstractFrame *)) {

diff --git a/Python/optimizer_cases.c.h b/Python/optimizer_cases.c.h
index b38c03bed4b4d0747840893f3bc2e8b78ff876a9..b34c57376df88e76a621769c33bee7ceae0d7c3c 100644 (file)
--- a/Python/optimizer_cases.c.h
+++ b/Python/optimizer_cases.c.h
@@ -172,8 +172,12 @@
                 sym_matches_type(right, &PyLong_Type)) {
                 REPLACE_OP(this_instr, _NOP, 0, 0);
             }
-            sym_set_type(left, &PyLong_Type);
-            sym_set_type(right, &PyLong_Type);
+            if (!sym_set_type(left, &PyLong_Type)) {
+                goto hit_bottom;
+            }
+            if (!sym_set_type(right, &PyLong_Type)) {
+                goto hit_bottom;
+            }
             break;
         }
 
@@ -276,8 +280,12 @@
                 sym_matches_type(right, &PyFloat_Type)) {
                 REPLACE_OP(this_instr, _NOP, 0 ,0);
             }
-            sym_set_type(left, &PyFloat_Type);
-            sym_set_type(right, &PyFloat_Type);
+            if (!sym_set_type(left, &PyFloat_Type)) {
+                goto hit_bottom;
+            }
+            if (!sym_set_type(right, &PyFloat_Type)) {
+                goto hit_bottom;
+            }
             break;
         }
 
@@ -383,8 +391,12 @@
                 sym_matches_type(right, &PyUnicode_Type)) {
                 REPLACE_OP(this_instr, _NOP, 0 ,0);
             }
-            sym_set_type(left, &PyUnicode_Type);
-            sym_set_type(right, &PyUnicode_Type);
+            if (!sym_set_type(left, &PyUnicode_Type)) {
+                goto hit_bottom;
+            }
+            if (!sym_set_type(right, &PyUnicode_Type)) {
+                goto hit_bottom;
+            }
             break;
         }
 
@@ -1397,8 +1409,12 @@
             _Py_UopsSymbol *callable;
             null = stack_pointer[-1 - oparg];
             callable = stack_pointer[-2 - oparg];
-            sym_set_null(null);
-            sym_set_type(callable, &PyMethod_Type);
+            if (!sym_set_null(null)) {
+                goto hit_bottom;
+            }
+            if (!sym_set_type(callable, &PyMethod_Type)) {
+                goto hit_bottom;
+            }
             break;
         }
 
@@ -1425,7 +1441,9 @@
             self_or_null = stack_pointer[-1 - oparg];
             callable = stack_pointer[-2 - oparg];
             uint32_t func_version = (uint32_t)this_instr->operand;
-            sym_set_type(callable, &PyFunction_Type);
+            if (!sym_set_type(callable, &PyFunction_Type)) {
+                goto hit_bottom;
+            }
             (void)self_or_null;
             (void)func_version;
             break;

diff --git a/Python/optimizer_symbols.c b/Python/optimizer_symbols.c
index a529cc2f5cf21505a74e6ac12c0881f79f812204..5c3ec2b5ed1a4c4657168d3c15e231483409dd7e 100644 (file)
--- a/Python/optimizer_symbols.c
+++ b/Python/optimizer_symbols.c
@@ -113,60 +113,68 @@ _Py_uop_sym_get_const(_Py_UopsSymbol *sym)
     return sym->const_val;
 }
 
-void
+bool
 _Py_uop_sym_set_type(_Py_UopsSymbol *sym, PyTypeObject *typ)
 {
     assert(typ != NULL && PyType_Check(typ));
     if (sym->flags & IS_NULL) {
         sym_set_bottom(sym);
-        return;
+        return false;
     }
     if (sym->typ != NULL) {
         if (sym->typ != typ) {
             sym_set_bottom(sym);
+            return false;
         }
-        return;
     }
-    sym_set_flag(sym, NOT_NULL);
-    sym->typ = typ;
+    else {
+        sym_set_flag(sym, NOT_NULL);
+        sym->typ = typ;
+    }
+    return true;
 }
 
-void
+bool
 _Py_uop_sym_set_const(_Py_UopsSymbol *sym, PyObject *const_val)
 {
     assert(const_val != NULL);
     if (sym->flags & IS_NULL) {
         sym_set_bottom(sym);
-        return;
+        return false;
     }
     PyTypeObject *typ = Py_TYPE(const_val);
     if (sym->typ != NULL && sym->typ != typ) {
         sym_set_bottom(sym);
-        return;
+        return false;
     }
     if (sym->const_val != NULL) {
         if (sym->const_val != const_val) {
             // TODO: What if they're equal?
             sym_set_bottom(sym);
+            return false;
         }
-        return;
     }
-    sym_set_flag(sym, NOT_NULL);
-    sym->typ = typ;
-    sym->const_val = Py_NewRef(const_val);
+    else {
+        sym_set_flag(sym, NOT_NULL);
+        sym->typ = typ;
+        sym->const_val = Py_NewRef(const_val);
+    }
+    return true;
 }
 
 
-void
+bool
 _Py_uop_sym_set_null(_Py_UopsSymbol *sym)
 {
     sym_set_flag(sym, IS_NULL);
+    return !_Py_uop_sym_is_bottom(sym);
 }
 
-void
+bool
 _Py_uop_sym_set_non_null(_Py_UopsSymbol *sym)
 {
     sym_set_flag(sym, NOT_NULL);
+    return !_Py_uop_sym_is_bottom(sym);
 }
 
 

diff --git a/Tools/cases_generator/optimizer_generator.py b/Tools/cases_generator/optimizer_generator.py
index d3ce4c8a25f5b8594c143e719232d27bdcffd157..fca42b51fbd68905b457dbc5153f56ecec0e4606 100644 (file)
--- a/Tools/cases_generator/optimizer_generator.py
+++ b/Tools/cases_generator/optimizer_generator.py
@@ -4,16 +4,12 @@ Writes the cases to optimizer_cases.c.h, which is #included in Python/optimizer_
 """
 
 import argparse
-import os.path
-import sys
 
 from analyzer import (
     Analysis,
     Instruction,
     Uop,
-    Part,
     analyze_files,
-    Skip,
     StackItem,
     analysis_error,
 )