Add tests for atexit/on_exit firing order

  On Mon, Jul 24, 2017 at 8:44 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> On Mon, Jul 10, 2017 at 8:56 AM, Joseph Myers <joseph@codesourcery.com> wrote:
>> On Mon, 10 Jul 2017, Szabolcs Nagy wrote:
>>
>>> you could call __cxa_atexit and __cxa_at_quick_exit
>>
>> Well, those should properly have such tests as well.
>
>
> Revised patch attached. Thanks.

I forgot to test the case where fn_final doesn't fire at all.
Revised (only last line of do_test changed).


2017-07-24  Paul Pluzhnikov  <ppluzhnikov@google.com>

        * stdlib/Makefile (tst-atexit, tst-at_quick_exit): New tests
        (tst-cxa_atexit, tst-on_exit): Likewise
        * stdlib/tst-atexit-common.c: New.
        * stdlib/tst-atexit.c: New.
        * stdlib/tst-at_quick_exit.c: New.
        * stdlib/tst-cxa_atexit.c: New.
  

On Mon, Jul 24, 2017 at 8:50 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> 2017-07-24  Paul Pluzhnikov  <ppluzhnikov@google.com>
>
>         * stdlib/Makefile (tst-atexit, tst-at_quick_exit): New tests
>         (tst-cxa_atexit, tst-on_exit): Likewise
>         * stdlib/tst-atexit-common.c: New.
>         * stdlib/tst-atexit.c: New.
>         * stdlib/tst-at_quick_exit.c: New.
>         * stdlib/tst-cxa_atexit.c: New.


Ping?
  

On Mon, Jul 31, 2017 at 10:00 AM, Paul Pluzhnikov
<ppluzhnikov@google.com> wrote:
> On Mon, Jul 24, 2017 at 8:50 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>
>> 2017-07-24  Paul Pluzhnikov  <ppluzhnikov@google.com>
>>
>>         * stdlib/Makefile (tst-atexit, tst-at_quick_exit): New tests
>>         (tst-cxa_atexit, tst-on_exit): Likewise
>>         * stdlib/tst-atexit-common.c: New.
>>         * stdlib/tst-atexit.c: New.
>>         * stdlib/tst-at_quick_exit.c: New.
>>         * stdlib/tst-cxa_atexit.c: New.
>
> Ping?

Ping x2?
  

On 07/24/2017 11:50 AM, Paul Pluzhnikov wrote:
> On Mon, Jul 24, 2017 at 8:44 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>> On Mon, Jul 10, 2017 at 8:56 AM, Joseph Myers <joseph@codesourcery.com> wrote:
>>> On Mon, 10 Jul 2017, Szabolcs Nagy wrote:
>>>
>>>> you could call __cxa_atexit and __cxa_at_quick_exit
>>> Well, those should properly have such tests as well.
>>
>> Revised patch attached. Thanks.
> I forgot to test the case where fn_final doesn't fire at all.
> Revised (only last line of do_test changed).

Thank you very much for adding new test. They are an important part of
accelerating glibc development. Overall the structure of the tests looks
good, but you need a few more comments.

OK with the changes outlined below. I make some additional testing suggestions
but these tests are better than nothing, so I don't hold you to anything but
documenting the possible options.
 
> 2017-07-24  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>         * stdlib/Makefile (tst-atexit, tst-at_quick_exit): New tests
>         (tst-cxa_atexit, tst-on_exit): Likewise

	* stdlib/Makefile (tests): Add tst-atexit, tst-at_quick_exit,
	tst-cxa_atexit, and tst-on_exit.

>         * stdlib/tst-atexit-common.c: New.
>         * stdlib/tst-atexit.c: New.
>         * stdlib/tst-at_quick_exit.c: New.
>         * stdlib/tst-cxa_atexit.c: New.

Say "New file."

> 
> -- Paul Pluzhnikov
> 
> 
> glibc-atexit-20170724a.txt
> 
> 
> diff --git a/stdlib/Makefile b/stdlib/Makefile
> index 0314d5926b..94cbd0e479 100644
> --- a/stdlib/Makefile
> +++ b/stdlib/Makefile
> @@ -80,7 +80,9 @@ tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
>  		   tst-strtol-locale tst-strtod-nan-locale tst-strfmon_l    \
>  		   tst-quick_exit tst-thread-quick_exit tst-width	    \
>  		   tst-width-stdint tst-strfrom tst-strfrom-locale	    \
> -		   tst-getrandom
> +		   tst-getrandom tst-atexit tst-at_quick_exit 		    \
> +		   tst-cxa_atexit tst-on_exit

OK. Add 4 new tests.

> +
>  tests-internal	:= tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
>  		   tst-tls-atexit tst-tls-atexit-nodelete
>  tests-static	:= tst-secure-getenv
> diff --git a/stdlib/tst-at_quick_exit.c b/stdlib/tst-at_quick_exit.c
> new file mode 100644
> index 0000000000..c86bbe3455
> --- /dev/null
> +++ b/stdlib/tst-at_quick_exit.c
> @@ -0,0 +1,25 @@
> +/* Test that functions registered via at_auick_exit are called in correct
> +   (LIFO) order.  In particular, exit handlers can themselves register
> +   additional handlers, so we test that.
> +

This needs to be a one line summary, the rest can move after the top-level
copyright block.

No empty line after the first.

> +   Copyright (C) 2017 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/>.  */
> +
> +#define ATEXIT(fn) at_quick_exit (fn)
> +#define EXIT(x) quick_exit (x)
> +
> +#include <stdlib/tst-atexit-common.c>
> diff --git a/stdlib/tst-atexit-common.c b/stdlib/tst-atexit-common.c
> new file mode 100644
> index 0000000000..428dba1742
> --- /dev/null
> +++ b/stdlib/tst-atexit-common.c
> @@ -0,0 +1,86 @@
> +/* Helper file for tst-{atexit,at_quick_exit,cxa_atexit,on_exit}.
> +

Remove empty line.

> +   Copyright (C) 2017 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 <assert.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#define MAX_ATEXIT 20

Why 20? Provide a comment stating this is arbitrarily chosen or chosen
for a specific purpose.

POSIX says we need to support at least ATEXIT_MAX? Should this be
dynamically computed via sysconf()?

It's OK to keep your code as it is, but you should list the various
considerations for POSIX and sysconf in the comment.

Likewise we don't test that a fork'd child inherits the registrations
of the parent, and we could. So that should also be mentioned briefly.

> +static char crumbs[MAX_ATEXIT];
> +static int next_slot = 0;
> +
> +static void
> +fn0 (void)
> +{
> +  crumbs[next_slot++] = '0';
> +}
> +
> +static void
> +fn1 (void)
> +{
> +  crumbs[next_slot++] = '1';
> +}
> +
> +static void
> +fn2 (void)
> +{
> +  crumbs[next_slot++] = '2';
> +  ATEXIT (fn1);
> +}
> +
> +static void
> +fn3 (void)
> +{
> +  crumbs[next_slot++] = '3';
> +  ATEXIT (fn2);
> +  ATEXIT (fn0);
> +}
> +
> +static void
> +fn_final (void)
> +{
> +  const char expected[] = "3021121130211";

Needs a comment explaining if this order is arbitrarily chosen
or specifically chosen to trigger the bug. For example, can this
be changed in the future?

> +  if (strcmp (crumbs, expected) == 0)
> +    _exit (0);
> +
> +  printf ("crumbs:   %s\n", crumbs);
> +  printf ("expected: %s\n", expected);
> +  _exit (1);
> +}
> +

Add a comment describing the test in more detail please.

> +static int
> +do_test (void)
> +{
> +  /* Register this first so it can verify expected order of the rest.  */
> +  ATEXIT (fn_final);
> +
> +  ATEXIT (fn1);
> +  ATEXIT (fn3);
> +  ATEXIT (fn1);
> +  ATEXIT (fn2);
> +  ATEXIT (fn1);
> +  ATEXIT (fn3);
> +
> +  EXIT (2);  /* If we see this exit code, fn_final must have not worked.  */
> +}
> +
> +#define TEST_FUNCTION do_test
> +#include <support/test-driver.c>

OK.

> diff --git a/stdlib/tst-atexit.c b/stdlib/tst-atexit.c
> new file mode 100644
> index 0000000000..8ca401bcbf
> --- /dev/null
> +++ b/stdlib/tst-atexit.c
> @@ -0,0 +1,25 @@
> +/* Test that functions registered via atexit are called in correct
> +   (LIFO) order.  In particular, exit handlers can themselves register
> +   additional handlers, so we test that.
> +

Same comments as above.

> +   Copyright (C) 2017 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/>.  */
> +
> +#define ATEXIT(fn) atexit (fn)
> +#define EXIT(x) exit (x)
> +
> +#include <stdlib/tst-atexit-common.c>

OK.

> diff --git a/stdlib/tst-cxa_atexit.c b/stdlib/tst-cxa_atexit.c
> new file mode 100644
> index 0000000000..46fa04a1f7
> --- /dev/null
> +++ b/stdlib/tst-cxa_atexit.c
> @@ -0,0 +1,27 @@
> +/* Test that functions registered via __cxa_atexit are called in correct
> +   (LIFO) order.  In particular, exit handlers can themselves register
> +   additional handlers, so we test that.
> +

Same comments as above.

> +   Copyright (C) 2017 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/>.  */
> +
> +extern int __cxa_atexit (void (*func) (void *), void *arg, void *d);
> +
> +#define ATEXIT(fn) __cxa_atexit ((void (*) (void *)) fn, (void *) 0, (void *) 0)
> +#define EXIT(x) exit (x)
> +
> +#include <stdlib/tst-atexit-common.c>

OK.

> diff --git a/stdlib/tst-on_exit.c b/stdlib/tst-on_exit.c
> new file mode 100644
> index 0000000000..8dda872154
> --- /dev/null
> +++ b/stdlib/tst-on_exit.c
> @@ -0,0 +1,25 @@
> +/* Test that functions registered via on_exit are called in correct
> +   (LIFO) order.  In particular, exit handlers can themselves register
> +   additional handlers, so we test that.
> +

Same comments as above.

> +   Copyright (C) 2017 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/>.  */
> +
> +#define ATEXIT(fn) on_exit ((void (*) (int, void *)) fn, (void *) 0)
> +#define EXIT(x) exit (x)
> +
> +#include <stdlib/tst-atexit-common.c>

OK.
  

On Wed, Aug 16, 2017 at 9:36 AM, Carlos O'Donell <carlos@redhat.com> wrote:

> OK with the changes outlined below.

Thanks. Committed with these changes as
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=5f3b183d198b39ca993a41aadb02bddd9fde078d

> I make some additional testing suggestions
> but these tests are better than nothing, so I don't hold you to anything but
> documenting the possible options.

I left this for a follow-up commit.