Re: [PATCH] nptl: Add test nptl/tst-pthread-perthread-inherit

[Carlos O'Donell <carlos at redhat dot com>]

On 6/28/19 4:05 PM, Florian Weimer wrote:
> This test checks the inheritance behavior of the per-thread flags.
> 
> It is also a regular (non-xtest) for pthread_attr_setperthreadids_np
> and pthread_attr_getperthreadids_np.
> 
> 2019-06-28  Florian Weimer  <fweimer@redhat.com>
> 
> 	* nptl/Makefile (tests): Add tst-pthread-perthread-inherit.
> 	* nptl/tst-pthread-perthread-inherit.c: New file.
> 

Great test overall! I have only one qualm about the boolean vs scope
comparison (see below).

> diff --git a/nptl/Makefile b/nptl/Makefile
> index 52913cc2f0..56f1578860 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -324,7 +324,8 @@ tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
>  	tst-rwlock-pwn \
>  	tst-rwlock-tryrdlock-stall tst-rwlock-trywrlock-stall \
>  	tst-unwind-thread \
> -	tst-pthread-perthreadfs tst-pthread-perthreadfs-chroot
> +	tst-pthread-perthreadfs tst-pthread-perthreadfs-chroot \
> +	tst-pthread-perthread-inherit

OK.

>  
>  tests-internal := tst-rwlock19 tst-rwlock20 \
>  		  tst-sem11 tst-sem12 tst-sem13 \
> diff --git a/nptl/tst-pthread-perthread-inherit.c b/nptl/tst-pthread-perthread-inherit.c
> new file mode 100644
> index 0000000000..913deffdcc
> --- /dev/null
> +++ b/nptl/tst-pthread-perthread-inherit.c
> @@ -0,0 +1,203 @@
> +/* Test inheritance of per-thread attributes.
> +   Copyright (C) 2019 Free Software Foundation, Inc.

OK.

> +   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 <errno.h>
> +#include <stdbool.h>
> +#include <stdlib.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/xthread.h>
> +
> +/* Controls how an attribute is applied. */
> +enum kind { null_attribute, default_attribute, attribute_per_process,
> +            attribute_per_thread };
> +
> +/* Determine the combined per-thread/per-process status from the two
> +   requested attributes.  */
> +static int
> +combine_kind (enum kind a, enum kind b)
> +{
> +  if (a == attribute_per_thread || b == attribute_per_thread)
> +    return PTHREAD_PER_THREAD_NP;
> +  else
> +    return PTHREAD_PER_PROCESS_NP;
> +}
> +
> +/* Convert one kind to the corresponding PTHREAD_PER_PROCESS_NP,
> +   PTHREAD_PER_THREAD_NP flag.  */
> +static int
> +convert_kind (enum kind k)
> +{
> +  if (k == attribute_per_thread)
> +    return PTHREAD_PER_THREAD_NP;
> +  else
> +    return PTHREAD_PER_PROCESS_NP;
> +}
> +
> +/* Return true if the thread has per-thread IDs.  */
> +static bool
> +perthread_flag (pthread_t thr,
> +                int (*getter) (const pthread_attr_t *, int *))
> +{
> +  pthread_attr_t attr;
> +  int ret = pthread_getattr_np (thr, &attr);
> +  if (ret != 0)
> +    {
> +      errno = ret;
> +      FAIL_EXIT1 ("pthread_getattr_np: %m");
> +    }
> +  int flag = -1;
> +  TEST_COMPARE (getter (&attr, &flag), 0);
> +  if (flag != PTHREAD_PER_THREAD_NP)
> +    TEST_COMPARE (flag, PTHREAD_PER_PROCESS_NP);
> +  xpthread_attr_destroy (&attr);
> +  return flag == PTHREAD_PER_THREAD_NP;
> +}

OK.

> +
> +/* The loop in do_test iterates through the Cartesian product of all
> +   possible values.  */
> +static enum kind outer_perthreadfs;
> +static enum kind inner_perthreadfs;
> +static enum kind outer_perthreadids;
> +static enum kind inner_perthreadids;
> +
> +static void *
> +inner_thread (void *closure)
> +{
> +  TEST_COMPARE (perthread_flag (pthread_self (),
> +                                pthread_attr_getperthreadfs_np),
> +                combine_kind (outer_perthreadfs, inner_perthreadfs));
> +  TEST_COMPARE (perthread_flag (pthread_self (),
> +                                pthread_attr_getperthreadids_np),
> +                combine_kind (outer_perthreadids, inner_perthreadids));

I don't like that this compares a boolean to a scope value, it's
not explicit enough about what we're comparing and so is confusing.

Please have perthread_flag return a proper scope value to compare,
or refactor to make them both compare boolean?

> +  return NULL;
> +}
> +
> +static void *
> +outer_thread (void *attr)
> +{
> +  TEST_COMPARE (perthread_flag (pthread_self (),
> +                                pthread_attr_getperthreadfs_np),
> +                convert_kind (outer_perthreadfs));
> +  TEST_COMPARE (perthread_flag (pthread_self (),
> +                                pthread_attr_getperthreadids_np),
> +                convert_kind (outer_perthreadids));
> +  pthread_t thr = xpthread_create (attr, inner_thread, NULL);
> +  TEST_COMPARE (perthread_flag (thr, pthread_attr_getperthreadfs_np),
> +                combine_kind (outer_perthreadfs, inner_perthreadfs));
> +  TEST_COMPARE (perthread_flag (thr, pthread_attr_getperthreadids_np),
> +                combine_kind (outer_perthreadids, inner_perthreadids));
> +  return xpthread_join (thr);

OK.

> +}
> +
> +/* Apply the attribute KIND according to SETTER to ATTR.  */
> +static void
> +attribute_apply (pthread_attr_t *attr, enum kind kind,
> +                 int (*setter) (pthread_attr_t *attr, int))
> +{
> +  switch (kind)
> +    {
> +    case default_attribute:
> +      return;
> +
> +    case attribute_per_process:
> +    case attribute_per_thread:
> +      TEST_COMPARE (setter (attr, convert_kind (kind)), 0);

OK.

> +      return;
> +
> +    case null_attribute:
> +      /* Report failure below.  */
> +      ;
> +    }
> +
> +  FAIL_EXIT1 ("invalid kind: %d", (int) kind);

OK.

> +}
> +
> +/* Create a new attribute according to both kinds.  */
> +static pthread_attr_t *
> +make_attribute (enum kind perthreadfs, enum kind perthreadids)
> +{
> +  if (perthreadfs == null_attribute)
> +    {
> +      TEST_COMPARE (perthreadids, null_attribute);
> +      return NULL;
> +    }
> +  pthread_attr_t *result = xmalloc (sizeof (*result));
> +  xpthread_attr_init (result);
> +  attribute_apply (result, perthreadfs, pthread_attr_setperthreadfs_np);
> +  attribute_apply (result, perthreadids, pthread_attr_setperthreadids_np);
> +  return result;
> +}
> +
> +/* Deallocate the attribute pointer returned from make_attribute.  */
> +static void
> +free_attribute (pthread_attr_t *attr)
> +{
> +  if (attr != NULL)
> +    {
> +      xpthread_attr_destroy (attr);
> +      free (attr);
> +    }
> +}
> +
> +static int
> +do_test (void)
> +{
> +  for (outer_perthreadfs = null_attribute;
> +       outer_perthreadfs <= attribute_per_thread;
> +       ++outer_perthreadfs)
> +    for (inner_perthreadfs = null_attribute;
> +         inner_perthreadfs <= attribute_per_thread;
> +         ++inner_perthreadfs)
> +      for (outer_perthreadids = null_attribute;
> +           outer_perthreadids <= attribute_per_thread;
> +           ++outer_perthreadids)
> +        for (inner_perthreadids = null_attribute;
> +             inner_perthreadids <= attribute_per_thread;
> +             ++inner_perthreadids)

OK. Yay! :-) All combinations!

> +          {
> +            /* Some combinations are impossibe.  If we must pass a

s/impossibe/impossible/g

> +               NULL attribute at one level, then that is true for both
> +               kinds of properties.  */
> +            if ((outer_perthreadfs == null_attribute)
> +                != (outer_perthreadids == null_attribute))
> +              continue;
> +            if ((inner_perthreadfs == null_attribute)
> +                != (inner_perthreadids == null_attribute))
> +              continue;

Do you *have* to exclude these? Could we let the extra tests run?

Sure they don't make sense, but why do you say they are "impossible?"

> +
> +            pthread_attr_t *outer_attr
> +              = make_attribute (outer_perthreadfs, outer_perthreadids);
> +            pthread_attr_t *inner_attr
> +              = make_attribute (inner_perthreadfs, inner_perthreadids);
> +            pthread_t thr = xpthread_create (outer_attr,
> +                                             outer_thread, inner_attr);
> +            TEST_COMPARE (perthread_flag (thr,
> +                                          pthread_attr_getperthreadfs_np),
> +                convert_kind (outer_perthreadfs));
> +            TEST_COMPARE (perthread_flag (thr,
> +                                          pthread_attr_getperthreadids_np),
> +                          convert_kind (outer_perthreadids));
> +            xpthread_join (thr);
> +            free_attribute (inner_attr);
> +            free_attribute (outer_attr);
> +          }
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> 


-- 
Cheers,
Carlos.