bpo-46465: Check eval breaker in specialized `CALL` opcodes#30826

Fidget-Spinner · 2022-01-23T13:02:35Z

https://bugs.python.org/issue46465

Fidget-Spinner · 2022-01-23T13:11:30Z

I added CHECK_EVAL_BREAKER to all CALL_X opcodes, but I'm not sure how to do that for CALL_NO_KW_PY_SIMPLE (and the inline Python call path of CALL_NO_KW). @pablogsal and @markshannon can I please get your opinion on whether we need to check for signals for those?

Lib/test/support/__init__.py

vstinner

Currently, the "error" label ends with DISPATCH(). It doesn't call CHECK_EVAL_BREAKER() even if we are coming from a CALL target which can get a signal (or any other reason to break the ceval).

I'm not sure that I like the test decorator, it makes a test 16x slower :-(

Lib/test/support/__init__.py

Lib/unittest/test/test_break.py

vstinner · 2022-01-23T13:28:32Z

Currently, the "error" label ends with DISPATCH(). It doesn't call CHECK_EVAL_BREAKER() even if we are coming from a CALL target which can get a signal (or any other reason to break the ceval).

In Python 3.10, the error label ends with "goto main_loop" which does check for the ceval breaker atomic variable, no?

vstinner · 2022-01-23T13:31:57Z

I added CHECK_EVAL_BREAKER to all CALL_X opcodes, but I'm not sure how to do that for CALL_NO_KW_PY_SIMPLE (and the inline Python call path of CALL_NO_KW). @pablogsal and @markshannon can I please get your opinion on whether we need to check for signals for those?

CALL_NO_KW_PY_SIMPLE optimization goes to start_frame which ends with DISPATCH(): it seems like the CHECK_EVAL_BREAKER() check has been eaten by the optimization, between Python 3.10 and 3.11. Either start_frame or start_frame should call CHECK_EVAL_BREAKER().

vstinner · 2022-01-23T13:34:48Z

The ceval.c change are enough for almost all TARGET, except CALL_NO_KW_PY_SIMPLE which miss CHECK_EVAL_BREAKER() somewhere, but it can be fixed in a separated PR (with a fix for "error" label ? ;-)) if you prefer.

Co-Authored-By: Victor Stinner <[email protected]>

Fidget-Spinner · 2022-01-23T16:16:12Z

The ceval.c change are enough for almost all TARGET, except CALL_NO_KW_PY_SIMPLE which miss CHECK_EVAL_BREAKER() somewhere, but it can be fixed in a separated PR (with a fix for "error" label ? ;-)) if you prefer.

Yeah I'd prefer another PR for that. I don't think those changes came with the CALL_X PR, maybe one of the refactoring/dispatch optimizing PRs.

vstinner

LGTM.

Python/ceval.c

Lib/test/support/__init__.py

Co-Authored-By: Mark Shannon <[email protected]>

Fidget-Spinner · 2022-01-24T16:08:27Z

Strange, Ubuntu tests fail even on non-specialized code.

testInstallHandler (unittest.test.test_break.TestBreak) ... testInstallHandler (unittest.test.test_break.TestBreak) (cpython_is_warmedup=False) ... FAIL testInstallHandler (unittest.test.test_break.TestBreak) (cpython_is_warmedup=False) ... FAIL testInstallHandler (unittest.test.test_break.TestBreak) (cpython_is_warmedup=False) ... FAIL testInstallHandler (unittest.test.test_break.TestBreak) (cpython_is_warmedup=False) ... FAIL testInstallHandler (unittest.test.test_break.TestBreak) (cpython_is_warmedup=False) ... FAIL testInstallHandler (unittest.test.test_break.TestBreak) (cpython_is_warmedup=False) ... FAIL testInstallHandler (unittest.test.test_break.TestBreak) (cpython_is_warmedup=False) ... FAIL testInstallHandler (unittest.test.test_break.TestBreak) (cpython_is_warmedup=True) ... FAIL ====================================================================== FAIL: testInstallHandler (unittest.test.test_break.TestBreak) (cpython_is_warmedup=False) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/support/__init__.py", line 2142, in wrapper f(self) ^^^^^^^ File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/test/test_break.py", line 33, in testInstallHandler self.assertNotEqual(signal.getsignal(signal.SIGINT), default_handler) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AssertionError: <unittest.signals._InterruptHandler object at 0x7f7f10afb760> == <unittest.signals._InterruptHandler object at 0x7f7f10afb760>

