This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] nptl: Avoid expected SIGALRM in most tests [BZ #20432]


On 08/17/2016 08:54 AM, Florian Weimer wrote:
> Before this change, several tests did not detect early deadlocks
> because they used SIGALRM as the expected signal, and they ran
> for the full default TIMEOUT seconds.
> 
> This commit adds a new delayed_exit function to the test skeleton,
> along with several error-checking wrappers to pthread functions.
> Additional error checking is introduced into several tests.

This is much better than what we have now, so please commit this.

My comments are purely optional and summarized as:

- Please try to use "error:" prefix for all error messages, adding
  some more, and cleaning up others.

- Might we consider robust xpthread_create handling in the face of
  EAGAIN.

Overall I think this patch is a step in the right direction.

I still dislike timed tests because you have no real guarantee that,
for example in tst-eintr1, that the EINTR source thread has delivered
any signals at all. The testing should verify that at least N signals
have been delivered to the processes in question, but that's harder
and certainly out of scope.

This patch is OK.

> 2016-08-17  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #20432]
> 	Avoid expected SIGALRM signals.
> 	* test-skeleton.c (xpthread_sigmask, xpthread_mutex_lock)
> 	(xpthread_spin_lock, xpthread_cond_wait, xpthread_barrier_wait)
> 	(xpthread_create, xpthread_detach, xpthread_join)
> 	(delayed_exit_thread, delayed_exit): New functions.
> 	* nptl/tst-cond3 (EXPECTED_SIGNAL): Remove.
> 	(tf): Use xpthread_cond_wait.
> 	(do_test): Likewise.  Replace alarm with delayed_exit.
> 	* nptl/tst-eintr1.c (EXPECTED_SIGNAL, TIMEOUT): Remove.
> 	(do_test): Call delayed_exit.  Report failure.
> 	* nptl/tst-eintr2.c (EXPECTED_SIGNAL, TIMEOUT): Remove.
> 	(do_test): Call delayed_exit.
> 	* nptl/tst-eintr3.c (EXPECTED_SIGNAL, TIMEOUT): Remove.
> 	(do_test): Call delayed_exit.  Use xpthread_join.  Report error.
> 	* nptl/tst-eintr4.c (EXPECTED_SIGNAL, TIMEOUT): Remove.
> 	(do_test): Call delayed_exit.  Use xpthread_barrier_wait.  Report
> 	error.
> 	* nptl/tst-eintr5.c (EXPECTED_SIGNAL, TIMEOUT): Remove.
> 	(do_test): Call delayed_exit.  Use xpthread_cond_wait.  Report
> 	error.
> 	* nptl/tst-exit2.c (EXPECTED_SIGNAL): Remove.
> 	(do_test): Call delayed_exit.
> 	* nptl/tst-exit3.c (EXPECTED_SIGNAL): Remove.
> 	(do_test): Call delayed_exit.
> 	* nptl/tst-mutex6.c (EXPECTED_SIGNAL): Remove.
> 	(do_test): Call delayed_exit instead of alarm.  Use
> 	xpthread_mutex_lock.
> 	* nptl/tst-rwlock5.c (EXPECTED_SIGNAL): Remove.
> 	(do_test): Call delayed_exit instead of alarm.  Use
> 	xpthread_mutex_lock.
> 	* nptl/tst-sem2.c (EXPECTED_SIGNAL): Remove.
> 	(do_test): Call delayed_exit instead of alarm.
> 	* nptl/tst-spin3.c (EXPECTED_SIGNAL): Remove.
> 	(do_test): Call delayed_exit instead of alarm.  Use
> 	xpthread_spin_lock.
> 	* nptl/tst-stdio1.c (EXPECTED_SIGNAL): Remove.
> 	(do_test): Call delayed_exit instead of alarm.  Use
> 	xpthread_join.
> 
> diff --git a/nptl/tst-cond3.c b/nptl/tst-cond3.c
> index ff904e4..724630f 100644
> --- a/nptl/tst-cond3.c
> +++ b/nptl/tst-cond3.c
> @@ -22,6 +22,10 @@
>  #include <string.h>
>  #include <unistd.h>
>  
> +static int do_test (void);
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
>  
>  /* Note that this test requires more than the standard.  It is
>     required that there are no spurious wakeups if only more readers
> @@ -50,7 +54,8 @@ tf (void *arg)
>      }
>  
>    /* This call should never return.  */
> -  pthread_cond_wait (&cond, &mut);
> +  xpthread_cond_wait (&cond, &mut);
> +  puts ("error: pthread_cond_wait in tf returned");

OK.

>  
>    /* We should never get here.  */
>    exit (1);
> @@ -96,17 +101,11 @@ do_test (void)
>  	}
>      }
>  
> -  /* Set an alarm for 1 second.  The wrapper will expect this.  */
> -  alarm (1);
> +  delayed_exit (1);
>  
>    /* This call should never return.  */
> -  pthread_cond_wait (&cond, &mut);
> +  xpthread_cond_wait (&cond, &mut);
>  
> -  puts ("cond_wait returned");
> +  puts ("pthread_cond_wait in do_test returned");

