GH-91079: Decouple C stack overflow checks from Python recursion checks. #96510

markshannon · 2022-09-02T16:32:11Z

Issue: Implement stack overflow protection for supported platforms #91079

bedevere-bot · 2022-09-05T13:54:49Z

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

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

bedevere-bot · 2022-09-05T14:45:54Z

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

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

bedevere-bot · 2022-09-06T10:57:14Z

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

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

bedevere-bot · 2022-09-13T08:43:33Z

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

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

gpshead · 2022-10-04T05:26:24Z

Lib/test/test_dynamic.py

-        # 1,000 on most systems
-        limit = sys.getrecursionlimit()
-        code = "lambda: " + "+".join(f"_number_{i}" for i in range(limit))
+        # Need more than 256 variables to use EXTENDED_ARGS


I assume EXTENDED_ARGS has stack implications? explaining the "why" of this here would be useful.

gpshead · 2022-10-04T05:26:24Z

Lib/test/test_exceptions.py

-                # and is equal to recursion_limit when _gen_throw() calls
-                # PyErr_NormalizeException().
-                recurse(setrecursionlimit(depth + 2) - depth)
+                recurse(5000)


There's a constant of 5000 used in all sorts of tests for the same purpose of being "too high" for recursion with this PR. I suggest making this a named test.support constant and referring to it instead of mystery constants spread throughout the test suite.

(and where other constants are derived from that to be higher or lower scale, use math from the main value?)

gpshead · 2022-10-04T05:26:24Z

Include/cpython/pystate.h

+/* WASI has limited call stack. Python's recursion limit depends on code
+   layout, optimization, and WASI runtime. Wasmtime can handle about 700
+   recursions, sometimes less. 500 is a more conservative limit. */
+#ifndef Py_DEFAULT_RECURSION_LIMIT


ifndef C_RECURSION_LIMIT

gpshead · 2022-10-04T05:26:24Z

Include/internal/pycore_ceval.h

-#    define Py_DEFAULT_RECURSION_LIMIT 1000
-#  endif
-#endif
+#define Py_DEFAULT_RECURSION_LIMIT 1000


keep the ifndef around this, those exist to allow someone setting their own via CFLAGS.

gpshead

this tied in nicely with the 3.11 talk :)

tacaswell · 2022-10-07T13:29:10Z

This broke compilation of https://github.com/python-greenlet/greenlet/

✔ 20:08:04 $ gcc -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -I/home/tcaswell/.virtualenvs/bleeding/include -I/home/tcaswell/.pybuild/bleeding/include/python3.12 -c src/greenlet/greenlet.cpp -o build/temp.linux-x86_64-3.12/src/greenlet/greenlet.o
In file included from src/greenlet/greenlet_internal.hpp:19,
                 from src/greenlet/greenlet.cpp:17:
src/greenlet/greenlet_greenlet.hpp: In member function ‘void greenlet::PythonState::operator<<(const PyThreadState*)’:
src/greenlet/greenlet_greenlet.hpp:826:37: error: ‘const PyThreadState’ {aka ‘const struct _ts’} has no member named ‘recursion_limit’; did you mean ‘py_recursion_limit’?
  826 |     this->recursion_depth = tstate->recursion_limit - tstate->recursion_remaining;
      |                                     ^~~~~~~~~~~~~~~
      |                                     py_recursion_limit
src/greenlet/greenlet_greenlet.hpp:826:63: error: ‘const PyThreadState’ {aka ‘const struct _ts’} has no member named ‘recursion_remaining’; did you mean ‘c_recursion_remaining’?
  826 |     this->recursion_depth = tstate->recursion_limit - tstate->recursion_remaining;
      |                                                               ^~~~~~~~~~~~~~~~~~~
      |                                                               c_recursion_remaining
src/greenlet/greenlet_greenlet.hpp: In member function ‘void greenlet::PythonState::operator>>(PyThreadState*)’:
src/greenlet/greenlet_greenlet.hpp:859:13: error: ‘PyThreadState’ {aka ‘struct _ts’} has no member named ‘recursion_remaining’; did you mean ‘c_recursion_remaining’?
  859 |     tstate->recursion_remaining = tstate->recursion_limit - this->recursion_depth;
      |             ^~~~~~~~~~~~~~~~~~~
      |             c_recursion_remaining
src/greenlet/greenlet_greenlet.hpp:859:43: error: ‘PyThreadState’ {aka ‘struct _ts’} has no member named ‘recursion_limit’; did you mean ‘py_recursion_limit’?
  859 |     tstate->recursion_remaining = tstate->recursion_limit - this->recursion_depth;
      |                                           ^~~~~~~~~~~~~~~
      |                                           py_recursion_limit
src/greenlet/greenlet_greenlet.hpp: In member function ‘void greenlet::PythonState::set_initial_state(const PyThreadState*)’:
src/greenlet/greenlet_greenlet.hpp:886:37: error: ‘const PyThreadState’ {aka ‘const struct _ts’} has no member named ‘recursion_limit’; did you mean ‘py_recursion_limit’?
  886 |     this->recursion_depth = tstate->recursion_limit - tstate->recursion_remaining;
      |                                     ^~~~~~~~~~~~~~~
      |                                     py_recursion_limit
