Skip to content

gh-100239: Handle NaN and zero division in guards - #128963

Merged
Eclips4 merged 8 commits into
python:mainfrom
Eclips4:fix-zero-division
Jan 19, 2025
Merged

gh-100239: Handle NaN and zero division in guards#128963
Eclips4 merged 8 commits into
python:mainfrom
Eclips4:fix-zero-division

Conversation

@Eclips4

@Eclips4Eclips4 commented Jan 17, 2025

Copy link
Copy Markdown
Member

Comment threadLib/test/test_opcache.py Outdated
@iritkatriel

Copy link
Copy Markdown
Member

Shall we check for NaN as well?

@Eclips4

Copy link
Copy Markdown
MemberAuthor

Shall we check for NaN as well?

You mean "Shall we continue with specialization if we encounter a NaN value?"?
If so, I think we should add a check for NaN to all guards because I don't see any benefit in specializing to get a NaN result (As far as I remember, any operation involving NaN results in NaN).

@iritkatriel

Copy link
Copy Markdown
Member

Shall we check for NaN as well?

You mean "Shall we continue with specialization if we encounter a NaN value?"? If so, I think we should add a check for NaN to all guards because I don't see any benefit in specializing to get a NaN result (As far as I remember, any operation involving NaN results in NaN).

Yes, I guess it would be fine. Might be good to cover this in the tests though.

@Eclips4Eclips4 changed the title gh-100239: Handle zero division in guardsgh-100239: Handle NaN and zero division in guardsJan 17, 2025
Comment threadLib/test/test_opcache.py Outdated
Comment threadPython/specialize.c Outdated
@iritkatriel

Copy link
Copy Markdown
Member

Doesn't need to be in this PR, but I don't think we have a test for deoptimizing BINARY_OP_EXTEND. We should.

Eclips4and others added 2 commits January 18, 2025 12:35
@Eclips4

Copy link
Copy Markdown
MemberAuthor

Unfortunately ./python.exe -m test -v test_opcache -m test_binary_op -R 3:3 is failing:

Details
==CPython3.14.0a4+ (heads/fix-zero-division-dirty:850a862a7d8, Jan182025, 13:17:14) [Clang16.0.0 (clang-1600.0.26.4)]
==macOS-15.1.1-arm64-arm-64bit-Mach-Olittle-endian==Pythonbuild: debug==cwd:/Users/eclips4/programming/programming-languages/cpython/build/test_python_worker_4826æ==CPUcount: 8==encodings: locale=UTF-8FS=utf-8==resources: alltestresourcesaredisabled, use-uoptiontounskiptestsUsingrandomseed: 5489788770:00:00loadavg: 2.19Run1testsequentiallyinasingleprocess0:00:00loadavg: 2.19 [1/1] test_opcachebeginning6repetitions. Showingnumberofleaks (. for0orless, Xfor10ormore)
123:456test_binary_op (test.test_opcache.TestSpecializer.test_binary_op) ... ok----------------------------------------------------------------------Ran1testin0.006sOKXtest_binary_op (test.test_opcache.TestSpecializer.test_binary_op) ... ok----------------------------------------------------------------------Ran1testin0.007sOKXtest_binary_op (test.test_opcache.TestSpecializer.test_binary_op) ... FAIL======================================================================FAIL: test_binary_op (test.test_opcache.TestSpecializer.test_binary_op)
----------------------------------------------------------------------Traceback (mostrecentcalllast):
File"/Users/eclips4/programming/programming-languages/cpython/Lib/test/test_opcache.py", line1405, intest_binary_opbinary_op_nan()
~~~~~~~~~~~~~^^File"/Users/eclips4/programming/programming-languages/cpython/Lib/test/test_opcache.py", line1402, inbinary_op_nanself.assert_no_opcode(compactlong_lhs, "BINARY_OP_EXTEND")
~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^File"/Users/eclips4/programming/programming-languages/cpython/Lib/test/test_opcache.py", line42, inassert_no_opcodeself.assertNotIn(opname, opnames)
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^AssertionError: 'BINARY_OP_EXTEND'unexpectedlyfoundin {'RESUME_CHECK', 'RETURN_VALUE', 'POP_TOP', 'LOAD_CONST_IMMORTAL', 'BINARY_OP_EXTEND', 'LOAD_SMALL_INT', 'LOAD_FAST'}
----------------------------------------------------------------------Ran1testin0.012sFAILED (failures=1)
testtest_opcachefailedtest_opcachefailed (1failure)
==Testsresult: FAILURE==1testfailed:
test_opcacheTotalduration: 174msTotaltests: run=1 (filtered) failures=1Totaltestfiles: run=1/1 (filtered) failed=1Result: FAILURE

I'm digging into what's happening.

@Eclips4

Copy link
Copy Markdown
MemberAuthor

Okay, It seems that to get de-specialization it need to "hit" more guards. I'm not sure, though, if this is right.

Comment threadPython/specialize.c Outdated
Comment threadPython/specialize.c Outdated
Comment threadPython/specialize.c Outdated
Comment threadPython/specialize.c
Comment threadLib/test/test_opcache.py Outdated
Eclips4and others added 2 commits January 19, 2025 11:48
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
@Eclips4
Eclips4 enabled auto-merge (squash) January 19, 2025 10:42

@iritkatrieliritkatriel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@Eclips4

Copy link
Copy Markdown
MemberAuthor

Thank you Irit and Tomas for your review!

@Eclips4
Eclips4 merged commit 6c52ada into python:mainJan 19, 2025
@Eclips4
Eclips4 deleted the fix-zero-division branch January 19, 2025 14:16
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 21, 2025
…P_EXTEND` (python#128963)
Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Eclips4@iritkatriel@tomasr8