nptl: Add test nptl/tst-pthread-perthread-inherit

Message ID 8736jtlbl6.fsf@oldenburg2.str.redhat.com
State New
Headers show
Series
  • nptl: Add test nptl/tst-pthread-perthread-inherit
Related show

Commit Message

Florian Weimer June 28, 2019, 8:05 p.m.
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.

Comments

Carlos O'Donell June 30, 2019, 5:02 a.m. | #1
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.
Florian Weimer June 30, 2019, 9:10 a.m. | #2
* Carlos O'Donell:

>> +               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?"


If threadfs requires a NULL attribute, and threadids requres
PTHREAD_PER_THREAD_NP, then we cannot run the subtest because the NULL
value does not carry any attributes.

Thanks,
Florian
Carlos O'Donell July 1, 2019, 3:23 a.m. | #3
On 6/30/19 5:10 AM, Florian Weimer wrote:
> * Carlos O'Donell:

> 

>>> +               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?"

> 

> If threadfs requires a NULL attribute, and threadids requres

> PTHREAD_PER_THREAD_NP, then we cannot run the subtest because the NULL

> value does not carry any attributes.


I don't follow.

You have two functions, pthread_attr_setperthreadfs_np, and
pthread_attr_setperthreadids_np.

You want to call them with the permutation of their options
and check the results.

At an API level the arguments used to call them are unrelated.

I don't understand what you what to express in the last paragraph.

-- 
Cheers,
Carlos.
Florian Weimer July 1, 2019, 4:47 a.m. | #4
* Carlos O'Donell:

> On 6/30/19 5:10 AM, Florian Weimer wrote:

>> * Carlos O'Donell:

>> 

>>>> +               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?"

>> 

>> If threadfs requires a NULL attribute, and threadids requres

>> PTHREAD_PER_THREAD_NP, then we cannot run the subtest because the NULL

>> value does not carry any attributes.

>

> I don't follow.

>

> You have two functions, pthread_attr_setperthreadfs_np, and

> pthread_attr_setperthreadids_np.

>

> You want to call them with the permutation of their options

> and check the results.

>

> At an API level the arguments used to call them are unrelated.

>

> I don't understand what you what to express in the last paragraph.


pthread_create has only one attribute argument.  If it is NULL, it
means that both attributes are absent.
Carlos O'Donell July 1, 2019, 4:52 a.m. | #5
On 7/1/19 12:47 AM, Florian Weimer wrote:
> * Carlos O'Donell:

> 

>> On 6/30/19 5:10 AM, Florian Weimer wrote:

>>> * Carlos O'Donell:

>>>

>>>>> +               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?"

>>>

>>> If threadfs requires a NULL attribute, and threadids requres

>>> PTHREAD_PER_THREAD_NP, then we cannot run the subtest because the NULL

>>> value does not carry any attributes.

>>

>> I don't follow.

>>

>> You have two functions, pthread_attr_setperthreadfs_np, and

>> pthread_attr_setperthreadids_np.

>>

>> You want to call them with the permutation of their options

>> and check the results.

>>

>> At an API level the arguments used to call them are unrelated.

>>

>> I don't understand what you what to express in the last paragraph.

> 

> pthread_create has only one attribute argument.  If it is NULL, it

> means that both attributes are absent.


Ah, In my mind I was just "promoting" the NULL attribute to a non-NULL
when an attribute is needed i.e. the merger of both requirements ends
up being a non-NULL attribute.

In your opinion this becomes a violation of the test constraint.
I see your point. I'm OK with the code as-is then.

-- 
Cheers,
Carlos.
Florian Weimer July 1, 2019, 4:31 p.m. | #6
I had to fix a race condition related to the use of pthread_getattr_np.
I believe the patch below should address your concerns.

Thanks,
Florian

nptl: Add test nptl/tst-pthread-perthread-inherit

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.

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
 
 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..99feb01dc9
