[patch] Add tests for atexit/on_exit firing order

[patch] Add tests for atexit/on_exit firing order

[Paul Pluzhnikov]

[-- Attachment #1: Type: text/plain, Size: 499 bytes --]

Greetings,

While working a on patch for bz14333, I discovered that there are no
tests for ordering of functions registered with atexit/on_exit, and in
particular the case where such function itself registers new exit
handlers.

This patch adds such test. I am using on_exit here because it
conveniently allows passing an argument.


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

        * stdlib/Makefile (tests): Add tst-on_exit
        * stdlib/tst-on_exit.c: New.


-- 
Paul Pluzhnikov

[-- Attachment #2: glibc-on_exit-20170710.txt --]
[-- Type: text/plain, Size: 2120 bytes --]

diff --git a/stdlib/Makefile b/stdlib/Makefile
index 0314d5926b..cc9f9215e4 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -80,7 +80,7 @@ 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-on_exit
 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-on_exit.c b/stdlib/tst-on_exit.c
new file mode 100644
index 0000000000..0de3b68525
--- /dev/null
+++ b/stdlib/tst-on_exit.c
@@ -0,0 +1,68 @@
+#include <assert.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#define MAX_ON_EXIT 10
+static int expected[MAX_ON_EXIT];
+static int next_slot = 0;
+static int next_expected = 0;
+
+static void my_on_exit (void (*fn) (int status, void *));
+
+static void
+fn1 (int status, void *arg)
+{
+  intptr_t k = (intptr_t) arg;
+
+  printf ("fn1:\t\t%p %d\n", fn1, (int) k);
+  if (next_slot < 1 || expected[--next_slot] != k)
+    _exit (1);
+}
+
+static void
+fn2 (int status, void *arg)
+{
+  intptr_t k = (intptr_t) arg;
+
+  printf ("fn2:\t\t%p %d\n", fn2, (int) k);
+  if (next_slot < 1 || expected[--next_slot] != k)
+    _exit (1);
+  my_on_exit (fn1);
+}
+
+static void
+fn3 (int status, void *arg)
+{
+  intptr_t k = (intptr_t) arg;
+
+  printf ("fn3:\t\t%p %d\n", fn3, (int) k);
+  if (next_slot < 1 || expected[--next_slot] != k)
+    _exit (1);
+  my_on_exit (fn2);
+}
+
+static void
+my_on_exit (void (*fn) (int, void *))
+{
+  intptr_t k = ++next_expected;
+
+  printf ("on_exit:\t%p %d\n", fn, (int) k);
+  on_exit (fn, (void *) k);
+  assert (next_slot < MAX_ON_EXIT);
+  expected[next_slot++] = k;
+}
+
+static int
+do_test (void)
+{
+  my_on_exit (fn2);
+  my_on_exit (fn1);
+  my_on_exit (fn2);
+  my_on_exit (fn3);
+  return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"

Re: [patch] Add tests for atexit/on_exit firing order

[Carlos O'Donell]

On 07/10/2017 11:00 AM, Paul Pluzhnikov wrote:
> Greetings,
> 
> While working a on patch for bz14333, I discovered that there are no
> tests for ordering of functions registered with atexit/on_exit, and in
> particular the case where such function itself registers new exit
> handlers.
> 
> This patch adds such test. I am using on_exit here because it
> conveniently allows passing an argument.
> 
> 
> 2017-07-10  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>         * stdlib/Makefile (tests): Add tst-on_exit
>         * stdlib/tst-on_exit.c: New.

Paul,

Always awesome to see new tests! :-)

* First line of test needs to describe the test, and BZ#.

* Test needs a copyright header.

* Test needs an explanatory paragraph talking about what is
  being tested, why, and the expected results.

* Test should use new support driver.
  e.g. #include <support/test-driver.c>

Thanks.

-- 
Cheers,
Carlos.

Re: [patch] Add tests for atexit/on_exit firing order

[Joseph Myers]

On Mon, 10 Jul 2017, Paul Pluzhnikov wrote:

> Greetings,
> 
> While working a on patch for bz14333, I discovered that there are no
> tests for ordering of functions registered with atexit/on_exit, and in
> particular the case where such function itself registers new exit
> handlers.
> 
> This patch adds such test. I am using on_exit here because it
> conveniently allows passing an argument.

I'd think that it would make sense to test all of atexit, on_exit, 
at_quick_exit this way.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch] Add tests for atexit/on_exit firing order

[Paul Pluzhnikov]

On Mon, Jul 10, 2017 at 8:18 AM, Joseph Myers <joseph@codesourcery.com> wrote:

>> This patch adds such test. I am using on_exit here because it
>> conveniently allows passing an argument.
>
> I'd think that it would make sense to test all of atexit, on_exit,
> at_quick_exit this way.

I could be missing something, but I don't see an easy way to test
atexit and at_quick_exit the same way due to them not taking an
argument.

To test atexit, I would have to implement separate fn1 .. fn8 and
hard-code expected call sequence into each of them, wouldn't I?

That would make for a more verbose and more difficult to modify test,
and given that all 3 functions currently share implementation, that
seems like an overkill. Of course if later the implementation is
un-shared, it would be nice to have a test case for each. But it seems
unlikely to ever happen.

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Add tests for atexit/on_exit firing order

[Szabolcs Nagy]

On 10/07/17 16:39, Paul Pluzhnikov wrote:
> On Mon, Jul 10, 2017 at 8:18 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> 
>>> This patch adds such test. I am using on_exit here because it
>>> conveniently allows passing an argument.
>>
>> I'd think that it would make sense to test all of atexit, on_exit,
>> at_quick_exit this way.
> 
> I could be missing something, but I don't see an easy way to test
> atexit and at_quick_exit the same way due to them not taking an
> argument.
> 
> To test atexit, I would have to implement separate fn1 .. fn8 and
> hard-code expected call sequence into each of them, wouldn't I?
> 
> That would make for a more verbose and more difficult to modify test,
> and given that all 3 functions currently share implementation, that
> seems like an overkill. Of course if later the implementation is
> un-shared, it would be nice to have a test case for each. But it seems
> unlikely to ever happen.
> 

you could call __cxa_atexit and __cxa_at_quick_exit

or just use globals.

Re: [patch] Add tests for atexit/on_exit firing order

[Joseph Myers]

On Mon, 10 Jul 2017, Paul Pluzhnikov wrote:

> On Mon, Jul 10, 2017 at 8:18 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> 
> >> This patch adds such test. I am using on_exit here because it
> >> conveniently allows passing an argument.
> >
> > I'd think that it would make sense to test all of atexit, on_exit,
> > at_quick_exit this way.
> 
> I could be missing something, but I don't see an easy way to test
> atexit and at_quick_exit the same way due to them not taking an
> argument.
> 
> To test atexit, I would have to implement separate fn1 .. fn8 and
> hard-code expected call sequence into each of them, wouldn't I?

You might have more separate functions, however the call sequence is 
encoded.  There are various ways with wrapper functions, macros etc. it 
should be possible to share the same test structure for all the functions 
(e.g. have wrappers for each function that call it with different 
arguments, and have my_on_exit select such a wrapper from an array).

> That would make for a more verbose and more difficult to modify test,
> and given that all 3 functions currently share implementation, that
> seems like an overkill. Of course if later the implementation is
> un-shared, it would be nice to have a test case for each. But it seems
> unlikely to ever happen.

Every public interface should have adequate test coverage.  All three are 
public interfaces, so all three should have test coverage.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch] Add tests for atexit/on_exit firing order

[Joseph Myers]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch] Add tests for atexit/on_exit firing order

