| Message ID | 20180110102147.C81C3439942E1@oldenburg.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | nptl: Add tst-minstack-cancel, tst-minstack-exit [BZ #22636] | expand |
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> >
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
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.
> 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).
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
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.
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).
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.
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> >
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>
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.
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>