From 85473936c1a3e24b262ad39dc4d4da6d4ce11fb6 Mon Sep 17 00:00:00 2001 From: Kirill Podoprigora Date: Fri, 17 Jan 2025 23:37:46 +0200 Subject: [PATCH 1/8] Fix zero division --- Lib/test/test_opcache.py | 13 +++++++++++++ Python/specialize.c | 28 ++++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py index 72b845fcc8fdbf..77fd17b3fd1c2e 100644 --- a/Lib/test/test_opcache.py +++ b/Lib/test/test_opcache.py @@ -1362,6 +1362,19 @@ def binary_op_add_extend(): self.assert_specialized(binary_op_add_extend, "BINARY_OP_EXTEND") self.assert_no_opcode(binary_op_add_extend, "BINARY_OP") + def compactlong_lhs(arg): + 42 / arg + def float_lhs(arg): + 42.0 / arg + + with self.assertRaises(ZeroDivisionError): + compactlong_lhs(0) + with self.assertRaises(ZeroDivisionError): + compactlong_lhs(0.0) + with self.assertRaises(ZeroDivisionError): + float_lhs(0) + with self.assertRaises(ZeroDivisionError): + float_lhs(0.0) @cpython_only @requires_specialization_ft diff --git a/Python/specialize.c b/Python/specialize.c index 09bfcd34b5a543..e067d061e2e708 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -2426,6 +2426,18 @@ float_compactlong_guard(PyObject *lhs, PyObject *rhs) ); } +static int +float_compactlong_guard_true_div(PyObject *lhs, PyObject *rhs) +{ + // guards should check if rhs has a non-zero value + return ( + PyFloat_CheckExact(lhs) && + PyLong_CheckExact(rhs) && + _PyLong_IsCompact((PyLongObject *)rhs) && + !PyLong_IsZero(rhs) + ); +} + #define FLOAT_LONG_ACTION(NAME, OP) \ static PyObject * \ (NAME)(PyObject *lhs, PyObject *rhs) \ @@ -2452,6 +2464,18 @@ compactlong_float_guard(PyObject *lhs, PyObject *rhs) ); } +static int +compactlong_float_guard_true_div(PyObject *lhs, PyObject *rhs) +{ + // guards should check if rhs has a non-zero value + return ( + PyFloat_CheckExact(rhs) && + PyLong_CheckExact(lhs) && + _PyLong_IsCompact((PyLongObject *)lhs) && + PyFloat_AsDouble(rhs) != 0.0 + ); +} + #define LONG_FLOAT_ACTION(NAME, OP) \ static PyObject * \ (NAME)(PyObject *lhs, PyObject *rhs) \ @@ -2469,14 +2493,14 @@ LONG_FLOAT_ACTION(compactlong_float_true_div, /) static _PyBinaryOpSpecializationDescr float_compactlong_specs[NB_OPARG_LAST+1] = { [NB_ADD] = {float_compactlong_guard, float_compactlong_add}, [NB_SUBTRACT] = {float_compactlong_guard, float_compactlong_subtract}, - [NB_TRUE_DIVIDE] = {float_compactlong_guard, float_compactlong_true_div}, + [NB_TRUE_DIVIDE] = {float_compactlong_guard_true_div, float_compactlong_true_div}, [NB_MULTIPLY] = {float_compactlong_guard, float_compactlong_multiply}, }; static _PyBinaryOpSpecializationDescr compactlong_float_specs[NB_OPARG_LAST+1] = { [NB_ADD] = {compactlong_float_guard, compactlong_float_add}, [NB_SUBTRACT] = {compactlong_float_guard, compactlong_float_subtract}, - [NB_TRUE_DIVIDE] = {compactlong_float_guard, compactlong_float_true_div}, + [NB_TRUE_DIVIDE] = {compactlong_float_guard_true_div, compactlong_float_true_div}, [NB_MULTIPLY] = {compactlong_float_guard, compactlong_float_multiply}, }; From 131b92a58055101987612e3fbc21f136fcab5e51 Mon Sep 17 00:00:00 2001 From: Kirill Podoprigora Date: Fri, 17 Jan 2025 23:50:50 +0200 Subject: [PATCH 2/8] Reorder calls --- Lib/test/test_opcache.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py index 77fd17b3fd1c2e..95877ef1bc419e 100644 --- a/Lib/test/test_opcache.py +++ b/Lib/test/test_opcache.py @@ -1371,10 +1371,10 @@ def float_lhs(arg): compactlong_lhs(0) with self.assertRaises(ZeroDivisionError): compactlong_lhs(0.0) - with self.assertRaises(ZeroDivisionError): - float_lhs(0) with self.assertRaises(ZeroDivisionError): float_lhs(0.0) + with self.assertRaises(ZeroDivisionError): + float_lhs(0) @cpython_only @requires_specialization_ft From 8363e9d270d95b7d23a0cda5dff6a9448bc28ea5 Mon Sep 17 00:00:00 2001 From: Kirill Podoprigora Date: Sat, 18 Jan 2025 00:53:48 +0200 Subject: [PATCH 3/8] Address Irit's review && Add guards for NaN --- Lib/test/test_opcache.py | 54 ++++++++++++++++++++++++++++++---------- Python/specialize.c | 16 +++++++----- 2 files changed, 51 insertions(+), 19 deletions(-) diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py index 95877ef1bc419e..597f172d530059 100644 --- a/Lib/test/test_opcache.py +++ b/Lib/test/test_opcache.py @@ -1362,19 +1362,47 @@ def binary_op_add_extend(): self.assert_specialized(binary_op_add_extend, "BINARY_OP_EXTEND") self.assert_no_opcode(binary_op_add_extend, "BINARY_OP") - def compactlong_lhs(arg): - 42 / arg - def float_lhs(arg): - 42.0 / arg - - with self.assertRaises(ZeroDivisionError): - compactlong_lhs(0) - with self.assertRaises(ZeroDivisionError): - compactlong_lhs(0.0) - with self.assertRaises(ZeroDivisionError): - float_lhs(0.0) - with self.assertRaises(ZeroDivisionError): - float_lhs(0) + def binary_op_zero_division(): + def compactlong_lhs(arg): + 42 / arg + def float_lhs(arg): + 42.0 / arg + + with self.assertRaises(ZeroDivisionError): + compactlong_lhs(0) + with self.assertRaises(ZeroDivisionError): + compactlong_lhs(0.0) + with self.assertRaises(ZeroDivisionError): + float_lhs(0.0) + with self.assertRaises(ZeroDivisionError): + float_lhs(0) + + self.assert_no_opcode(compactlong_lhs, "BINARY_OP_EXTEND") + self.assert_no_opcode(float_lhs, "BINARY_OP_EXTEND") + + binary_op_zero_division() + + def binary_op_nan(): + def compactlong_lhs(arg): + 42 + arg + 42 - arg + 42 * arg + 42 / arg + def compactlong_rhs(arg): + arg + 42 + arg - 42 + arg * 42 + arg / 42 + + compactlong_lhs(1.0) + compactlong_lhs(float('nan')) + compactlong_rhs(1.0) + compactlong_rhs(float('nan')) + + self.assert_no_opcode(compactlong_lhs, "BINARY_OP_EXTEND") + self.assert_no_opcode(compactlong_rhs, "BINARY_OP_EXTEND") + + binary_op_nan() @cpython_only @requires_specialization_ft diff --git a/Python/specialize.c b/Python/specialize.c index e067d061e2e708..54159c4679aa01 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -2416,25 +2416,28 @@ binary_op_fail_kind(int oparg, PyObject *lhs, PyObject *rhs) /* float-long */ +// Guards should check that the float part is not NaN. +// Guards for / (aka true_div) should check for 0 or 0.0 as the rhs. static int float_compactlong_guard(PyObject *lhs, PyObject *rhs) { return ( PyFloat_CheckExact(lhs) && PyLong_CheckExact(rhs) && - _PyLong_IsCompact((PyLongObject *)rhs) + _PyLong_IsCompact((PyLongObject *)rhs) && + !isnan(PyFloat_AsDouble(lhs)) ); } static int float_compactlong_guard_true_div(PyObject *lhs, PyObject *rhs) { - // guards should check if rhs has a non-zero value return ( PyFloat_CheckExact(lhs) && PyLong_CheckExact(rhs) && _PyLong_IsCompact((PyLongObject *)rhs) && - !PyLong_IsZero(rhs) + !PyLong_IsZero(rhs) && + !isnan(PyFloat_AsDouble(lhs)) ); } @@ -2460,19 +2463,20 @@ compactlong_float_guard(PyObject *lhs, PyObject *rhs) return ( PyFloat_CheckExact(rhs) && PyLong_CheckExact(lhs) && - _PyLong_IsCompact((PyLongObject *)lhs) + _PyLong_IsCompact((PyLongObject *)lhs) && + !isnan(PyFloat_AsDouble(rhs)) ); } static int compactlong_float_guard_true_div(PyObject *lhs, PyObject *rhs) { - // guards should check if rhs has a non-zero value return ( PyFloat_CheckExact(rhs) && PyLong_CheckExact(lhs) && _PyLong_IsCompact((PyLongObject *)lhs) && - PyFloat_AsDouble(rhs) != 0.0 + PyFloat_AsDouble(rhs) != 0.0 && + !isnan(PyFloat_AsDouble(rhs)) ); } From 850a862a7d86ceb1ae44d1400af5b90c24239af7 Mon Sep 17 00:00:00 2001 From: Kirill Podoprigora Date: Sat, 18 Jan 2025 12:35:48 +0200 Subject: [PATCH 4/8] Update Python/specialize.c Co-authored-by: Tomas R. --- Python/specialize.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 54159c4679aa01..ebd5a03ae4fca3 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -2433,11 +2433,7 @@ static int float_compactlong_guard_true_div(PyObject *lhs, PyObject *rhs) { return ( - PyFloat_CheckExact(lhs) && - PyLong_CheckExact(rhs) && - _PyLong_IsCompact((PyLongObject *)rhs) && - !PyLong_IsZero(rhs) && - !isnan(PyFloat_AsDouble(lhs)) + float_compactlong_guard(lhs, rhs) && !PyLong_IsZero(rhs) ); } From f98667c8a540a181e364f44beebf6683f7eb7fc3 Mon Sep 17 00:00:00 2001 From: Kirill Podoprigora Date: Sat, 18 Jan 2025 13:18:33 +0200 Subject: [PATCH 5/8] Apply the same technique for compactlong_float --- Python/specialize.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index ebd5a03ae4fca3..7eb13fc4a2a2f0 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -2418,7 +2418,7 @@ binary_op_fail_kind(int oparg, PyObject *lhs, PyObject *rhs) // Guards should check that the float part is not NaN. // Guards for / (aka true_div) should check for 0 or 0.0 as the rhs. -static int +static inline int float_compactlong_guard(PyObject *lhs, PyObject *rhs) { return ( @@ -2429,7 +2429,7 @@ float_compactlong_guard(PyObject *lhs, PyObject *rhs) ); } -static int +static inline int float_compactlong_guard_true_div(PyObject *lhs, PyObject *rhs) { return ( @@ -2453,7 +2453,7 @@ FLOAT_LONG_ACTION(float_compactlong_true_div, /) /* long-float */ -static int +static inline int compactlong_float_guard(PyObject *lhs, PyObject *rhs) { return ( @@ -2464,15 +2464,11 @@ compactlong_float_guard(PyObject *lhs, PyObject *rhs) ); } -static int +static inline int compactlong_float_guard_true_div(PyObject *lhs, PyObject *rhs) { return ( - PyFloat_CheckExact(rhs) && - PyLong_CheckExact(lhs) && - _PyLong_IsCompact((PyLongObject *)lhs) && - PyFloat_AsDouble(rhs) != 0.0 && - !isnan(PyFloat_AsDouble(rhs)) + compactlong_float_guard(lhs, rhs) && PyFloat_AsDouble(rhs) != 0.0 ); } From 27cd99a342abb678ba5f5e0630f36c53f26f4114 Mon Sep 17 00:00:00 2001 From: Kirill Podoprigora Date: Sat, 18 Jan 2025 13:45:44 +0200 Subject: [PATCH 6/8] I guess it should work --- Lib/test/test_opcache.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py index 597f172d530059..ed0f107e709d22 100644 --- a/Lib/test/test_opcache.py +++ b/Lib/test/test_opcache.py @@ -1395,9 +1395,11 @@ def compactlong_rhs(arg): arg / 42 compactlong_lhs(1.0) - compactlong_lhs(float('nan')) + for _ in range(100): + compactlong_lhs(float('nan')) compactlong_rhs(1.0) - compactlong_rhs(float('nan')) + for _ in range(100): + compactlong_rhs(float('nan')) self.assert_no_opcode(compactlong_lhs, "BINARY_OP_EXTEND") self.assert_no_opcode(compactlong_rhs, "BINARY_OP_EXTEND") From aa416bbcb7def88eb633862f849338cedc5bc08d Mon Sep 17 00:00:00 2001 From: Kirill Podoprigora Date: Sun, 19 Jan 2025 11:48:35 +0200 Subject: [PATCH 7/8] Apply suggestions from code review Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> --- Python/specialize.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 7eb13fc4a2a2f0..9e3de025c8be7d 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -2416,8 +2416,6 @@ binary_op_fail_kind(int oparg, PyObject *lhs, PyObject *rhs) /* float-long */ -// Guards should check that the float part is not NaN. -// Guards for / (aka true_div) should check for 0 or 0.0 as the rhs. static inline int float_compactlong_guard(PyObject *lhs, PyObject *rhs) { From 127cb30e1b0c25b2472a2547ccea409883cefecb Mon Sep 17 00:00:00 2001 From: Kirill Podoprigora Date: Sun, 19 Jan 2025 12:40:39 +0200 Subject: [PATCH 8/8] Address Irit's review --- Lib/test/test_opcache.py | 30 +++++++++++++++++------------- Python/specialize.c | 14 +++++++------- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py index ed0f107e709d22..4d7304b1c9abb6 100644 --- a/Lib/test/test_opcache.py +++ b/Lib/test/test_opcache.py @@ -1384,22 +1384,26 @@ def float_lhs(arg): def binary_op_nan(): def compactlong_lhs(arg): - 42 + arg - 42 - arg - 42 * arg - 42 / arg + return ( + 42 + arg, + 42 - arg, + 42 * arg, + 42 / arg, + ) def compactlong_rhs(arg): - arg + 42 - arg - 42 - arg * 42 - arg / 42 - - compactlong_lhs(1.0) + return ( + arg + 42, + arg - 42, + arg * 2, + arg / 42, + ) + nan = float('nan') + self.assertEqual(compactlong_lhs(1.0), (43.0, 41.0, 42.0, 42.0)) for _ in range(100): - compactlong_lhs(float('nan')) - compactlong_rhs(1.0) + self.assertTrue(all(filter(lambda x: x is nan, compactlong_lhs(nan)))) + self.assertEqual(compactlong_rhs(42.0), (84.0, 0.0, 84.0, 1.0)) for _ in range(100): - compactlong_rhs(float('nan')) + self.assertTrue(all(filter(lambda x: x is nan, compactlong_rhs(nan)))) self.assert_no_opcode(compactlong_lhs, "BINARY_OP_EXTEND") self.assert_no_opcode(compactlong_rhs, "BINARY_OP_EXTEND") diff --git a/Python/specialize.c b/Python/specialize.c index 9e3de025c8be7d..fa022346bdea6a 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -2421,14 +2421,14 @@ float_compactlong_guard(PyObject *lhs, PyObject *rhs) { return ( PyFloat_CheckExact(lhs) && + !isnan(PyFloat_AsDouble(lhs)) && PyLong_CheckExact(rhs) && - _PyLong_IsCompact((PyLongObject *)rhs) && - !isnan(PyFloat_AsDouble(lhs)) + _PyLong_IsCompact((PyLongObject *)rhs) ); } static inline int -float_compactlong_guard_true_div(PyObject *lhs, PyObject *rhs) +nonzero_float_compactlong_guard(PyObject *lhs, PyObject *rhs) { return ( float_compactlong_guard(lhs, rhs) && !PyLong_IsZero(rhs) @@ -2455,15 +2455,15 @@ static inline int compactlong_float_guard(PyObject *lhs, PyObject *rhs) { return ( - PyFloat_CheckExact(rhs) && PyLong_CheckExact(lhs) && _PyLong_IsCompact((PyLongObject *)lhs) && + PyFloat_CheckExact(rhs) && !isnan(PyFloat_AsDouble(rhs)) ); } static inline int -compactlong_float_guard_true_div(PyObject *lhs, PyObject *rhs) +nonzero_compactlong_float_guard(PyObject *lhs, PyObject *rhs) { return ( compactlong_float_guard(lhs, rhs) && PyFloat_AsDouble(rhs) != 0.0 @@ -2487,14 +2487,14 @@ LONG_FLOAT_ACTION(compactlong_float_true_div, /) static _PyBinaryOpSpecializationDescr float_compactlong_specs[NB_OPARG_LAST+1] = { [NB_ADD] = {float_compactlong_guard, float_compactlong_add}, [NB_SUBTRACT] = {float_compactlong_guard, float_compactlong_subtract}, - [NB_TRUE_DIVIDE] = {float_compactlong_guard_true_div, float_compactlong_true_div}, + [NB_TRUE_DIVIDE] = {nonzero_float_compactlong_guard, float_compactlong_true_div}, [NB_MULTIPLY] = {float_compactlong_guard, float_compactlong_multiply}, }; static _PyBinaryOpSpecializationDescr compactlong_float_specs[NB_OPARG_LAST+1] = { [NB_ADD] = {compactlong_float_guard, compactlong_float_add}, [NB_SUBTRACT] = {compactlong_float_guard, compactlong_float_subtract}, - [NB_TRUE_DIVIDE] = {compactlong_float_guard_true_div, compactlong_float_true_div}, + [NB_TRUE_DIVIDE] = {nonzero_compactlong_float_guard, compactlong_float_true_div}, [NB_MULTIPLY] = {compactlong_float_guard, compactlong_float_multiply}, };