Hi Victor, thanks for the comments. Responses inline below.

On Wed, 14 Aug 2019 at 12:25, STINNER Victor <report@bugs.python.org> wrote:

>
> STINNER Victor <vstinner@redhat.com> added the comment:
>
> I dislike PR 13649 because it touch the thread module, only to fix a
> faulthandler unit test.

My original patch (posted in the comments above) was purely in faulthandler
- I went at the threading code on your suggestion:

PyThread_start_new_thread() of thread_pthread.h already contains logic to
> get a "good" stack size. I would prefer to reuse this code."

I have no problem reformulating the code to avoid touching the threads
library - let me redo it as such.

> The relationship between thread stack size and faulthandler is not obvious
> to me. Currently, faulthandler uses SIGSTKSZ, not the thread stack size.
> faulthandler usage of the stack should be quite low: it should need less
> than 1 MiB for example.
>

The point of contention here is really he choice of stack size. SIGSTKSZ is
ridiculously small - it is the bare minimum amount of memory required to
actually handle the signal. The signal handling mechanism eats a huge chunk
of it, and the dynamic linker can also eat several K too. The intent was to
use the default thread stack size as a heuristic for what the platform
considers to be a reasonable size stack for applications. If the
pthread-aware OS is somehow constrained for address space, then I'd expect
it to reflect that in the default stack size. For 32-bit linux, the 8M of
address space is a bit of a chunk, but it's not a huge proportion of the
3-4G you have, and you're not consuming actual memory. On a 64-bit system,
consuming 8M of address space is a drop in the ocean.

>
> "When faulthandler.c uses sigaltstack(2), the stack size is set up with a
> buffer of size SIGSTKSZ. That is, sadly, only 8k."
>
> "A chained signal handler that needs to invoke dynamic linking will
> therefore consume more than the default stack space allocated in
> faulthandler.c, just in machine-state saves alone. So, the failing test is
> failing because its scribbling on random memory before the allocated stack
> space."
>
> Aha, that's interesting: SIGSTKSZ should be enough for 1 signal handler,
> but test_register_chain calls 2 signal handlers using the same stack. Can
> you please try the following patch?
>

It's more complex than that - in dynamically linked applications when you
call functions that still need to be resolved by the dynamic linker, the
resolving thunk in the PLT also ends up saving the register state via
xsavec, so with a chained call, there are up to 3 register states saved on
the stack, each over 2.5k on actual hardware we have now. I'm not convinced
there are not other ways stack space will be consumed during the signal
handler, and I'm not convinced that the amount of memory required per
handler will not go up as new CPUs come out, and I'm not convinced that
SIGSTKSZ will be bumped to reflect that (it certainly hasn't in the past),
so scaling SIGSTKSZ like this, while it'll likely fix the problem on any
machine I can test it on, doesn't seem like a stable solution

> diff --git a/Modules/faulthandler.c b/Modules/faulthandler.c
> index 2331051f79..e7d13f2b2d 100644
> --- a/Modules/faulthandler.c
> +++ b/Modules/faulthandler.c
> @@ -1325,7 +1325,7 @@ _PyFaulthandler_Init(int enable)
>       * be able to allocate memory on the stack, even on a stack overflow.
> If it
>       * fails, ignore the error. */
>      stack.ss_flags = 0;
> -    stack.ss_size = SIGSTKSZ;
> +    stack.ss_size = SIGSTKSZ * 2;
>      stack.ss_sp = PyMem_Malloc(stack.ss_size);
>      if (stack.ss_sp != NULL) {
>          err = sigaltstack(&stack, &old_stack);
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue21131>
> _______________________________________
>