src/greenlet/greenlet_greenlet.hpp:886:63: error: ‘const PyThreadState’ {aka ‘const struct _ts’} has no member named ‘recursion_remaining’; did you mean ‘c_recursion_remaining’?
  886 |     this->recursion_depth = tstate->recursion_limit - tstate->recursion_remaining;
      |                                                               ^~~~~~~~~~~~~~~~~~~
      |                                                               c_recursion_remaining

Doing the renames as suggested by the compiler fixes it (although from reading the PR here, maybe py_recursion_remaining should be used instead of c_recursion_remaining).

Is this an unintended side effect of this change and or does greenlets need to adapt?

gpshead · 2022-10-07T17:22:37Z

Not being familiar with greenlet much I suspect it wants py_recursion_remaining. This is a rather internal structure that changes between releases. https://docs.python.org/3.11/whatsnew/3.11.html has a change around these that you've adapted to in https://github.com/python-greenlet/greenlet/blob/master/src/greenlet/greenlet_greenlet.hpp#L825 so this is probably just another one that we need to document in 3.12's "What's New".

gpshead · 2022-10-07T17:23:17Z

BTW @tacaswell major kudos to you for testing against CPython main =)

tacaswell · 2022-10-07T18:45:20Z

Thanks, I'll get a PR to greenlet done in the next few days (I am also barely familiar with greenlet but it is a dependency of a dependency of something I care about).

I have built a whole rube-goldberg machine to test CPython main (https://github.com/tacaswell/build_the_world).

…n checks. (pythonGH-96510)

markshannon added 2 commits Sep 2, 2022

Separate Python recursion tracking from C stack tracking.

de4085f

Add tests for super deep recursion.

6b380eb

bedevere-bot added the awaiting core review label Sep 2, 2022

markshannon added 4 commits Sep 2, 2022

Fixup Windows code.

b64c83b

Fixup asdl_c.py

e29a4cb

Use C recursion limit to determine AST recursion limits.

4897476

Add news.

e8a1bed

markshannon marked this pull request as ready for review Sep 5, 2022

markshannon requested review from isidentical and iritkatriel as code owners Sep 5, 2022

markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 5, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 5, 2022

Reduce C recursion depth on WASI.

7e21a74

markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 5, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 5, 2022

Reduce AST depth to get WASI test to pass.

65cca7d

markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 6, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 6, 2022

Merge branch 'main' into stack-overflow-3

d75797c

markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 13, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 13, 2022

markshannon requested a review from gpshead Sep 15, 2022

gpshead added the sprint label Oct 3, 2022

gpshead self-assigned this Oct 4, 2022

gpshead reviewed Oct 4, 2022

View changes

markshannon requested a review from rhettinger as a code owner Oct 4, 2022

markshannon requested a review from tiran as a code owner Oct 4, 2022

Address gps's review comments.

d1abed5

markshannon force-pushed the stack-overflow-3 branch from 63f55aa to d1abed5 Compare Oct 4, 2022

gpshead approved these changes Oct 4, 2022

View changes

bedevere-bot added awaiting merge and removed awaiting core review labels Oct 4, 2022

markshannon merged commit 7644935 into python:main Oct 5, 2022
14 checks passed

bedevere-bot removed the awaiting merge label Oct 5, 2022

mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022

pythonGH-91079: Decouple C stack overflow checks from Python recursio…

0ba4aad

…n checks. (pythonGH-96510)

tacaswell mentioned this pull request Oct 29, 2022

support for python3.12 python-greenlet/greenlet#323

Open

GH-91079: Decouple C stack overflow checks from Python recursion checks. #96510

GH-91079: Decouple C stack overflow checks from Python recursion checks. #96510

markshannon commented Sep 2, 2022 •

edited by bedevere-bot

bedevere-bot commented Sep 5, 2022

bedevere-bot commented Sep 5, 2022

bedevere-bot commented Sep 6, 2022

bedevere-bot commented Sep 13, 2022

gpshead Oct 4, 2022

gpshead Oct 4, 2022

gpshead Oct 4, 2022

gpshead Oct 4, 2022

gpshead Oct 4, 2022

gpshead left a comment

tacaswell commented Oct 7, 2022

gpshead commented Oct 7, 2022

gpshead commented Oct 7, 2022

tacaswell commented Oct 7, 2022 •

edited

GH-91079: Decouple C stack overflow checks from Python recursion checks. #96510

GH-91079: Decouple C stack overflow checks from Python recursion checks. #96510

Conversation

markshannon commented Sep 2, 2022 • edited by bedevere-bot

bedevere-bot commented Sep 5, 2022

bedevere-bot commented Sep 5, 2022

bedevere-bot commented Sep 6, 2022

bedevere-bot commented Sep 13, 2022

gpshead Oct 4, 2022

Choose a reason for hiding this comment

gpshead Oct 4, 2022

Choose a reason for hiding this comment

gpshead Oct 4, 2022

Choose a reason for hiding this comment

gpshead Oct 4, 2022

Choose a reason for hiding this comment

gpshead Oct 4, 2022

Choose a reason for hiding this comment

gpshead left a comment

tacaswell commented Oct 7, 2022

gpshead commented Oct 7, 2022

gpshead commented Oct 7, 2022

tacaswell commented Oct 7, 2022 • edited

markshannon commented Sep 2, 2022 •

edited by bedevere-bot

tacaswell commented Oct 7, 2022 •

edited