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

markshannon · 2023-08-01T12:34:24Z

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 ~~2500~~1500 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

Issue: Regression in 3.12 beta in json.dump deeply nested dict #107263

…and compiler mutliply to 2.

Lib/test/list_tests.py

Include/cpython/pystate.h

Lib/test/support/__init__.py

markshannon · 2023-08-01T14:27:39Z

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 · 2023-08-01T15:05:22Z

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.

Misc/NEWS.d/next/Core and Builtins/2023-07-30-05-20-16.gh-issue-107263.q0IU2M.rst

markshannon · 2023-08-02T11:47:37Z

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.

bedevere-bot · 2023-08-02T11:47:51Z

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

Yhg1s · 2023-08-02T13:00:35Z

Include/cpython/pystate.h

@@ -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


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

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.

markshannon · 2023-08-03T15:07:16Z

!buildbot s390

bedevere-bot · 2023-08-03T15:07:18Z

🤖 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 · 2023-08-03T22:20:47Z

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 · 2023-08-04T09:09:59Z

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.

miss-islington · 2023-08-04T09:10:43Z

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

bedevere-bot · 2023-08-04T09:10:54Z

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

…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>

…_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>

encukou · 2024-10-16T11:33:11Z

Lib/test/test_zlib.py

-# 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')
-


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

markshannon added 10 commits July 30, 2023 04:55

Increase C stack limit for most functions, except PyEval_EvalDefault()

18f16d3

Add news and what's new item.

8d05620

Reduce C_RECURSION_LIMIT to 2400. 2500 was too high.

aa78add

Reduce C recursion limit to 2000. 2400 is too hgh for Windows.

d6793ea

Use symbolic limit rather than hard coded value for recursion tests.

c0af892

Reduce C recursion limit to 1500, set cost of eval loop to 2 frames, …

534a36c

…and compiler mutliply to 2.

Fix comment.

3bb0cf0

Fix news entry

a8c72d0

Skip test for WASI due to limited stack depth

7e6a521

Skip test on s390x

0e8c992

markshannon requested review from isidentical and iritkatriel as code owners August 1, 2023 12:34

bedevere-bot added the awaiting core review label Aug 1, 2023

bedevere-bot mentioned this pull request Aug 1, 2023

Regression in 3.12 beta in json.dump deeply nested dict #107263

Closed

Yhg1s reviewed Aug 1, 2023

View reviewed changes

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

Yhg1s approved these changes Aug 1, 2023

View reviewed changes

Misc/NEWS.d/next/Core and Builtins/2023-07-30-05-20-16.gh-issue-107263.q0IU2M.rst Outdated Show resolved Hide resolved

bedevere-bot added awaiting merge and removed awaiting core review labels Aug 1, 2023

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

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 2, 2023

Yhg1s self-assigned this Aug 2, 2023

Yhg1s reviewed Aug 2, 2023

View reviewed changes

python deleted a comment from bedevere-bot Aug 3, 2023

markshannon merged commit fa45958 into python:main Aug 4, 2023
17 checks passed

bedevere-bot removed the awaiting merge label Aug 4, 2023

markshannon added the needs backport to 3.12 bug and security fixes label Aug 4, 2023

bedevere-bot removed the needs backport to 3.12 bug and security fixes label Aug 4, 2023

vstinner mentioned this pull request Nov 15, 2023

test_super_deep() for test_call triggers a stack overflow protection under a debug build for WASI #111798

Closed

markshannon deleted the increase-c-stack-limit-for-most-functions branch August 6, 2024 10:18

encukou mentioned this pull request Oct 7, 2024

Re-enable skipped test_zlib tests on s390x hardware acceleration #125041

Open

encukou reviewed Oct 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

markshannon commented Aug 1, 2023 •

edited

Loading

markshannon commented Aug 1, 2023

Yhg1s commented Aug 1, 2023

markshannon commented Aug 2, 2023 •

edited

Loading

bedevere-bot commented Aug 2, 2023

Yhg1s Aug 2, 2023

markshannon Aug 2, 2023

markshannon commented Aug 3, 2023

bedevere-bot commented Aug 3, 2023

Yhg1s commented Aug 3, 2023

markshannon commented Aug 4, 2023

miss-islington commented Aug 4, 2023

bedevere-bot commented Aug 4, 2023

encukou Oct 16, 2024 •

edited

Loading

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

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

Conversation

markshannon commented Aug 1, 2023 • edited Loading

markshannon commented Aug 1, 2023

Yhg1s commented Aug 1, 2023

markshannon commented Aug 2, 2023 • edited Loading

bedevere-bot commented Aug 2, 2023

Yhg1s Aug 2, 2023

Choose a reason for hiding this comment

markshannon Aug 2, 2023

Choose a reason for hiding this comment

markshannon commented Aug 3, 2023

bedevere-bot commented Aug 3, 2023

Yhg1s commented Aug 3, 2023

markshannon commented Aug 4, 2023

miss-islington commented Aug 4, 2023

bedevere-bot commented Aug 4, 2023

encukou Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

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

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

markshannon commented Aug 1, 2023 •

edited

Loading

markshannon commented Aug 2, 2023 •

edited

Loading

encukou Oct 16, 2024 •

edited

Loading