Skip to content

bpo-31857: Make the behavior of USE_STACKCHECK deterministic #4098

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
Oct 26, 2017
Merged

bpo-31857: Make the behavior of USE_STACKCHECK deterministic #4098

merged 3 commits into from
Oct 26, 2017

Conversation

pdox
Copy link

@pdox pdox commented Oct 24, 2017

Add a counter in the thread state object to trigger USE_STACKCHECK invocations every 64 times Py_EnterRecursiveCall is called in a thread.

https://bugs.python.org/issue31857

@benjaminp
Copy link
Contributor

Does this make _ceval_runtime_state.recursion_limit dead?

@pdox
Copy link
Author

pdox commented Oct 24, 2017

Hm, this is a bit confusing. There are three different values:

ceval.recursion_limit - Used by Py_GetRecursionLimit
ceval.check_recursion_limit - Not used at all
_Py_CheckRecursionLimit - Used by the ceval macros

Then there's this comment:

/* XXX _Py_CheckRecursionLimit should be changed to
   _PyRuntime.ceval.check_recursion_limit.  However, due to the macros
   in which it's used, _Py_CheckRecursionLimit is stuck in the stable
   ABI.  It should be removed therefrom when possible.
*/
PyAPI_DATA(int) _Py_CheckRecursionLimit;

I think the idea was for check_recursion_limit to be the replacement for _Py_CheckRecursionLimit, as a mutable counter field for USE_STACKCHECK, with recursion_limit being constant. So technically, this change makes both _Py_CheckRecursionLimit and check_recursion_limit obsolete. But if we really want to maintain it as part of the "ABI", then we need to keep it around. I doubt the existing code is actually ABI stable though, since the existing macro was:

(++(x) > (_Py_CheckRecursionLimit += PyThreadState_GET()->overflowed - 1))

Which assumes the offset of "overflowed" in the PyThreadState structure is part of the ABI, which it is explicitly not. ( http://legacy.python.org/dev/peps/pep-0384/#structures )

But if we want to keep it compatible, I could add the assignment:

_Py_CheckRecursionLimit = _PyRuntime.ceval.recursion_limit;

in _Py_CheckRecursiveCall. This would preserve the old behavior when the old macro is in use.

@pdox
Copy link
Author

pdox commented Oct 24, 2017

My preference would be to remove _Py_CheckRecursionLimit and check_recursion_limit, but if you think ABI compatibility for this feature is important, I could remove check_recursion_limit and make the behavior of _Py_CheckRecursionLimit backwards-compatible.

@benjaminp
Copy link
Contributor

Yeah, to actually provide a stable ABI, _Py_CheckRecursionLimit should be a function. However, completely cleaning that up is probably outside the scope of this PR. Since _Py_CheckRecursionLimit is the one that's primarily used right now, I'd suggest nixing check_recursion_limit.

@benjaminp benjaminp merged commit 1896793 into python:master Oct 26, 2017
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.

3 participants