--- /dev/null
+++ b/nptl/tst-pthread-perthread-inherit.c
@@ -0,0 +1,220 @@
+/* Test inheritance of per-thread attributes.
+   Copyright (C) 2019 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 <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 either PTHREAD_PER_PROCESS_NP or PTHREAD_PER_THREAD_NP.  */
+static int
+scope (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;
+}
+
+/* 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;
+
+/* Used to prevent premature thread exit, so that we can safely call
+   pthread_getattr_np.  */
+static pthread_barrier_t barrier;
+
+static void *
+inner_thread (void *closure)
+{
+  TEST_COMPARE (scope (pthread_self (),
+                       pthread_attr_getperthreadfs_np),
+                combine_kind (outer_perthreadfs, inner_perthreadfs));
+  TEST_COMPARE (scope (pthread_self (),
+                       pthread_attr_getperthreadids_np),
+                combine_kind (outer_perthreadids, inner_perthreadids));
+  /* Delay exit of the thread until the outer thread has read the
+     attributes.  */
+  xpthread_barrier_wait (&barrier);
+  return NULL;
+}
+
+static void *
+outer_thread (void *attr)
+{
+  TEST_COMPARE (scope (pthread_self (),
+                       pthread_attr_getperthreadfs_np),
+                convert_kind (outer_perthreadfs));
+  TEST_COMPARE (scope (pthread_self (),
+                       pthread_attr_getperthreadids_np),
+                convert_kind (outer_perthreadids));
+  pthread_t thr = xpthread_create (attr, inner_thread, NULL);
+  TEST_COMPARE (scope (thr, pthread_attr_getperthreadfs_np),
+                combine_kind (outer_perthreadfs, inner_perthreadfs));
+  TEST_COMPARE (scope (thr, pthread_attr_getperthreadids_np),
+                combine_kind (outer_perthreadids, inner_perthreadids));
+  /* Delay exit of the thread until the main thread has read the
+     attributes.  */
+  xpthread_barrier_wait (&barrier);
+  return xpthread_join (thr);
+}
+
+/* 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);
+      return;
+
+    case null_attribute:
+      /* Report failure below.  */
+      ;
+    }
+
+  FAIL_EXIT1 ("invalid kind: %d", (int) kind);
+}
+
+/* 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)
+{
+  /* Used to delay thread exit, so that pthread_getattr_np works
+     reliably.  Inner thread, outer thread, and main thread are
+     waiting.  */
+  xpthread_barrier_init (&barrier, NULL, 3);
+
+  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)
+          {
+            /* Some combinations are impossible.  If we must pass a
+               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;
+
+            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 (scope (thr, pthread_attr_getperthreadfs_np),
+                          convert_kind (outer_perthreadfs));
+            TEST_COMPARE (scope (thr, pthread_attr_getperthreadids_np),
+                          convert_kind (outer_perthreadids));
+            /* The threads may now exit, after we have checked the
+               attributes.  */
+            xpthread_barrier_wait (&barrier);
+            xpthread_join (thr);
+            free_attribute (inner_attr);
+            free_attribute (outer_attr);
+          }
+
+  xpthread_barrier_destroy (&barrier);
+  return 0;
+}
+
+#include <support/test-driver.c>
Carlos O'Donell July 4, 2019, 4:07 a.m. | #7
On 7/1/19 12:31 PM, Florian Weimer wrote:
> I had to fix a race condition related to the use of pthread_getattr_np.

> I believe the patch below should address your concerns.

> 

> Thanks,

> Florian

> 

> nptl: Add test nptl/tst-pthread-perthread-inherit

> 

> 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.


LGTM

Reviewed-by: Carlos O'Donell <carlos@redhat.com>


> 2019-06-28  Florian Weimer  <fweimer@redhat.com>

> 

> 	* nptl/Makefile (tests): Add tst-pthread-perthread-inherit.

> 	* nptl/tst-pthread-perthread-inherit.c: New file.

> 

