bpo-46465: Check eval breaker in specialized `CALL` opcodes by Fidget-Spinner · Pull Request #30826 · python/cpython

Fidget-Spinner · Jan 23, 2022

https://bugs.python.org/issue46465

Fidget-Spinner · Jan 23, 2022

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

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

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

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 <vstinner@python.org>

Fidget-Spinner · Jan 23, 2022

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 <mark@hotpy.org>

Fidget-Spinner · Jan 24, 2022

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 · Feb 11, 2022

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 · Feb 12, 2022

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

sobolevn · Feb 12, 2022

@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 <vstinner@python.org>

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 <mark@hotpy.org>

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

Search code, repositories, users, issues, pull requests...

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!

sobolevn Feb 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

Fidget-Spinner commented Jan 23, 2022 •

edited by bedevere-bot

Loading

Fidget-Spinner commented Jan 24, 2022 •

edited

Loading