Add "error:"

>    return 1;
>  }
> -
> -
> -#define EXPECTED_SIGNAL SIGALRM
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> diff --git a/nptl/tst-eintr1.c b/nptl/tst-eintr1.c
> index 11cd876..0946894 100644
> --- a/nptl/tst-eintr1.c
> +++ b/nptl/tst-eintr1.c
> @@ -23,6 +23,11 @@
>  #include <stdlib.h>
>  #include <string.h>
>  
> +static int do_test (void);
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> +
>  #include "eintr.c"
>  
>  
> @@ -92,13 +97,8 @@ do_test (void)
>  	}
>      }
>  
> +  delayed_exit (3);
> +  /* This call must never return.  */
>    (void) tf1 (NULL);
> -  /* NOTREACHED */
> -

Print a failure message please.

> -  return 0;
> +  return 1;
>  }
> -
> -#define EXPECTED_SIGNAL SIGALRM
> -#define TIMEOUT 3
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> diff --git a/nptl/tst-eintr2.c b/nptl/tst-eintr2.c
> index 7f50f4b..0ef6d30 100644
> --- a/nptl/tst-eintr2.c
> +++ b/nptl/tst-eintr2.c
> @@ -24,6 +24,11 @@
>  #include <string.h>
>  #include <sys/time.h>
>  
> +static int do_test (void);
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> +
>  #include "eintr.c"
>  
>  
> @@ -103,6 +108,7 @@ do_test (void)
>        exit (1);
>      }
>  
> +  delayed_exit (3);
>    /* This call must never return.  */
>    e = pthread_mutex_lock (&m1);
>    printf ("main: mutex_lock returned: %s\n",

Would be nice to add "error:" to the front of this.

> @@ -110,8 +116,3 @@ do_test (void)
>  
>    return 1;
>  }
> -
> -#define EXPECTED_SIGNAL SIGALRM
> -#define TIMEOUT 3
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> diff --git a/nptl/tst-eintr3.c b/nptl/tst-eintr3.c
> index d2c32b9..f6b2ccf 100644
> --- a/nptl/tst-eintr3.c
> +++ b/nptl/tst-eintr3.c
> @@ -23,6 +23,11 @@
>  #include <stdlib.h>
>  #include <string.h>
>  
> +static int do_test (void);
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> +
>  #include "eintr.c"
>  
>  
> @@ -56,16 +61,9 @@ do_test (void)
>        exit (1);
>      }
>  
> +  delayed_exit (1);
>    /* This call must never return.  */
> -  e = pthread_join (th, NULL);
> -
> -  if (e == EINTR)
> -    puts ("pthread_join returned with EINTR");
> -
> -  return 0;
> +  xpthread_join (th);
> +  puts ("error: pthread_join returned");

Likewise "error: ..."

> +  return 1;
>  }
> -
> -#define EXPECTED_SIGNAL SIGALRM
> -#define TIMEOUT 1
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> diff --git a/nptl/tst-eintr4.c b/nptl/tst-eintr4.c
> index 25fae34..f6d068f 100644
> --- a/nptl/tst-eintr4.c
> +++ b/nptl/tst-eintr4.c
> @@ -23,6 +23,11 @@
>  #include <stdlib.h>
>  #include <string.h>
>  
> +static int do_test (void);
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> +
>  #include "eintr.c"
>  
>  
> @@ -40,16 +45,9 @@ do_test (void)
>        exit (1);
>      }
>  
> +  delayed_exit (1);
>    /* This call must never return.  */
> -  int e = pthread_barrier_wait (&b);
> -
> -  if (e == EINTR)
> -    puts ("pthread_join returned with EINTR");
> -
> -  return 0;
> +  xpthread_barrier_wait (&b);
> +  puts ("error: pthread_barrier_wait returned");

Likewise.