> 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

>   

>   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..99feb01dc9

> --- /dev/null

> +++ b/nptl/tst-pthread-perthread-inherit.c

> @@ -0,0 +1,220 @@

> +/* Test inheritance of per-thread attributes.

> +   Copyright (C) 2019 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 <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 either PTHREAD_PER_PROCESS_NP or PTHREAD_PER_THREAD_NP.  */

> +static int

> +scope (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;

> +}

> +

> +/* 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;

> +

> +/* Used to prevent premature thread exit, so that we can safely call

> +   pthread_getattr_np.  */

> +static pthread_barrier_t barrier;

> +

> +static void *

> +inner_thread (void *closure)

> +{

> +  TEST_COMPARE (scope (pthread_self (),

> +                       pthread_attr_getperthreadfs_np),

> +                combine_kind (outer_perthreadfs, inner_perthreadfs));

> +  TEST_COMPARE (scope (pthread_self (),

> +                       pthread_attr_getperthreadids_np),

> +                combine_kind (outer_perthreadids, inner_perthreadids));

> +  /* Delay exit of the thread until the outer thread has read the

> +     attributes.  */

> +  xpthread_barrier_wait (&barrier);

> +  return NULL;

> +}

> +

> +static void *

> +outer_thread (void *attr)

> +{

> +  TEST_COMPARE (scope (pthread_self (),

> +                       pthread_attr_getperthreadfs_np),

> +                convert_kind (outer_perthreadfs));

> +  TEST_COMPARE (scope (pthread_self (),

> +                       pthread_attr_getperthreadids_np),

> +                convert_kind (outer_perthreadids));

> +  pthread_t thr = xpthread_create (attr, inner_thread, NULL);

> +  TEST_COMPARE (scope (thr, pthread_attr_getperthreadfs_np),

> +                combine_kind (outer_perthreadfs, inner_perthreadfs));

> +  TEST_COMPARE (scope (thr, pthread_attr_getperthreadids_np),

> +                combine_kind (outer_perthreadids, inner_perthreadids));

> +  /* Delay exit of the thread until the main thread has read the

> +     attributes.  */

> +  xpthread_barrier_wait (&barrier);

> +  return xpthread_join (thr);

> +}

> +

> +/* 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);

> +      return;

> +

> +    case null_attribute:

> +      /* Report failure below.  */

> +      ;

> +    }

> +

> +  FAIL_EXIT1 ("invalid kind: %d", (int) kind);

> +}

> +

> +/* 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)

> +{

> +  /* Used to delay thread exit, so that pthread_getattr_np works

> +     reliably.  Inner thread, outer thread, and main thread are

> +     waiting.  */

> +  xpthread_barrier_init (&barrier, NULL, 3);

> +

> +  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)

> +          {

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

> +               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;

> +

> +            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 (scope (thr, pthread_attr_getperthreadfs_np),

> +                          convert_kind (outer_perthreadfs));

> +            TEST_COMPARE (scope (thr, pthread_attr_getperthreadids_np),

> +                          convert_kind (outer_perthreadids));

> +            /* The threads may now exit, after we have checked the

> +               attributes.  */

> +            xpthread_barrier_wait (&barrier);

> +            xpthread_join (thr);

> +            free_attribute (inner_attr);

> +            free_attribute (outer_attr);

> +          }

> +

> +  xpthread_barrier_destroy (&barrier);

> +  return 0;

> +}

> +

> +#include <support/test-driver.c>

> 



-- 
Cheers,
Carlos.

Patch

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
 
 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.
+   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;
+}
+
+/* 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));
+  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);
+}
+
+/* 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);
+      return;
+
+    case null_attribute:
+      /* Report failure below.  */
+      ;
+    }
+
+  FAIL_EXIT1 ("invalid kind: %d", (int) kind);
+}
+
+/* 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)
+          {
+            /* Some combinations are impossibe.  If we must pass a
+               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;
+
+            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>