I'll look into it.

sobolevn · 2022-02-11T13:19:48Z

Lib/unittest/test/test_break.py

-
+ # Tests both adaptive and specialized opcodes for proper
+ # CHECK_EVAL_BREAKER(). See bpo-46465 for an example bug.
+ @repeat_cpython_adaptative


From what I've tested in https://bugs.python.org/issue46709 there several more test cases that require this decorator:
testInterruptCaught
testSecondInterrupt
testTwoResults

Fidget-Spinner · 2022-02-12T03:40:07Z

Sorry, can I trouble someone here to take over this PR please? I can't complete it right now.

sobolevn · 2022-02-12T06:55:36Z

@Fidget-Spinner I will cover your back 😉

check eval breaker in specialized CALL opcodes
dd17ce9

Fidget-Spinner requested a review from markshannon as a code owner January 23, 2022 13:02

the-knights-who-say-ni added the CLA signed label Jan 23, 2022

bedevere-bot added the awaiting core review label Jan 23, 2022

Fidget-Spinner requested a review from vstinner January 23, 2022 13:02

kumaraditya303 reviewed Jan 23, 2022
View reviewed changes

Lib/test/support/__init__.py Outdated Show resolvedHide resolved

reference the C macro warmup count
0557134

vstinner reviewed Jan 23, 2022
View reviewed changes

Address code review
2ca3d9e
Co-Authored-By: Victor Stinner <[email protected]>

vstinner approved these changes Jan 23, 2022
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jan 23, 2022

markshannon reviewed Jan 24, 2022
View reviewed changes

Python/ceval.c Outdated Show resolvedHide resolved

markshannon reviewed Jan 24, 2022
View reviewed changes

Python/ceval.c Outdated Show resolvedHide resolved

markshannon reviewed Jan 24, 2022
View reviewed changes

Python/ceval.c Outdated Show resolvedHide resolved

markshannon reviewed Jan 24, 2022
View reviewed changes

Lib/test/support/__init__.py Outdated Show resolvedHide resolved

raghavthind2005 approved these changes Jan 24, 2022
View reviewed changes

Address Mark's reviews
d905928
Co-Authored-By: Mark Shannon <[email protected]>

sobolevn reviewed Feb 11, 2022
View reviewed changes

Fidget-Spinner closed this Feb 12, 2022

sobolevn mentioned this pull request Feb 18, 2022
bpo-46709: check eval breaker in specialized CALL opcodes #31404
Merged

vstinner mentioned this pull request Apr 10, 2022
Regression caused by CALL_FUNCTION specialization for C function calls (test_urllib fails when run multiple times) #90623
Closed

Uh oh!

bpo-46465: Check eval breaker in specialized CALL opcodes#30826

bpo-46465: Check eval breaker in specialized CALL opcodes #30826

Uh oh!

Conversation

Fidget-Spinner commented Jan 23, 2022• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fidget-Spinner commented Jan 23, 2022

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner commented Jan 23, 2022

Uh oh!

vstinner commented Jan 23, 2022

Uh oh!

vstinner commented Jan 23, 2022

Uh oh!

Fidget-Spinner commented Jan 23, 2022

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fidget-Spinner commented Jan 24, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sobolevnFeb 11, 2022

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner commented Feb 12, 2022

Uh oh!

sobolevn commented Feb 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

bpo-46465: Check eval breaker in specialized `CALL` opcodes#30826

bpo-46465: Check eval breaker in specialized `CALL` opcodes #30826

Fidget-Spinner commented Jan 23, 2022•
edited by bedevere-bot
Loading

Fidget-Spinner commented Jan 24, 2022•
edited
Loading