[Paul Pluzhnikov]

[-- Attachment #1: Type: text/plain, Size: 664 bytes --]

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.

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.
        * stdlib/tst-on_exit.c: New.

-- 
Paul Pluzhnikov

[-- Attachment #2: glibc-atexit-20170724.txt --]
[-- Type: text/plain, Size: 7963 bytes --]

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
+
 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.
+
+   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..a585614733
--- /dev/null
+++ b/stdlib/tst-atexit-common.c
@@ -0,0 +1,86 @@
+/* Helper file for tst-{atexit,at_quick_exit,cxa_atexit,on_exit}.
+
+   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
+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";
+  if (strcmp (crumbs, expected) == 0)
+    _exit (0);
+
+  printf ("crumbs:   %s\n", crumbs);
+  printf ("expected: %s\n", expected);
+  _exit (1);
+}
+
+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 (0);
+}
+
+#define TEST_FUNCTION do_test
+#include <support/test-driver.c>
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.
+
+   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>
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.
+
+   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>
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.
+
+   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>

Re: [patch] Add tests for atexit/on_exit firing order

[Paul Pluzhnikov]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

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.

-- 
Paul Pluzhnikov

[-- Attachment #2: glibc-atexit-20170724a.txt --]
[-- Type: text/plain, Size: 8028 bytes --]

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
+
 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.
+
+   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}.
+
+   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
+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";
+  if (strcmp (crumbs, expected) == 0)
+    _exit (0);
+
+  printf ("crumbs:   %s\n", crumbs);
+  printf ("expected: %s\n", expected);
+  _exit (1);
+}
+
+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>
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.
+
+   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>
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.
+
+   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>
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.
+
+   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>

Re: [patch] Add tests for atexit/on_exit firing order

[Paul Pluzhnikov]

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?


-- 
Paul Pluzhnikov

Re: [patch] Add tests for atexit/on_exit firing order

[Paul Pluzhnikov]

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?
-- 
Paul Pluzhnikov

Re: [patch] Add tests for atexit/on_exit firing order

[Carlos O'Donell]

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.

-- 
Cheers,
Carlos.

Re: [patch] Add tests for atexit/on_exit firing order

[Paul Pluzhnikov]

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.

-- 
Paul Pluzhnikov