Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

markshannon
Copy link
Member

@markshannonmarkshannon commentedAug 1, 2023•
edited
Loading

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 bysys.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 to25001500 and counts each call to_PyEval_EvalFrameDefault() asthree 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

olife-png reacted with thumbs up emojiolife-png reacted with hooray emoji
@markshannon
Copy link
MemberAuthor

WithC_RECURSION_LIMIT set to 2500, the Windows (x64) tests failed with a stack overflow callingPy_Eval_EvalFrameDefault() recursively using the C API.
With theC_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 ofPy_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

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
MemberAuthor

markshannon commentedAug 2, 2023•
edited
Loading

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 inobj2ast_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.

@markshannonmarkshannon added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelAug 2, 2023
@bedevere-bot
Copy link

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

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

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelAug 2, 2023
@Yhg1sYhg1s self-assigned thisAug 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
MemberAuthor

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.

Yhg1s reacted with thumbs up emoji
@pythonpython deleted a comment frombedevere-botAug 3, 2023
@markshannon
Copy link
MemberAuthor

!buildbot s390

@bedevere-bot
Copy link

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

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

Ithink 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
MemberAuthor

Ithink 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.

@markshannonmarkshannon merged commitfa45958 intopython:mainAug 4, 2023
@markshannonmarkshannon added the needs backport to 3.12only security fixes labelAug 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 the3.12 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestAug 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 commitfa45958)Co-authored-by: Mark Shannon <mark@hotpy.org>
@bedevere-botbedevere-bot removed the needs backport to 3.12only security fixes labelAug 4, 2023
Yhg1s pushed a commit that referenced this pull requestAug 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 commitfa45958)Co-authored-by: Mark Shannon <mark@hotpy.org>
@markshannonmarkshannon deleted the increase-c-stack-limit-for-most-functions branchAugust 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

@encukouencukouOct 16, 2024•
edited
Loading

Choose a reason for hiding this comment

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

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@encukouencukouencukou left review comments

@Yhg1sYhg1sYhg1s approved these changes

@isidenticalisidenticalAwaiting requested review from isidenticalisidentical is a code owner

@iritkatrieliritkatrielAwaiting requested review from iritkatrieliritkatriel is a code owner

Assignees

@Yhg1sYhg1s

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@markshannon@Yhg1s@bedevere-bot@miss-islington@encukou
[8]ページ先頭
©2009-2025 Movatter.jp