Skip to content
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

bpo-46329: Reduce default recursion to 800 if Py_DEBUG is enabled. #31033

Closed
wants to merge 2 commits into from

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Jan 31, 2022

Prevents C stack overflow on Clang debug builds.

This is a temporary fix to get the buildbots working again.
This should be reverted in time for the 3.11 beta.

@pablogsal
As it looks like #31011 has failed, would this be an acceptable, temporary fix?

https://bugs.python.org/issue46329

@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 31, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit b2d5700 🤖

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 31, 2022
@pablogsal
Copy link
Member

I am ok to go this way if we don't manage to properly fix the issue today or tomorrow.

@pablogsal
Copy link
Member

We should also consider asking clang for more stack space as someone also suggested.

@markshannon
Copy link
Member Author

I think you and I have different ideas about what "properly" is in this context.
To me, that means handling C stack overflows, and that isn't happening today or tomorrow. Soon hopefully, but not that soon.

Even if #31011 had passed the tests, we would be very close to stack overflow on the clang debug build, such that any number of otherwise harmless changes could cause a stack overflow. Any "proper" fix, would need to provide a decent safety margin.

Asking clang for more stack space would work, if it has such an option.

@pablogsal
Copy link
Member

I think you and I have different ideas about what "properly" is in this context.
To me, that means handling C stack overflows, and that isn't happening today or tomorrow. Soon hopefully, but not that soon.

Sorry, I think I didn't explain myself correctly. You are talking about some fix to avoid the C stack overflow but I'm taking about just getting the buildbots green without introducing any user visible changes (such as reducing the recursion depth in debug builds).

The issue of resolving the stack overflow problem in general still stands and indeed is worth a more propper solution as you describe 👍

@lazka
Copy link
Contributor

lazka commented Jan 31, 2022

Asking clang for more stack space would work, if it has such an option.

We use this in the mingw port for both gcc/clang: msys2-contrib@6fcebb6 to mirror a6a116c

@gvanrossum gvanrossum changed the title bpo-46329: Temporarily reduce default recursion to 800 if Py_DEBUG is enabled. bpo-46329: Reduce default recursion to 800 if Py_DEBUG is enabled. Jan 31, 2022
@markshannon
Copy link
Member Author

This is no longer needed, as #31035 did the job.

@markshannon markshannon closed this Feb 1, 2022
@vstinner
Copy link
Member

vstinner commented Feb 1, 2022

For the specific case of clang with ./configure --with-pydebug, I wrote PR #31052 which reduces the f() stack usage from 9,104 bytes to 736 bytes.

See also https://bugs.python.org/issue46600

@markshannon markshannon deleted the debug-recursion-800 branch September 26, 2023 12:52
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.

6 participants