Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-107263: Increase C stack limit for most functions, except _PyEval_EvalFrameDefault() #107535

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Aug 1, 2023

This should fix almost all cases of the new C recursion limit preventing code that worked for 3.11 working for 3.12.

The original recursion limit of 1000 was to protect the C stack and to provide a meaningful error in case of runaway recursion.
Since calls to Python functions no longer consume C stack, protection of the C stack is independent of the depth of the Python stack. So we decoupled the two operations. The default Python recursion limit is unchanged at 1000 and can be modified by sys.setrecursionlimit(). However, the C recursion limit is set to 800, as the limit of 1000 was too large for some platforms, especially debug builds.

Historically it has been _PyEval_EvalFrameDefault() that consumes the most stack space, so that sets the (quite low) limit of 800 for C recursion.
We can allow deeper recursion of C code and preserve safety by increasing the C recursion counting a call to _PyEval_EvalFrameDefault() as several normal calls.

This PR raises the C recursion limit from 800 to 25001500 and counts each call to _PyEval_EvalFrameDefault() as three two normal calls.

If this is deemed insufficient, we could expose the means to modify the C recursion limit, but that might be too large a change for 3.12.

@Yhg1s
@gpshead

Lib/test/list_tests.py Outdated Show resolved Hide resolved
Include/cpython/pystate.h Outdated Show resolved Hide resolved
Lib/test/support/__init__.py Outdated Show resolved Hide resolved
@markshannon
Copy link
Member Author

With C_RECURSION_LIMIT set to 2500, the Windows (x64) tests failed with a stack overflow calling Py_Eval_EvalFrameDefault() recursively using the C API.
With the C_RECURSION_LIMIT set to 2400, the Windows (x64) tests failed with a stack overflow compiling a recursive AST node.

From this, it seems that the estimate of Py_Eval_EvalFrameDefault() being worth 3 normal calls is about right, but that a C recursion limit of 2400 is too large.
I've chosen 2000 as a compromise value.

@Yhg1s
Copy link
Member

Yhg1s commented Aug 1, 2023

The new C recursion limit is used the same way as the old (generic) recursion limit, which defaulted to 1000, right? Or are there things that use the C recursion limit that now count recursion differently? 2000 doesn't sound unreasonable, but if Python's own tests overrun the stack at 2400, maybe it's better to put it closer to the old limit to avoid causing problems with large stack frames in user (C) code.

@markshannon
Copy link
Member Author

markshannon commented Aug 2, 2023

I had thought that a limit of 2000 and _Py_Eval_EvalFrameDefault() counting as 3 frames seems the best, but the failures on Windows are, I think, due to recursion in obj2ast_expr (which is ~2000 lines long).
This adds weight to @Yhg1s's concern about large user C functions overflowing the stack.

A limit of 1500, counting _Py_Eval_EvalFrameDefault() as using two frames, looks like the best option.

@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 2, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @markshannon for commit a8c72d0 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 2, 2023
@Yhg1s Yhg1s self-assigned this Aug 2, 2023
@@ -222,7 +222,8 @@ struct _ts {
# ifdef __wasi__
# define C_RECURSION_LIMIT 500
# else
# define C_RECURSION_LIMIT 800
// This value is duplicated in Lib/test/support/__init__.py
# define C_RECURSION_LIMIT 1500
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the WASI conditional will have to be duplicated in test.support as well, for the WASI ast recursion test (https://buildbot.python.org/all/#/builders/1061/builds/526).

Copy link
Member Author

Choose a reason for hiding this comment

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

We already skip a few tests for WASI due to the limited stack use, so I'll just add another skip.
We can remove all of those for 3.13.

@python python deleted a comment from bedevere-bot Aug 3, 2023
@markshannon
Copy link
Member Author

!buildbot s390

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @markshannon for commit 0e8c992 🤖

The command will test the builders whose names match following regular expression: s390

The builders matched are:

  • s390x RHEL8 PR
  • s390x Fedora PR
  • s390x Fedora Clang PR
  • s390x RHEL8 Refleaks PR
  • s390x Fedora Clang Installed PR
  • s390x RHEL7 PR
  • s390x RHEL7 LTO + PGO PR
  • s390x Fedora LTO PR
  • s390x RHEL8 LTO PR
  • s390x Fedora LTO + PGO PR
  • s390x RHEL8 LTO + PGO PR
  • s390x RHEL7 LTO PR
  • s390x SLES PR
  • s390x Debian PR
  • s390x RHEL7 Refleaks PR
  • s390x Fedora Refleaks PR

@Yhg1s
Copy link
Member

Yhg1s commented Aug 3, 2023

I think all the buildbot failures are pre-existing ones (some of which have already been fixed in main), so is this good to check in?

@markshannon
Copy link
Member Author

I think all the buildbot failures are pre-existing ones (some of which have already been fixed in main), so is this good to check in?

Yes.
No new failures and none of the failures are recursion related.

@markshannon markshannon merged commit fa45958 into python:main Aug 4, 2023
17 checks passed
@markshannon markshannon added the needs backport to 3.12 bug and security fixes label Aug 4, 2023
@miss-islington
Copy link
Contributor

Thanks @markshannon for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-107618 is a backport of this pull request to the 3.12 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 4, 2023
…PyEval_EvalFrameDefault()` (pythonGH-107535)

* Set C recursion limit to 1500, set cost of eval loop to 2 frames, and compiler mutliply to 2.
(cherry picked from commit fa45958)

Co-authored-by: Mark Shannon <mark@hotpy.org>
@bedevere-bot bedevere-bot removed the needs backport to 3.12 bug and security fixes label Aug 4, 2023
Yhg1s pushed a commit that referenced this pull request Aug 4, 2023
…_PyEval_EvalFrameDefault()` (GH-107535) (#107618)

GH-107263: Increase C stack limit for most functions, except `_PyEval_EvalFrameDefault()` (GH-107535)

* Set C recursion limit to 1500, set cost of eval loop to 2 frames, and compiler mutliply to 2.
(cherry picked from commit fa45958)

Co-authored-by: Mark Shannon <mark@hotpy.org>
@markshannon markshannon deleted the increase-c-stack-limit-for-most-functions branch August 6, 2024 10:18
Comment on lines -47 to -50
# condition. Windows doesn't have os.uname() but it doesn't support s390x.
skip_on_s390x = unittest.skipIf(hasattr(os, 'uname') and os.uname().machine == 's390x',
'skipped on s390x')

Copy link
Member

@encukou encukou Oct 16, 2024

Choose a reason for hiding this comment

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

For reference: this moved skip_on_s390x away from its comment. #125041 fixes that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants