Skip to content

GH-91079: Rename C_RECURSION_LIMIT to Py_C_RECURSION_LIMIT #108507

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

Merged
merged 3 commits into from
Sep 8, 2023

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Aug 26, 2023

Symbols of the C API should be prefixed by "Py_" to avoid conflict with existing names in 3rd party C extensions on #include <Python.h>.

@vstinner
Copy link
Member Author

@iritkatriel: Are you ok with this rename?

@vstinner
Copy link
Member Author

cc @markshannon

@@ -197,12 +197,12 @@ struct _ts {
/* 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 C_RECURSION_LIMIT
#ifndef Py_C_RECURSION_LIMIT
Copy link
Member

Choose a reason for hiding this comment

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

Do users define this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if it's relevant to support that. It's supposed to be a constant built into Python, no?

@vstinner
Copy link
Member Author

@iritkatriel: I removed #ifndef Py_C_RECURSION_LIMIT. Would you mind to review again the PR?

@vstinner vstinner removed the needs backport to 3.12 only security fixes label Sep 6, 2023
@vstinner
Copy link
Member Author

vstinner commented Sep 6, 2023

vstinner removed the needs backport to 3.12

It's too late to backport to Python 3.12.

@vstinner
Copy link
Member Author

vstinner commented Sep 7, 2023

I don't think that it makes sense to override the Py_C_RECURSION_LIMIT constant. Python does hardcode this constant in its code. For example, init_threadstate() sets tstate->c_recursion_remaining = Py_C_RECURSION_LIMIT;.

Building Python with a different Py_C_RECURSION_LIMIT value sounds like a borderline usecase. If someone cares about fine tuning Py_C_RECURSION_LIMIT, I would suggest adding a ./configure option. Currently, building Python with a different C_RECURSION_LIMIT value leads to inconsistencies for C extensions built with the default Py_C_RECURSION_LIMIT value (1500).

Including <Python.h> and overriding Py_C_RECURSION_LIMIT sounds like a bad idea: Python will anyway uses its hardcoded constants (ex: in init_threadstate()), changing the constant doesn't change Python behavior. It only leads to inconsistency.

In short, IMO it's a good thing to remove #ifndef C_RECURSION_LIMIT :-)

Symbols of the C API should be prefixed by "Py_" to avoid conflict
with existing names in 3rd party C extensions on #include <Python.h>.
test.pythoninfo logs Py_C_RECURSION_LIMIT and other _testcapi and
_testinternalcapi constants.
@vstinner
Copy link
Member Author

vstinner commented Sep 8, 2023

Since Py_C_RECURSION_LIMIT value is different on WASI, I modified test.pythoninfo to log the value, to ease debugging test failures related to recursion. Recently, I added sys.getrecursionlimit to test.pythoninfo when I fixed test_tomllib recursion error (commit 8ff1142).

@vstinner vstinner enabled auto-merge (squash) September 8, 2023 09:09
@vstinner vstinner merged commit b0edf3b into python:main Sep 8, 2023
@vstinner vstinner deleted the c_recursion_limit branch September 8, 2023 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants