[PATCH] nptl: add TLS/TCB sizes to requested stack size rather than subtract

[Carlos O'Donell]

On Fri, Mar 23, 2012 at 4:25 PM, Carlos O'Donell
<carlos@systemhalted.org> wrote:
> On Fri, Mar 23, 2012 at 3:22 PM, Mike Frysinger <vapier@gentoo.org> wrote:
>> From: Ahmad Sharif <asharif@chromium.org>
>>
>> When Chrome is built with -fprofile-generate, pthread_create() fails
>> because the TLS size is larger than the stack size requested.  This
>> is the same issue as reported in bugzilla a while ago.
>
> It fails because the TLS size is larger than the *default* machine
> stack size (minus the DTV and pthread descriptor)?
>
> ... and because the default machine stack size is not configurable,
> and computing the size of the TLS data is difficult you'd like the
> default behaviour to be that it just works (tm)?
>
> That sounds reasonable.
>
>> URL: http://sourceware.org/bugzilla/show_bug.cgi?id=11787
>> Signed-off-by: Ahmad Sharif <asharif@chromium.org>
>> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>> ---
>>  nptl/ChangeLog       |    7 +++++++
>>  nptl/allocatestack.c |    6 +++++-
>>  2 files changed, 12 insertions(+), 1 deletions(-)
>>
>> diff --git a/nptl/ChangeLog b/nptl/ChangeLog
>> index ad345f9..6817d97 100644
>> --- a/nptl/ChangeLog
>> +++ b/nptl/ChangeLog
>> @@ -1,3 +1,10 @@
>> +2012-03-23  Ahmad Sharif  <asharif@chromium.org>
>> +
>> +       [BZ #11787]
>> +       * allocatestack.c (allocate_stack): Add __static_tls_size to
>> +       size, and add TLS_TCB_SIZE to size when TLS_TCB_AT_TP is defined.
>> +       Check attr->stacksize against just MINIMAL_REST_STACK.
>> +
>>  2012-03-19  H.J. Lu  <hongjiu.lu@intel.com>
>>
>>        * sysdeps/x86_64/pthreaddef.h (CURRENT_STACK_FRAME): Use
>> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
>> index 79c4531..4b5998d 100644
>> --- a/nptl/allocatestack.c
>> +++ b/nptl/allocatestack.c
>> @@ -356,6 +356,10 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
>>   /* Get the stack size from the attribute if it is set.  Otherwise we
>>      use the default we determined at start time.  */
>>   size = attr->stacksize ?: __default_stacksize;
>> +  size += __static_tls_size;
>> +#if TLS_TCB_AT_TP
>> +  size += TLS_TCB_SIZE;
>> +#endif
>
> You must not adjust size here.
>
> Several other asserts depend on it being the original size passed in
> by the user e.g.
>  375       assert (size > adj + TLS_TCB_SIZE);
> ...
>  379       assert (size > adj);
> ...
>  401       /* Remember the stack-related values.  */
>  402       pd->stackblock = (char *) attr->stackaddr - size;
>  403       pd->stackblock_size = size;
>
> If you need to adjust it do so *only* in the default else clause where
> we handle stack allocation.
>
> You should be using __pthread_get_minstack to compute the minimum
> stack and check to see if it's larger than the default and if so use
> it.
>
> I don't want to increase the default stack size by the static TLS
> block size since doing so could potentially break programs that are at
> the limit of their memory usage. Using __pthread_get_minstack gives
> you a minimum of PTHREAD_STACK_MIN to work with, and beyond that you
> should be allocating custom stacks.
>
> You should not second guess the user though, if attr->stacksize is set
> you should use it and *not* adjust anything.
>
> You'll see that __pthread_get_minstack takes into account __static_tls_size.
>
> Please rework the patch.
>
>>   /* Get memory for the stack.  */
>>   if (__builtin_expect (attr->flags & ATTR_FLAG_STACKADDR, 0))
>> @@ -365,7 +369,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
>>       /* If the user also specified the size of the stack make sure it
>>         is large enough.  */
>>       if (attr->stacksize != 0
>> -         && attr->stacksize < (__static_tls_size + MINIMAL_REST_STACK))
>> +         && attr->stacksize < (MINIMAL_REST_STACK))
>
> Why are you changing this? It has nothing to do with your current patch.

Additional comments here: http://sourceware.org/bugzilla/show_bug.cgi?id=11787

Cheers,
Carlos.