bpo-21131: don't use SIGSTKSZ for stack size in sigaltstack #13649
Conversation
auvipy
left a comment
auvipy
left a comment
There was a problem hiding this comment.
news entry needed if it is not private.
|
Is there any chance that the priority of this could get promoted prior to the next release? This causes the FaultHandler test to segfault when compiling on the Skylake architecture. This seems somewhat serious, yet it's not been reviewed, and I think perhaps is should not be a type-enhancement but an actual bug? Adding @ambv here in lieu of sending mail per the release candidate notes at |
This function gives a value, in bytes, that represents a usable amount of memory to use for a stack on the host system. The existing thread module does provide THREAD_STACKSIZE, but this is only set if the pthreads implementation supports changing thread stack sizes, and is nominally set to 0, to indicate that pthreads can choose the stack size of new threads. We need to be able to create stacks for other reasons, however, and it is useful to be able to query the system for it's preferred stack size. (specifically for faulthandler and sigaltstack()) We also provide an implementation for pthread-nt.h, that dumbly returns 1MB. This will not currently be used, as sigaltstack is not being used for faulthandler on Windows. This is apparently the default windows stack size as inserted into the executable header by the linker. I don't know enough win32 API to work out how to extract the value for the current exe.
The system-defined SIGSTKSZ is inadequate for Linux on modern x86_64 CPUs when handling multiple nested signal handlers and dynamic linking The value is very much for a minimal viable stack, and is not enough to run user-supplied python code. _Pythread_preferred_stack_size will nominally return the stack size the system would use to create thrads.
|
I just updated the Bug report at https://bugs.python.org/issue21131 to report that this only seems to apply when Python is configured to use shared libraries. Using leads to leads to the I am not sure why that is, but it may explain some people's being unable to reproduce the problem. |
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
I'm not comfortable with this change since it modify the core thread module only to fix a faulthandler unit test. faulthandler uses SIGSTKSZ. It's not obvious to me how it relates to the "preferred thread stack size".
| * Windows default stack size is 1MB, overridable by the linker in the | ||
| * executable | ||
| */ | ||
| return 1024 * 1024; |
There was a problem hiding this comment.
I'm not comfortable with hardcoded values, especially the comment "overridable by the linker in the executable". Maybe we should get the size written by the linker in this case? We don't really need this function for faulthandler, maybe remove the Windows implementation?
There was a problem hiding this comment.
Sure - I added it for completeness/documentation, but it's not called. I can remove it.
| #undef THREAD_STACK_SIZE | ||
| #if defined(__APPLE__) | ||
| /* Note: This matches the value of -Wl,-stack_size in configure.ac */ | ||
| #define THREAD_STACK_SIZE 0x1000000 |
There was a problem hiding this comment.
Previously, THREAD_STACK_SIZE was redefined if THREAD_STACK_SIZE=0. With your patch, it seems like it's always overriden: THREAD_STACK_SIZE value is no longer tested. I would prefer to minimize changes in this PR.
There was a problem hiding this comment.
The end result is the same, but with less code - Originally THREAD_STACK_SIZE was always defined to 0 if not previously defined, and then was undefined/redefined if it was 0 in response to the platform detection macros. The code here was really just trying to extract the platform's preferred stack size (as reported by pthreads), and to not replicate this between the threads and faulthandler modules. Let me minimize the diff then.
| } | ||
|
|
||
| size_t | ||
| _PyThread_preferred_stacksize() |
There was a problem hiding this comment.
| _PyThread_preferred_stacksize() | |
| _PyThread_preferred_stacksize(void) |
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Co-Authored-By: Victor Stinner <[email protected]>
Co-Authored-By: Victor Stinner <[email protected]>
|
The bug tracker issue was resolved with GH-15276, so I am going to close this pull request. |
8 participants
Following up discussion on http://bugs.python.org/issue21131
vstinner suggested using the logic in
thread_pthread.hto work out the "good" stack size for sigaltstack. I've integrated the logic in there, but it doesn't actually provide an accessible way of finding the default thread stack size - it just nominally requests pthread_create to create a stack of a "default" size by not poking at the stack size attribute.So, instead, I've rejigged the macros in that file to provide PLATFORM_STACK_SIZE and PLATFORM_STACK_PREFERRED that are available regardless of the ability of pthreads to set the thread stack size attribute (i.e., regardless of the presence of _POSIX_THREAD_ATTR_STACK_SIZE) (and simplified some of the conditionals in the code using THREAD_STACK_SIZE in the process). The eventual sizes past to
pthread_attr_setstacksizeare unaffected.This is then used as a fallback in the new function,
size_t _Pthread_preferred_stacksize(), which will first attempt to use a default-constructed pthread_attr_t to work out at runtime what size stack the system prefers for new threads. This value is far more likely to be usable than the value defined in SIGSTKSZ.If that call fails, or the thread size attribute is not available, we default to the maximum of PTHREAD_STACK_MIN, whatever we've defined for PLATFORM_STACK_SIZE for those platforms with an inadequate default stacksize, or finally the arbitrary 32k setting previously used as a floor.
All feedback gratefully appreciated.
https://bugs.python.org/issue21131