nptl: Add tst-minstack-cancel, tst-minstack-exit [BZ #22636]

Message ID 20180110102147.C81C3439942E1@oldenburg.str.redhat.com
State New
Headers show
Series
  • nptl: Add tst-minstack-cancel, tst-minstack-exit [BZ #22636]
Related show

Commit Message

Florian Weimer Jan. 10, 2018, 10:21 a.m.
I verified that without the guard accounting change in commit
630f4cc3aa019ede55976ea561f1a7af2f068639 (Fix stack guard size
accounting) the tst-minstack-cancel test fails on an AVX-512F machine.
(tst-minstack-exit still passes.)

The two tests cannot be merged because lazy binding in libgcc occurs
only once and contributes significantly to stack usage, so the first
test would alter stack usage in the second test.

2018-01-10  Florian Weimer  <fweimer@redhat.com>

	[BZ #22636]
	* nptl/Makefile (tests): Add tst-minstack-cancel, tst-minstack-exit.
	* nptl/tst-minstack-cancel.c, nptl/tst-minstack-exit.c: New files.

Comments

Adhemerval Zanella Jan. 10, 2018, 11:53 a.m. | #1
On 10/01/2018 08:21, Florian Weimer wrote:
> I verified that without the guard accounting change in commit
> 630f4cc3aa019ede55976ea561f1a7af2f068639 (Fix stack guard size
> accounting) the tst-minstack-cancel test fails on an AVX-512F machine.
> (tst-minstack-exit still passes.)
> 
> The two tests cannot be merged because lazy binding in libgcc occurs
> only once and contributes significantly to stack usage, so the first
> test would alter stack usage in the second test.
> 
> 2018-01-10  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #22636]
> 	* nptl/Makefile (tests): Add tst-minstack-cancel, tst-minstack-exit.
> 	* nptl/tst-minstack-cancel.c, nptl/tst-minstack-exit.c: New files.

From the discussion about PTHREAD_STACK_MIN [1] I think the consensus is
PTHREAD_STACK_MIN does not include enough stack necessary for the 
cancellation mechanism. Shouldn't we mark 'tst-minstack-cancel.c' as
an XFAIL?

[1] https://sourceware.org/ml/libc-alpha/2017-12/msg00816.html

> 
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 7265c0a53c..12c69f99d8 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -307,7 +307,7 @@ tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
>  	tst-bad-schedattr \
>  	tst-thread_local1 tst-mutex-errorcheck tst-robust10 \
>  	tst-robust-fork tst-create-detached tst-memstream \
> -	tst-thread-exit-clobber
> +	tst-thread-exit-clobber tst-minstack-cancel tst-minstack-exit
>  
>  tests-internal := tst-rwlock19 tst-rwlock20 \
>  		  tst-sem11 tst-sem12 tst-sem13 \
> diff --git a/nptl/tst-minstack-cancel.c b/nptl/tst-minstack-cancel.c
> new file mode 100644
> index 0000000000..3d8d0dba70
> --- /dev/null
> +++ b/nptl/tst-minstack-cancel.c
> @@ -0,0 +1,45 @@
> +/* Test cancellation with a minimal stack size.
> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <limits.h>
> +#include <unistd.h>
> +#include <support/check.h>
> +#include <support/xthread.h>
> +
> +static void *
> +threadfunc (void *closure)
> +{
> +  while (1)
> +    pause ();
> +  return NULL;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  pthread_attr_t attr;
> +  xpthread_attr_init (&attr);
> +  xpthread_attr_setstacksize (&attr, PTHREAD_STACK_MIN);
> +  pthread_t thr = xpthread_create (&attr, threadfunc, NULL);
> +  xpthread_cancel (thr);
> +  TEST_VERIFY (xpthread_join (thr) == PTHREAD_CANCELED);
> +  xpthread_attr_destroy (&attr);
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/nptl/tst-minstack-exit.c b/nptl/tst-minstack-exit.c
> new file mode 100644
> index 0000000000..eb6808e889
> --- /dev/null
> +++ b/nptl/tst-minstack-exit.c
> @@ -0,0 +1,43 @@
> +/* Test that pthread_exit works with the minimum stack size.
> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <limits.h>
> +#include <unistd.h>
> +#include <support/check.h>
> +#include <support/xthread.h>
> +
> +static void *
> +threadfunc (void *closure)
> +{
> +  pthread_exit (threadfunc);
> +  return NULL;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  pthread_attr_t attr;
> +  xpthread_attr_init (&attr);
> +  xpthread_attr_setstacksize (&attr, PTHREAD_STACK_MIN);
> +  pthread_t thr = xpthread_create (&attr, threadfunc, NULL);
> +  TEST_VERIFY (xpthread_join (thr) == threadfunc);
> +  xpthread_attr_destroy (&attr);
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
>
Florian Weimer Jan. 10, 2018, 12:16 p.m. | #2
On 01/10/2018 12:53 PM, Adhemerval Zanella wrote:
> 
> 
> On 10/01/2018 08:21, Florian Weimer wrote:
>> I verified that without the guard accounting change in commit
>> 630f4cc3aa019ede55976ea561f1a7af2f068639 (Fix stack guard size
>> accounting) the tst-minstack-cancel test fails on an AVX-512F machine.
>> (tst-minstack-exit still passes.)
>>
>> The two tests cannot be merged because lazy binding in libgcc occurs
>> only once and contributes significantly to stack usage, so the first
>> test would alter stack usage in the second test.
>>
>> 2018-01-10  Florian Weimer  <fweimer@redhat.com>
>>
>> 	[BZ #22636]
>> 	* nptl/Makefile (tests): Add tst-minstack-cancel, tst-minstack-exit.
>> 	* nptl/tst-minstack-cancel.c, nptl/tst-minstack-exit.c: New files.
> 
>  From the discussion about PTHREAD_STACK_MIN [1] I think the consensus is
> PTHREAD_STACK_MIN does not include enough stack necessary for the
> cancellation mechanism. Shouldn't we mark 'tst-minstack-cancel.c' as
> an XFAIL?
> 
> [1] https://sourceware.org/ml/libc-alpha/2017-12/msg00816.html

I am pretty sure that PTHREAD_STACK_MIN was sufficient for cancellation 
to work on glibc 2.26 (as released) and earlier, across all 
architectures.  The test case pretty much mirrors what ntpd does during 
startup, and it's so widely used that people report problems with that 
pretty quickly.  (It's how this was reported originally.)

I think the XSAVE ld.so trampoline simply introduced a regression, and 
we are putting in changes to fix it (4 KiB of stack added, libgcc_s.so 
with RTLD_NOW).  The new tests try to make sure that it won't happen again.

Thanks,
Florian
Szabolcs Nagy Jan. 10, 2018, 12:18 p.m. | #3
On 10/01/18 11:53, Adhemerval Zanella wrote:
> On 10/01/2018 08:21, Florian Weimer wrote:
>> I verified that without the guard accounting change in commit
>> 630f4cc3aa019ede55976ea561f1a7af2f068639 (Fix stack guard size
>> accounting) the tst-minstack-cancel test fails on an AVX-512F machine.
>> (tst-minstack-exit still passes.)
>>
>> The two tests cannot be merged because lazy binding in libgcc occurs
>> only once and contributes significantly to stack usage, so the first
>> test would alter stack usage in the second test.
>>
>> 2018-01-10  Florian Weimer  <fweimer@redhat.com>
>>
>> 	[BZ #22636]
>> 	* nptl/Makefile (tests): Add tst-minstack-cancel, tst-minstack-exit.
>> 	* nptl/tst-minstack-cancel.c, nptl/tst-minstack-exit.c: New files.
> 
> From the discussion about PTHREAD_STACK_MIN [1] I think the consensus is
> PTHREAD_STACK_MIN does not include enough stack necessary for the 
> cancellation mechanism. Shouldn't we mark 'tst-minstack-cancel.c' as
> an XFAIL?
> 
> [1] https://sourceware.org/ml/libc-alpha/2017-12/msg00816.html
> 

existing code relies on it:
https://sourceware.org/ml/libc-help/2017-02/msg00000.html

tl;dr: glibc uses dlopen for cancellation which can fail, so
the only way for ntpd to avoid crashing at runtime or leaking
blocked threads is to a cancel a thread at program startup time
(which does the dlopen early) and it uses a minimal stack to
avoid wasting resources for this nasty workaround.
Adhemerval Zanella Jan. 10, 2018, 12:30 p.m. | #4
> I am pretty sure that PTHREAD_STACK_MIN was sufficient for cancellation to work on glibc 2.26 (as released) and earlier, across all architectures.  The test case pretty much mirrors what ntpd does during startup, and it's so widely used that people report problems with that pretty quickly.  (It's how this was reported originally.)
> 
> I think the XSAVE ld.so trampoline simply introduced a regression, and we are putting in changes to fix it (4 KiB of stack added, libgcc_s.so with RTLD_NOW).  The new tests try to make sure that it won't happen again.


> existing code relies on it:
> https://sourceware.org/ml/libc-help/2017-02/msg00000.html
> 
> tl;dr: glibc uses dlopen for cancellation which can fail, so
> the only way for ntpd to avoid crashing at runtime or leaking
> blocked threads is to a cancel a thread at program startup time
> (which does the dlopen early) and it uses a minimal stack to
> avoid wasting resources for this nasty workaround.

So in this case I think the summary Carlos wrote about "What can 
a thread do with PTHREAD_STACK_MIN?" should be changed to also
include cancellation (at least this specific scenario).
Florian Weimer Jan. 10, 2018, 12:47 p.m. | #5
On 01/10/2018 01:30 PM, Adhemerval Zanella wrote:
> 
> 
>> I am pretty sure that PTHREAD_STACK_MIN was sufficient for cancellation to work on glibc 2.26 (as released) and earlier, across all architectures.  The test case pretty much mirrors what ntpd does during startup, and it's so widely used that people report problems with that pretty quickly.  (It's how this was reported originally.)
>>
>> I think the XSAVE ld.so trampoline simply introduced a regression, and we are putting in changes to fix it (4 KiB of stack added, libgcc_s.so with RTLD_NOW).  The new tests try to make sure that it won't happen again.
> 
> 
>> existing code relies on it:
>> https://sourceware.org/ml/libc-help/2017-02/msg00000.html
>>
>> tl;dr: glibc uses dlopen for cancellation which can fail, so
>> the only way for ntpd to avoid crashing at runtime or leaking
>> blocked threads is to a cancel a thread at program startup time
>> (which does the dlopen early) and it uses a minimal stack to
>> avoid wasting resources for this nasty workaround.
> 
> So in this case I think the summary Carlos wrote about "What can
> a thread do with PTHREAD_STACK_MIN?" should be changed to also
> include cancellation (at least this specific scenario).

Has this text been incorporated anywhere?  It wasn't proposed as a diff, 
so I didn't comment on it further.

I firmly believe we shouldn't try to fix regressions by documentation, 
blaming application authors, especially if there is an alternative which 
is technically feasible and not too hard to implement and maintain going 
forward.

Thanks,
Florian
Szabolcs Nagy Jan. 10, 2018, 12:49 p.m. | #6
On 10/01/18 12:30, Adhemerval Zanella wrote:
> 
> 
>> I am pretty sure that PTHREAD_STACK_MIN was sufficient for cancellation to work on glibc 2.26 (as released) and earlier, across all architectures.  The test case pretty much mirrors what ntpd does during startup, and it's so widely used that people report problems with that pretty quickly.  (It's how this was reported originally.)
>>
>> I think the XSAVE ld.so trampoline simply introduced a regression, and we are putting in changes to fix it (4 KiB of stack added, libgcc_s.so with RTLD_NOW).  The new tests try to make sure that it won't happen again.
> 
> 
>> existing code relies on it:
>> https://sourceware.org/ml/libc-help/2017-02/msg00000.html
>>
>> tl;dr: glibc uses dlopen for cancellation which can fail, so
>> the only way for ntpd to avoid crashing at runtime or leaking
>> blocked threads is to a cancel a thread at program startup time
>> (which does the dlopen early) and it uses a minimal stack to
>> avoid wasting resources for this nasty workaround.
> 
> So in this case I think the summary Carlos wrote about "What can 
> a thread do with PTHREAD_STACK_MIN?" should be changed to also
> include cancellation (at least this specific scenario).
> 

hm on a second thought it is probably not reasonable to
guarantee cancellation to work (and signal frame to be
included) on PTHREAD_STACK_MIN, it prevents creation of
tiny thread stacks which may be useful and signal frames
can grow big in the future (e.g. sve on aarch64, although
aarch64 has huge PTHREAD_STACK_MIN so that's fortunately
not an issue right now).

it might make sense to silently keep old binaries working,
but not give documented guarantee (deprecate this use), i
don't have a strong opinion which way this goes.
Adhemerval Zanella Jan. 10, 2018, 3:24 p.m. | #7
On 10/01/2018 10:47, Florian Weimer wrote:
> On 01/10/2018 01:30 PM, Adhemerval Zanella wrote:
>>
>>
>>> I am pretty sure that PTHREAD_STACK_MIN was sufficient for cancellation to work on glibc 2.26 (as released) and earlier, across all architectures.  The test case pretty much mirrors what ntpd does during startup, and it's so widely used that people report problems with that pretty quickly.  (It's how this was reported originally.)
>>>
>>> I think the XSAVE ld.so trampoline simply introduced a regression, and we are putting in changes to fix it (4 KiB of stack added, libgcc_s.so with RTLD_NOW).  The new tests try to make sure that it won't happen again.
>>
>>
>>> existing code relies on it:
>>> https://sourceware.org/ml/libc-help/2017-02/msg00000.html
>>>
>>> tl;dr: glibc uses dlopen for cancellation which can fail, so
>>> the only way for ntpd to avoid crashing at runtime or leaking
>>> blocked threads is to a cancel a thread at program startup time
>>> (which does the dlopen early) and it uses a minimal stack to
>>> avoid wasting resources for this nasty workaround.
>>
>> So in this case I think the summary Carlos wrote about "What can
>> a thread do with PTHREAD_STACK_MIN?" should be changed to also
>> include cancellation (at least this specific scenario).
> 
> Has this text been incorporated anywhere?  It wasn't proposed as a diff, so I didn't comment on it further.
> 
> I firmly believe we shouldn't try to fix regressions by documentation, blaming application authors, especially if there is an alternative which is technically feasible and not too hard to implement and maintain going forward.
> 
> Thanks,
> Florian

I do not think it has been incorporated since Carlos did not gave any update
about it.  However my understanding was that the thread intended exactly to
discuss how to handle PTHREAD_STACK_MIN regarding the various pthread ABI
guarantees and your patch goes contrary to the cancellation discussion back
then.  I just noted that if Carlos documentation ended up being incorporated
on manual (if it is indeed the idea) I think we need to discuss it further. 

> hm on a second thought it is probably not reasonable to
> guarantee cancellation to work (and signal frame to be
> included) on PTHREAD_STACK_MIN, it prevents creation of
> tiny thread stacks which may be useful and signal frames
> can grow big in the future (e.g. sve on aarch64, although
> aarch64 has huge PTHREAD_STACK_MIN so that's fortunately
> not an issue right now).
> 
> it might make sense to silently keep old binaries working,
> but not give documented guarantee (deprecate this use), i
> don't have a strong opinion which way this goes.

That's a good point to continue to not guarantee cancellation to work 
with PTHREAD_STACK_MIN regardless. The same scenario might occur in other
architectures as well and we will face the same issue if/when it happens
(binaries that rely on the very trick ntpd uses).
Carlos O'Donell Jan. 10, 2018, 5:42 p.m. | #8
On 01/10/2018 04:47 AM, Florian Weimer wrote:
> I firmly believe we shouldn't try to fix regressions by
> documentation, blaming application authors, especially if there is an
> alternative which is technically feasible and not too hard to
> implement and maintain going forward.

I agree completely.

Though I also agree with Szabolcs.

We want to maximize backwards compatibility where possible, in this case
getting 4KiB back from the stack guard accounting is enough to allow your
test to pass. This is good and will help ntpd binaries to continue to run.

We also need to be clear in our documentation on what can and can't be 
done with PTHREAD_STACK_MIN. I *do* want to add text into the manual that
explains what can and can't be done with the minimal stack.
Carlos O'Donell Jan. 10, 2018, 5:58 p.m. | #9
On 01/10/2018 02:21 AM, Florian Weimer wrote:
> I verified that without the guard accounting change in commit
> 630f4cc3aa019ede55976ea561f1a7af2f068639 (Fix stack guard size
> accounting) the tst-minstack-cancel test fails on an AVX-512F machine.
> (tst-minstack-exit still passes.)
> 
> The two tests cannot be merged because lazy binding in libgcc occurs
> only once and contributes significantly to stack usage, so the first
> test would alter stack usage in the second test.
> 

* BIND_NOW vs lazy binding?

- Do the two tests need to be built explicitly with BIND_NOW to avoid
  lazy binding stack usage on systems that do not have compilers that
  default to building binaries with BIND_NOW?

- Should both tests be built and run with and without lazy binding?

* Comment about merging the tests.

- Please add a comment to both tests that explains that the tests
  should not be merged together with any other test, and that this is
  done to avoid impacting the stack of whichever test is run second.

OK if you take both issues into consideration.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> 2018-01-10  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #22636]
> 	* nptl/Makefile (tests): Add tst-minstack-cancel, tst-minstack-exit.
> 	* nptl/tst-minstack-cancel.c, nptl/tst-minstack-exit.c: New files.
> 
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 7265c0a53c..12c69f99d8 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -307,7 +307,7 @@ tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
>  	tst-bad-schedattr \
>  	tst-thread_local1 tst-mutex-errorcheck tst-robust10 \
>  	tst-robust-fork tst-create-detached tst-memstream \
> -	tst-thread-exit-clobber
> +	tst-thread-exit-clobber tst-minstack-cancel tst-minstack-exit

OK.

>  
>  tests-internal := tst-rwlock19 tst-rwlock20 \
>  		  tst-sem11 tst-sem12 tst-sem13 \
> diff --git a/nptl/tst-minstack-cancel.c b/nptl/tst-minstack-cancel.c
> new file mode 100644
> index 0000000000..3d8d0dba70
> --- /dev/null
> +++ b/nptl/tst-minstack-cancel.c
> @@ -0,0 +1,45 @@
> +/* Test cancellation with a minimal stack size.
> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <limits.h>
> +#include <unistd.h>
> +#include <support/check.h>
> +#include <support/xthread.h>
> +
> +static void *
> +threadfunc (void *closure)
> +{
> +  while (1)
> +    pause ();

OK.

> +  return NULL;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  pthread_attr_t attr;
> +  xpthread_attr_init (&attr);
> +  xpthread_attr_setstacksize (&attr, PTHREAD_STACK_MIN);

OK.

> +  pthread_t thr = xpthread_create (&attr, threadfunc, NULL);
> +  xpthread_cancel (thr);
> +  TEST_VERIFY (xpthread_join (thr) == PTHREAD_CANCELED);

OK.

> +  xpthread_attr_destroy (&attr);
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/nptl/tst-minstack-exit.c b/nptl/tst-minstack-exit.c
> new file mode 100644
> index 0000000000..eb6808e889
> --- /dev/null
> +++ b/nptl/tst-minstack-exit.c
> @@ -0,0 +1,43 @@
> +/* Test that pthread_exit works with the minimum stack size.
> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <limits.h>
> +#include <unistd.h>
> +#include <support/check.h>
> +#include <support/xthread.h>
> +
> +static void *
> +threadfunc (void *closure)
> +{
> +  pthread_exit (threadfunc);

OK, run through thread exit with minimal stack.

> +  return NULL;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  pthread_attr_t attr;
> +  xpthread_attr_init (&attr);
> +  xpthread_attr_setstacksize (&attr, PTHREAD_STACK_MIN);
> +  pthread_t thr = xpthread_create (&attr, threadfunc, NULL);
> +  TEST_VERIFY (xpthread_join (thr) == threadfunc);

OK.

> +  xpthread_attr_destroy (&attr);
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
>
Florian Weimer Jan. 10, 2018, 7:41 p.m. | #10
On 01/10/2018 06:58 PM, Carlos O'Donell wrote:
> On 01/10/2018 02:21 AM, Florian Weimer wrote:
>> I verified that without the guard accounting change in commit
>> 630f4cc3aa019ede55976ea561f1a7af2f068639 (Fix stack guard size
>> accounting) the tst-minstack-cancel test fails on an AVX-512F machine.
>> (tst-minstack-exit still passes.)
>>
>> The two tests cannot be merged because lazy binding in libgcc occurs
>> only once and contributes significantly to stack usage, so the first
>> test would alter stack usage in the second test.
>>
> 
> * BIND_NOW vs lazy binding?
> 
> - Do the two tests need to be built explicitly with BIND_NOW to avoid
>    lazy binding stack usage on systems that do not have compilers that
>    default to building binaries with BIND_NOW?
> 
> - Should both tests be built and run with and without lazy binding?

We now open libgcc_s.so with RTLD_NOW, so this is a bit outdated.

Even before that, we did not actually have control over the binding 
style used by libgcc_s because it could have been linked with -z now 
(and which I plan to implement for Fedora, but Jakub wants internal PLT 
avoidance for libgcc_s first).

Thanks,
Florian
Subject: [PATCH] nptl: Add tst-minstack-cancel, tst-minstack-exit [BZ #22636]
To: libc-alpha@sourceware.org

I verified that without the guard accounting change in commit
630f4cc3aa019ede55976ea561f1a7af2f068639 (Fix stack guard size
accounting) and RTLD_NOW for libgcc_s introduced by commit
f993b8754080ac7572b692870e926d8b493db16c (nptl: Open libgcc.so with
RTLD_NOW during pthread_cancel), the tst-minstack-cancel test fails on
an AVX-512F machine.  tst-minstack-exit still passes, and either of
the mentioned commit by itself frees sufficient stack space to make
tst-minstack-cancel pass, too.

2018-01-10  Florian Weimer  <fweimer@redhat.com>

	[BZ #22636]
	* nptl/Makefile (tests): Add tst-minstack-cancel, tst-minstack-exit.
	* nptl/tst-minstack-cancel.c, nptl/tst-minstack-exit.c: New files.

diff --git a/nptl/Makefile b/nptl/Makefile
index 7265c0a53c..12c69f99d8 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -307,7 +307,7 @@ tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
 	tst-bad-schedattr \
 	tst-thread_local1 tst-mutex-errorcheck tst-robust10 \
 	tst-robust-fork tst-create-detached tst-memstream \
-	tst-thread-exit-clobber
+	tst-thread-exit-clobber tst-minstack-cancel tst-minstack-exit
 
 tests-internal := tst-rwlock19 tst-rwlock20 \
 		  tst-sem11 tst-sem12 tst-sem13 \
diff --git a/nptl/tst-minstack-cancel.c b/nptl/tst-minstack-cancel.c
new file mode 100644
index 0000000000..a306320e88
--- /dev/null
+++ b/nptl/tst-minstack-cancel.c
@@ -0,0 +1,48 @@
+/* Test cancellation with a minimal stack size.
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* Note: This test is similar to tst-minstack-exit, but is separate to
+   avoid spurious test passes due to warm-up effects.  */
+
+#include <limits.h>
+#include <unistd.h>
+#include <support/check.h>
+#include <support/xthread.h>
+
+static void *
+threadfunc (void *closure)
+{
+  while (1)
+    pause ();
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  pthread_attr_t attr;
+  xpthread_attr_init (&attr);
+  xpthread_attr_setstacksize (&attr, PTHREAD_STACK_MIN);
+  pthread_t thr = xpthread_create (&attr, threadfunc, NULL);
+  xpthread_cancel (thr);
+  TEST_VERIFY (xpthread_join (thr) == PTHREAD_CANCELED);
+  xpthread_attr_destroy (&attr);
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/nptl/tst-minstack-exit.c b/nptl/tst-minstack-exit.c
new file mode 100644
index 0000000000..9c7e9a4dfe
--- /dev/null
+++ b/nptl/tst-minstack-exit.c
@@ -0,0 +1,46 @@
+/* Test that pthread_exit works with the minimum stack size.
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* Note: This test is similar to tst-minstack-cancel, but is separate
+   to avoid spurious test passes due to warm-up effects.  */
+
+#include <limits.h>
+#include <unistd.h>
+#include <support/check.h>
+#include <support/xthread.h>
+
+static void *
+threadfunc (void *closure)
+{
+  pthread_exit (threadfunc);
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  pthread_attr_t attr;
+  xpthread_attr_init (&attr);
+  xpthread_attr_setstacksize (&attr, PTHREAD_STACK_MIN);
+  pthread_t thr = xpthread_create (&attr, threadfunc, NULL);
+  TEST_VERIFY (xpthread_join (thr) == threadfunc);
+  xpthread_attr_destroy (&attr);
+  return 0;
+}
+
+#include <support/test-driver.c>
Carlos O'Donell Jan. 10, 2018, 8:31 p.m. | #11
On 01/10/2018 11:41 AM, Florian Weimer wrote:
> On 01/10/2018 06:58 PM, Carlos O'Donell wrote:
>> On 01/10/2018 02:21 AM, Florian Weimer wrote:
>>> I verified that without the guard accounting change in commit
>>> 630f4cc3aa019ede55976ea561f1a7af2f068639 (Fix stack guard size
>>> accounting) the tst-minstack-cancel test fails on an AVX-512F machine.
>>> (tst-minstack-exit still passes.)
>>>
>>> The two tests cannot be merged because lazy binding in libgcc occurs
>>> only once and contributes significantly to stack usage, so the first
>>> test would alter stack usage in the second test.
>>>
>>
>> * BIND_NOW vs lazy binding?
>>
>> - Do the two tests need to be built explicitly with BIND_NOW to avoid
>>    lazy binding stack usage on systems that do not have compilers that
>>    default to building binaries with BIND_NOW?
>>
>> - Should both tests be built and run with and without lazy binding?
> 
> We now open libgcc_s.so with RTLD_NOW, so this is a bit outdated.

Agreed. Though note that this increases the stack requirement for
*calling* pthread_cancel from another thread ;-)
 
> Even before that, we did not actually have control over the binding
> style used by libgcc_s because it could have been linked with -z now
> (and which I plan to implement for Fedora, but Jakub wants internal
> PLT avoidance for libgcc_s first).

Correct. We can't force the worst case scenario, but then again our
purpose in testing is to evaluate the target environment is going to
work (native in this case).

> Subject: [PATCH] nptl: Add tst-minstack-cancel, tst-minstack-exit [BZ #22636]
> To: libc-alpha@sourceware.org

OK, with just one more comment:

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> I verified that without the guard accounting change in commit
> 630f4cc3aa019ede55976ea561f1a7af2f068639 (Fix stack guard size
> accounting) and RTLD_NOW for libgcc_s introduced by commit
> f993b8754080ac7572b692870e926d8b493db16c (nptl: Open libgcc.so with
> RTLD_NOW during pthread_cancel), the tst-minstack-cancel test fails on
> an AVX-512F machine.  tst-minstack-exit still passes, and either of
> the mentioned commit by itself frees sufficient stack space to make
> tst-minstack-cancel pass, too.
> 
> 2018-01-10  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #22636]
> 	* nptl/Makefile (tests): Add tst-minstack-cancel, tst-minstack-exit.
> 	* nptl/tst-minstack-cancel.c, nptl/tst-minstack-exit.c: New files.
> 
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 7265c0a53c..12c69f99d8 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -307,7 +307,7 @@ tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
>  	tst-bad-schedattr \
>  	tst-thread_local1 tst-mutex-errorcheck tst-robust10 \
>  	tst-robust-fork tst-create-detached tst-memstream \
> -	tst-thread-exit-clobber
> +	tst-thread-exit-clobber tst-minstack-cancel tst-minstack-exit
>  
>  tests-internal := tst-rwlock19 tst-rwlock20 \
>  		  tst-sem11 tst-sem12 tst-sem13 \
> diff --git a/nptl/tst-minstack-cancel.c b/nptl/tst-minstack-cancel.c
> new file mode 100644
> index 0000000000..a306320e88
> --- /dev/null
> +++ b/nptl/tst-minstack-cancel.c
> @@ -0,0 +1,48 @@
> +/* Test cancellation with a minimal stack size.
> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* Note: This test is similar to tst-minstack-exit, but is separate to
> +   avoid spurious test passes due to warm-up effects.  */
> +
> +#include <limits.h>
> +#include <unistd.h>
> +#include <support/check.h>
> +#include <support/xthread.h>
> +
> +static void *
> +threadfunc (void *closure)
> +{
> +  while (1)
> +    pause ();
> +  return NULL;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  pthread_attr_t attr;
> +  xpthread_attr_init (&attr);

Should we mention that the signal stack should not be changed too?

/* We do not use an alternate sized signal stack because we want
   to also test that the SIGCANCEL handler can run in the 
   minimum stack.  */

> +  xpthread_attr_setstacksize (&attr, PTHREAD_STACK_MIN);
> +  pthread_t thr = xpthread_create (&attr, threadfunc, NULL);
> +  xpthread_cancel (thr);
> +  TEST_VERIFY (xpthread_join (thr) == PTHREAD_CANCELED);
> +  xpthread_attr_destroy (&attr);
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/nptl/tst-minstack-exit.c b/nptl/tst-minstack-exit.c
> new file mode 100644
> index 0000000000..9c7e9a4dfe
> --- /dev/null
> +++ b/nptl/tst-minstack-exit.c
> @@ -0,0 +1,46 @@
> +/* Test that pthread_exit works with the minimum stack size.
> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* Note: This test is similar to tst-minstack-cancel, but is separate
> +   to avoid spurious test passes due to warm-up effects.  */
> +
> +#include <limits.h>
> +#include <unistd.h>
> +#include <support/check.h>
> +#include <support/xthread.h>
> +
> +static void *
> +threadfunc (void *closure)
> +{
> +  pthread_exit (threadfunc);
> +  return NULL;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  pthread_attr_t attr;
> +  xpthread_attr_init (&attr);
> +  xpthread_attr_setstacksize (&attr, PTHREAD_STACK_MIN);
> +  pthread_t thr = xpthread_create (&attr, threadfunc, NULL);
> +  TEST_VERIFY (xpthread_join (thr) == threadfunc);
> +  xpthread_attr_destroy (&attr);
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
Florian Weimer Jan. 10, 2018, 8:35 p.m. | #12
On 01/10/2018 09:31 PM, Carlos O'Donell wrote:
> Should we mention that the signal stack should not be changed too?
> 
> /* We do not use an alternate sized signal stack because we want
>     to also test that the SIGCANCEL handler can run in the
>     minimum stack.  */

Isn't this obvious?  I mean, sigaltstack is *really* specialized.

Thanks,
Florian
Carlos O'Donell Jan. 10, 2018, 10:29 p.m. | #13
On 01/10/2018 12:35 PM, Florian Weimer wrote:
> On 01/10/2018 09:31 PM, Carlos O'Donell wrote:
>> Should we mention that the signal stack should not be changed too?
>>
>> /* We do not use an alternate sized signal stack because we want
>>     to also test that the SIGCANCEL handler can run in the
>>     minimum stack.  */
> 
> Isn't this obvious?  I mean, sigaltstack is *really* specialized.

It's a hard question to answer and it depends on the average competence
of people working on our test cases. It's exactly the reason why I asked
for your input :-)

It sounds like you don't think we need it, in which case I'm happy without
it also.

Patch

diff --git a/nptl/Makefile b/nptl/Makefile
index 7265c0a53c..12c69f99d8 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -307,7 +307,7 @@  tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
 	tst-bad-schedattr \
 	tst-thread_local1 tst-mutex-errorcheck tst-robust10 \
 	tst-robust-fork tst-create-detached tst-memstream \
-	tst-thread-exit-clobber
+	tst-thread-exit-clobber tst-minstack-cancel tst-minstack-exit
 
 tests-internal := tst-rwlock19 tst-rwlock20 \
 		  tst-sem11 tst-sem12 tst-sem13 \
diff --git a/nptl/tst-minstack-cancel.c b/nptl/tst-minstack-cancel.c
new file mode 100644
index 0000000000..3d8d0dba70
--- /dev/null
+++ b/nptl/tst-minstack-cancel.c
@@ -0,0 +1,45 @@ 
+/* Test cancellation with a minimal stack size.
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <limits.h>
+#include <unistd.h>
+#include <support/check.h>
+#include <support/xthread.h>
+
+static void *
+threadfunc (void *closure)
+{
+  while (1)
+    pause ();
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  pthread_attr_t attr;
+  xpthread_attr_init (&attr);
+  xpthread_attr_setstacksize (&attr, PTHREAD_STACK_MIN);
+  pthread_t thr = xpthread_create (&attr, threadfunc, NULL);
+  xpthread_cancel (thr);
+  TEST_VERIFY (xpthread_join (thr) == PTHREAD_CANCELED);
+  xpthread_attr_destroy (&attr);
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/nptl/tst-minstack-exit.c b/nptl/tst-minstack-exit.c
new file mode 100644
index 0000000000..eb6808e889
--- /dev/null
+++ b/nptl/tst-minstack-exit.c
@@ -0,0 +1,43 @@ 
+/* Test that pthread_exit works with the minimum stack size.
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <limits.h>
+#include <unistd.h>
+#include <support/check.h>
+#include <support/xthread.h>
+
+static void *
+threadfunc (void *closure)
+{
+  pthread_exit (threadfunc);
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  pthread_attr_t attr;
+  xpthread_attr_init (&attr);
+  xpthread_attr_setstacksize (&attr, PTHREAD_STACK_MIN);
+  pthread_t thr = xpthread_create (&attr, threadfunc, NULL);
+  TEST_VERIFY (xpthread_join (thr) == threadfunc);
+  xpthread_attr_destroy (&attr);
+  return 0;
+}
+
+#include <support/test-driver.c>