> +  return 1;
>  }
> -
> -#define EXPECTED_SIGNAL SIGALRM
> -#define TIMEOUT 1
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> diff --git a/nptl/tst-eintr5.c b/nptl/tst-eintr5.c
> index be6731c..81f900e 100644
> --- a/nptl/tst-eintr5.c
> +++ b/nptl/tst-eintr5.c
> @@ -24,6 +24,11 @@
>  #include <string.h>
>  #include <sys/time.h>
>  
> +static int do_test (void);
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> +
>  #include "eintr.c"
>  
>  
> @@ -66,15 +71,9 @@ do_test (void)
>        exit (1);
>      }
>  
> +  delayed_exit (3);
>    /* This call must never return.  */
> -  e = pthread_cond_wait (&c, &m);
> -  printf ("main: cond_wait returned: %s\n",
> -	  strerror_r (e, buf, sizeof (buf)));
> -
> -  return 0;
> +  xpthread_cond_wait (&c, &m);
> +  puts ("error: pthread_cond_wait returned");
> +  return 1;

Likewise.

>  }
> -
> -#define EXPECTED_SIGNAL SIGALRM
> -#define TIMEOUT 3
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> diff --git a/nptl/tst-exit2.c b/nptl/tst-exit2.c
> index 3f5ff27..0b7a2ca 100644
> --- a/nptl/tst-exit2.c
> +++ b/nptl/tst-exit2.c
> @@ -4,6 +4,10 @@
>  #include <string.h>
>  #include <unistd.h>
>  
> +static int do_test (void);
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
>  
>  static void *
>  tf (void *arg)
> @@ -28,13 +32,11 @@ do_test (void)
>        return 1;
>      }
>  
> +  delayed_exit (1);
> +

Add failure message.

>    /* Terminate only this thread.  */
>    pthread_exit (NULL);
>  
>    /* NOTREACHED */
>    return 1;
>  }
> -
> -#define EXPECTED_SIGNAL SIGALRM
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> diff --git a/nptl/tst-exit3.c b/nptl/tst-exit3.c
> index da92c82..9481ed9 100644
> --- a/nptl/tst-exit3.c
> +++ b/nptl/tst-exit3.c
> @@ -5,6 +5,10 @@
>  #include <string.h>
>  #include <unistd.h>
>  
> +static int do_test (void);
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
>  
>  static pthread_barrier_t b;
>  
> @@ -69,13 +73,11 @@ do_test (void)
>        exit (1);
>      }
>  
> +  delayed_exit (3);
> +

Add failure message.

>    /* Terminate only this thread.  */
>    pthread_exit (NULL);
>  
>    /* NOTREACHED */
>    return 1;
>  }
> -
> -#define EXPECTED_SIGNAL SIGALRM
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> diff --git a/nptl/tst-mutex6.c b/nptl/tst-mutex6.c
> index 1940687..3e989d8 100644
> --- a/nptl/tst-mutex6.c
> +++ b/nptl/tst-mutex6.c
> @@ -23,6 +23,11 @@
>  #include <errno.h>
>  #include <stdbool.h>
>  
> +#ifndef TEST_FUNCTION
> +static int do_test (void);
> +# define TEST_FUNCTION do_test ()
> +#endif
> +#include "../test-skeleton.c"
>  
>  #ifndef ATTR
>  pthread_mutexattr_t *attr;
> @@ -62,18 +67,10 @@ do_test (void)
>        return 1;
>      }
>  
> -  /* Set an alarm for 1 second.  The wrapper will expect this.  */
> -  alarm (1);
> -
> +  delayed_exit (1);
>    /* This call should never return.  */
> -  pthread_mutex_lock (&m);
> +  xpthread_mutex_lock (&m);

Add failure message.

>  
>    puts ("2nd mutex_lock returned");
>    return 1;
>  }
> -
> -#define EXPECTED_SIGNAL SIGALRM
> -#ifndef TEST_FUNCTION
> -# define TEST_FUNCTION do_test ()
> -#endif
> -#include "../test-skeleton.c"
> diff --git a/nptl/tst-rwlock5.c b/nptl/tst-rwlock5.c
> index b6c5d8a..20fb471 100644
> --- a/nptl/tst-rwlock5.c
> +++ b/nptl/tst-rwlock5.c
> @@ -22,6 +22,10 @@
>  #include <stdlib.h>
>  #include <unistd.h>
>  
> +static int do_test (void);
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
>  
>  static pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;
>  static pthread_rwlock_t r;
> @@ -65,22 +69,16 @@ do_test (void)
>        return 1;
>      }
>  
> -  /* Set an alarm for 1 second.  The wrapper will expect this.  */
> -  alarm (1);
> -
>    if (pthread_create (&th, NULL, tf, NULL) != 0)
>      {
>        puts ("create failed");
>        return 1;
>      }
>  
> +  delayed_exit (1);
>    /* This call should never return.  */
> -  pthread_mutex_lock (&m);
> +  xpthread_mutex_lock (&m);
>  
>    puts ("2nd mutex_lock returned");

Use error: in failure message.

>    return 1;
>  }
> -
> -#define EXPECTED_SIGNAL SIGALRM
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> diff --git a/nptl/tst-sem2.c b/nptl/tst-sem2.c
> index 301dde7..1f609fc 100644
> --- a/nptl/tst-sem2.c
> +++ b/nptl/tst-sem2.c
> @@ -17,11 +17,16 @@
>     <http://www.gnu.org/licenses/>.  */
>  
>  #include <errno.h>
> +#include <pthread.h>
>  #include <semaphore.h>
>  #include <signal.h>
>  #include <stdio.h>
>  #include <unistd.h>
>  
> +static int do_test (void);
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
>  
>  static int
>  do_test (void)
> @@ -34,8 +39,7 @@ do_test (void)
>        return 1;
>      }
>  
> -  /* Set an alarm for 1 second.  The wrapper will expect this.  */
> -  alarm (1);
> +  delayed_exit (1);
>  
>    if (TEMP_FAILURE_RETRY (sem_wait (&s)) == -1)
>      {
> @@ -47,7 +51,3 @@ do_test (void)
>    puts ("wait succeeded");
>    return 1;
>  }
> -
> -#define EXPECTED_SIGNAL SIGALRM
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> diff --git a/nptl/tst-spin3.c b/nptl/tst-spin3.c
> index 8964ecc..6cb6f9d 100644
> --- a/nptl/tst-spin3.c
> +++ b/nptl/tst-spin3.c
> @@ -21,6 +21,10 @@
>  #include <stdio.h>
>  #include <unistd.h>
>  
> +static int do_test (void);
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
>  
>  static int
>  do_test (void)
> @@ -39,16 +43,11 @@ do_test (void)
>        return 1;
>      }
>  
> -  /* Set an alarm for 1 second.  The wrapper will expect this.  */
> -  alarm (1);
> +  delayed_exit (1);
>  
>    /* This call should never return.  */
> -  pthread_spin_lock (&s);
> +  xpthread_spin_lock (&s);
>  
>    puts ("2nd spin_lock returned");

Use error: in failure message.

>    return 1;
>  }
> -
> -#define EXPECTED_SIGNAL SIGALRM
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> diff --git a/nptl/tst-stdio1.c b/nptl/tst-stdio1.c
> index 2d44c16..4250e53 100644
> --- a/nptl/tst-stdio1.c
> +++ b/nptl/tst-stdio1.c
> @@ -21,6 +21,10 @@
>  #include <stdio.h>
>  #include <unistd.h>
>  
> +static int do_test (void);
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
>  
>  static void *tf (void *a)
>  {
> @@ -43,14 +47,10 @@ do_test (void)
>        _exit (1);
>      }
>  
> -  pthread_join (th, NULL);
> +  delayed_exit (1);
> +  xpthread_join (th);
>  
>    puts ("join returned");

Use error: in failure message.

>  
> -  return 0;
> +  return 1;
>  }
> -
> -
> -#define EXPECTED_SIGNAL SIGALRM
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> diff --git a/test-skeleton.c b/test-skeleton.c
> index 5a90c65..b24ce1d 100644
> --- a/test-skeleton.c
> +++ b/test-skeleton.c
> @@ -559,3 +559,160 @@ main (int argc, char *argv[])
>  #endif
>      }
>  }

I like the xFUNC variants. Thanks for this.

