bpo-32307: thread: fix stack size on musl platforms. #26224
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| Force a default thread stack size value for all platforms instead of | ||
| assuming that they all have enough for Python threads. This makes it so too | ||
| many recursions are caught by the interpreter instead of stack overflowing | ||
| on musl libc. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,7 +29,9 @@ | |
| be conditional on _POSIX_THREAD_ATTR_STACKSIZE being defined. */ | ||
| #ifdef _POSIX_THREAD_ATTR_STACKSIZE | ||
| #ifndef THREAD_STACK_SIZE | ||
| /* -1 means THREAD_STACK_SIZE hasn't been given a proper value yet. | ||
| -DTHREAD_STACK_SIZE=0 in CFLAGS will force the platform's default instead. */ | ||
| #define THREAD_STACK_SIZE -1 | ||
| #endif | ||
|
|
||
| /* The default stack size for new threads on OSX and BSD is small enough that | ||
|
|
@@ -39,33 +41,35 @@ | |
| * The default stack sizes below are the empirically determined minimal stack | ||
| * sizes where a simple recursive function doesn't cause a hard crash. | ||
| */ | ||
| #if defined(__APPLE__) && THREAD_STACK_SIZE == -1 | ||
| #undef THREAD_STACK_SIZE | ||
| /* Note: This matches the value of -Wl,-stack_size in configure.ac */ | ||
| #define THREAD_STACK_SIZE 0x1000000 | ||
| #endif | ||
| #if defined(__FreeBSD__) && THREAD_STACK_SIZE == -1 | ||
| #undef THREAD_STACK_SIZE | ||
| #define THREAD_STACK_SIZE 0x400000 | ||
| #endif | ||
| /* bpo-38852: test_threading.test_recursion_limit() checks that 1000 recursive | ||
| Python calls (default recursion limit) doesn't crash, but raise a regular | ||
| RecursionError exception. In debug mode, Python function calls allocates | ||
| more memory on the stack, so use a stack of 8 MiB. */ | ||
| #if defined(__ANDROID__) && THREAD_STACK_SIZE == -1 | ||
| # ifdef Py_DEBUG | ||
| # undef THREAD_STACK_SIZE | ||
| # define THREAD_STACK_SIZE 0x800000 | ||
| # endif | ||
| #endif | ||
| #if defined(__VXWORKS__) && THREAD_STACK_SIZE == -1 | ||
| #undef THREAD_STACK_SIZE | ||
| #define THREAD_STACK_SIZE 0x100000 | ||
| #endif | ||
| #if THREAD_STACK_SIZE == -1 | ||
|
There was a problem hiding this comment. this |
||
| #undef THREAD_STACK_SIZE | ||
| /* This is big enough for most platforms without being wasteful. */ | ||
|
There was a problem hiding this comment. do we have a unittest in the stdlib that exercises the default recursion limit to on a thread to ensure the process doesn't crash? (it'll be imperfect as different internal call recursion codepaths have different C stack needs, but it still serves as a good failsafe; perhaps have the test raise recursionlimit to 1.5x its default value). https://github.com/python/cpython/blob/v3.10.0/Lib/test/test_threading.py#L1234 test_threading.test_recursion_limit exists. We should probably change that to raise the recursion limit a bit above the default in it's thread for some safety margin. |
||
| #define THREAD_STACK_SIZE 0x200000 | ||
| #endif | ||
|
|
||
| /* for safety, ensure a viable minimum stacksize */ | ||
| #define THREAD_STACK_MIN 0x8000 /* 32 KiB */ | ||
| #else /* !_POSIX_THREAD_ATTR_STACKSIZE */ | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to avoid disruption for anyone who may already be supplying CFLAGS containing -DTHREAD_STACK_SIZE=0 in their builds already keep the existing 0 behavior: 0 should be the platform specific values below.
If we want a new value in some circumstances, that should either be enabled via an appropriate test in
configure.acand thus via pyconfig.h. Or yet another platform specific compile time#ifcheck should be used here to detect musl libc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, the purpose here is to make the code universal. Right now, the only platform where the default thread size actually works is glibc. Instead of adding a case for each and every platform other than glibc, IMO it makes much more sense to have a sane default and override for one or two platforms as necessary, instead.
Testing for musl is unsupported, they don't provide any identifying macros, by design.