diff mbox

Add tests for atexit/on_exit firing order

Message ID CALoOobMo+n2qfftAYG6Tyibg7Y7iCDn0PEMuBhvJyzwT4vVxZA@mail.gmail.com
State New, archived
Headers

Commit Message

Paul Pluzhnikov July 10, 2017, 3 p.m. UTC 
  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.

Comments

Carlos O'Donell July 10, 2017, 3:09 p.m. UTC | #1 
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.
Joseph Myers July 10, 2017, 3:18 p.m. UTC | #2 
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.
Paul Pluzhnikov July 10, 2017, 3:39 p.m. UTC | #3 
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,
Szabolcs Nagy July 10, 2017, 3:45 p.m. UTC | #4 
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.
Joseph Myers July 10, 2017, 3:55 p.m. UTC | #5 
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 Myers July 10, 2017, 3:56 p.m. UTC | #6 
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.
diff mbox

Patch

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"