> +
> +/* The following functionality is only available if <pthread.h> was
> +   included before this file.  */
> +#ifdef _PTHREAD_H
> +
> +/* Call pthread_sigmask with error checking.  */
> +static void
> +xpthread_sigmask (int how, const sigset_t *set, sigset_t *oldset)
> +{
> +  if (pthread_sigmask (how, set, oldset) != 0)
> +    {
> +      write_message ("error: pthread_setmask failed\n");
> +      _exit (1);

OK.

> +    }
> +}
> +
> +/* Call pthread_mutex_lock with error checking.  */
> +__attribute__ ((unused))
> +static void
> +xpthread_mutex_lock (pthread_mutex_t *mutex)
> +{
> +  int ret = pthread_mutex_lock (mutex);
> +  if (ret != 0)
> +    {
> +      errno = ret;
> +      printf ("error: pthread_mutex_lock: %m\n");
> +      exit (1);

OK.

> +    }
> +}
> +
> +/* Call pthread_spin_lock with error checking.  */
> +__attribute__ ((unused))
> +static void
> +xpthread_spin_lock (pthread_spinlock_t *lock)
> +{
> +  int ret = pthread_spin_lock (lock);
> +  if (ret != 0)
> +    {
> +      errno = ret;
> +      printf ("error: pthread_spin_lock: %m\n");
> +      exit (1);

OK.

> +    }
> +}
> +
> +/* Call pthread_cond_wait with error checking.  */
> +__attribute__ ((unused))
> +static void
> +xpthread_cond_wait (pthread_cond_t * cond,
> +		    pthread_mutex_t * mutex)
> +{
> +  int ret = pthread_cond_wait (cond, mutex);
> +  if (ret != 0)
> +    {
> +      errno = ret;
> +      printf ("error: pthread_cond_wait: %m\n");
> +      exit (1);

OK.

> +    }
> +}
> +
> +/* Call pthread_barrier_wait with error checking.  */
> +__attribute__ ((unused))
> +static int
> +xpthread_barrier_wait (pthread_barrier_t *barrier)
> +{
> +  int ret = pthread_barrier_wait (barrier);
> +  if (ret != 0 && ret != PTHREAD_BARRIER_SERIAL_THREAD)
> +    {
> +      errno = ret;
> +      printf ("error: pthread_barrier_wait: %m\n");
> +      exit (1);
> +    }
> +  return ret;

Ok.

> +}
> +
> +/* Call pthread_create with error checking.  */
> +static pthread_t
> +xpthread_create (pthread_attr_t *attr,
> +		 void *(*thread_func) (void *), void *closure)
> +{
> +  pthread_t thr;
> +  int ret = pthread_create (&thr, attr, thread_func, closure);
> +  if (ret != 0)

Could we make this more robust by looping on EAGAIN and trying
to create the thread until it succeeds or we timeout the test?

This way embedded systems with less memory might operate correctly,
and we might avoid the slow reaping problems in the linux kernel?

> +    {
> +      errno = ret;
> +      printf ("error: pthread_create: %m\n");
> +      exit (1);
> +    }
> +  return thr;
> +}
> +
> +/* Call pthread_detach with error checking.  */
> +static void
> +xpthread_detach (pthread_t thr)
> +{
> +  int ret = pthread_detach (thr);
> +  if (ret != 0)
> +    {
> +      errno = ret;
> +      printf ("error: pthread_detach: %m\n");
> +      exit (1);
> +    }

Ok.

> +}
> +
> +/* Call pthread_join with error checking.  */
> +__attribute__ ((unused))
> +static void *
> +xpthread_join (pthread_t thr)
> +{
> +  void *result;
> +  int ret = pthread_join (thr, &result);
> +  if (ret != 0)
> +    {
> +      errno = ret;
> +      printf ("error: pthread_join: %m\n");
> +      exit (1);
> +    }
> +  return result;

OK.

> +}
> +
> +/* Used to implement the delayed_exit function defined below.  */
> +static void *
> +delayed_exit_thread (void *seconds_as_ptr)
> +{
> +  int seconds = (uintptr_t) seconds_as_ptr;
> +  struct timespec delay = { seconds, 0 };
> +  struct timespec remaining = {};
> +  if (nanosleep (&delay, &remaining) != 0)
> +    {
> +      printf ("error: nanosleep: %m\n");
> +      _exit (1);
> +    }
> +  /* Exit the process sucessfully.  */
> +  exit (0);
> +  return NULL;

OK.

> +}
> +
> +/* Exit (with status 0) after SECONDS have elapsed, from a helper
> +   thread.  The process is terminated with the exit function, so
> +   atexit handlers are executed.  */
> +__attribute__ ((unused))
> +static void
> +delayed_exit (int seconds)
> +{
> +  /* Create the new thread with all signals blocked.  */
> +  sigset_t all_blocked;
> +  sigfillset (&all_blocked);
> +  sigset_t old_set;
> +  xpthread_sigmask (SIG_SETMASK, &all_blocked, &old_set);
> +  /* Create a detached thread. */
> +  pthread_t thr = xpthread_create
> +    (NULL, delayed_exit_thread, (void *) (uintptr_t) seconds);
> +  xpthread_detach (thr);
> +  /* Restore the original signal mask.  */
> +  xpthread_sigmask (SIG_SETMASK, &old_set, NULL);

OK.

> +}
> +
> +#endif	/* _PTHREAD_H */
> 


-- 
Cheers,
Carlos.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]