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 resolved Hide 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 resolved Hide resolved

markshannon reviewed Jan 24, 2022

View reviewed changes

Python/ceval.c Outdated Show resolved Hide resolved

markshannon reviewed Jan 24, 2022

View reviewed changes

Python/ceval.c Outdated Show resolved Hide resolved

markshannon reviewed Jan 24, 2022

View reviewed changes

Lib/test/support/__init__.py Outdated Show resolved Hide 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 23, 2022

vstinner left a comment

vstinner commented Jan 23, 2022

vstinner commented Jan 23, 2022

vstinner commented Jan 23, 2022

Fidget-Spinner commented Jan 23, 2022

vstinner left a comment

Fidget-Spinner commented Jan 24, 2022 •

edited

Loading

sobolevn Feb 11, 2022

Fidget-Spinner commented Feb 12, 2022

sobolevn commented Feb 12, 2022

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

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

Conversation

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

Fidget-Spinner commented Jan 23, 2022

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented Jan 23, 2022

vstinner commented Jan 23, 2022

vstinner commented Jan 23, 2022

Fidget-Spinner commented Jan 23, 2022

vstinner left a comment

Choose a reason for hiding this comment

Fidget-Spinner commented Jan 24, 2022 • edited Loading

sobolevn Feb 11, 2022

Choose a reason for hiding this comment

Fidget-Spinner commented Feb 12, 2022

sobolevn commented Feb 12, 2022

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