FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-31857: Make the behavior of USE_STACKCHECK deterministic by pdox · Pull Request #4098 · python/cpython · GitHub

/ cpython Public

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

Merged
benjaminp merged 3 commits into
python:masterfrom
pdox:det_stackcheck
Oct 26, 2017
Merged

bpo-31857: Make the behavior of USE_STACKCHECK deterministic#4098
benjaminp merged 3 commits into
python:masterfrom
pdox:det_stackcheck

Conversation

pdox commented Oct 24, 2017
edited by bedevere-bot
Loading

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor

Does this make _ceval_runtime_state.recursion_limit dead?

pdox commented Oct 24, 2017
edited
Loading

Copy link
Copy Markdown
Author

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 commented Oct 24, 2017

Copy link
Copy Markdown
Author

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.

Copy link
Copy Markdown
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 merged commit 1896793 into python:master Oct 26, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL