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

  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
  

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>
  

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
  

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.