nptl: Add test for callee-saved register restore in pthread_exit

Message ID 20171231192400.46AB7401BC22B@oldenburg.str.redhat.com
State New
Headers show
Series
  • nptl: Add test for callee-saved register restore in pthread_exit
Related show

Commit Message

Florian Weimer Dec. 31, 2017, 7:24 p.m.
GCC PR 83641 results in a miscompilation of libpthread, which
causes pthread_exit not to restore callee-saved registers before
running destructors for objects on the stack.  This test detects
this situation:

info: unsigned int, direct pthread_exit call
tst-thread-exit-clobber.cc:80: numeric comparison failure
   left: 4148288912 (0xf741dd90); from: value
  right: 1600833940 (0x5f6ac994); from: magic_values.v2
info: double, direct pthread_exit call
info: unsigned int, indirect pthread_exit call
info: double, indirect pthread_exit call
error: 1 test failures

2017-12-31  Florian Weimer  <fweimer@redhat.com>

	* nptl/tst-thread-exit-clobber.cc: New file.
	* nptl/Makefile (CFLAGS-tst-thread-exit-clobber.o): Compile in
	C++11 mode.
	(LDLIBS-tst-thread-exit-clobber): Link with libstdc++.
	(tests): Add tst-thread-exit-clobber.
	[!CXX] (tests-unsupported): Add tst-thread-exit-clobber.

Comments

Florian Weimer Jan. 5, 2018, 1:05 p.m. | #1
On 12/31/2017 08:24 PM, Florian Weimer wrote:
> GCC PR 83641 results in a miscompilation of libpthread, which
> causes pthread_exit not to restore callee-saved registers before
> running destructors for objects on the stack.  This test detects
> this situation:
> 
> info: unsigned int, direct pthread_exit call
> tst-thread-exit-clobber.cc:80: numeric comparison failure
>     left: 4148288912 (0xf741dd90); from: value
>    right: 1600833940 (0x5f6ac994); from: magic_values.v2
> info: double, direct pthread_exit call
> info: unsigned int, indirect pthread_exit call
> info: double, indirect pthread_exit call
> error: 1 test failures
> 
> 2017-12-31  Florian Weimer<fweimer@redhat.com>
> 
> 	* nptl/tst-thread-exit-clobber.cc: New file.
> 	* nptl/Makefile (CFLAGS-tst-thread-exit-clobber.o): Compile in
> 	C++11 mode.
> 	(LDLIBS-tst-thread-exit-clobber): Link with libstdc++.
> 	(tests): Add tst-thread-exit-clobber.
> 	[!CXX] (tests-unsupported): Add tst-thread-exit-clobber.

FYI, I will push this once the static_asssert matter is settled.  Thanks.

Florian
Carlos O'Donell Jan. 5, 2018, 6:36 p.m. | #2
On 12/31/2017 11:24 AM, Florian Weimer wrote:
> GCC PR 83641 results in a miscompilation of libpthread, which
> causes pthread_exit not to restore callee-saved registers before
> running destructors for objects on the stack.  This test detects
> this situation:
> 
> info: unsigned int, direct pthread_exit call
> tst-thread-exit-clobber.cc:80: numeric comparison failure
>    left: 4148288912 (0xf741dd90); from: value
>   right: 1600833940 (0x5f6ac994); from: magic_values.v2
> info: double, direct pthread_exit call
> info: unsigned int, indirect pthread_exit call
> info: double, indirect pthread_exit call
> error: 1 test failures
> 
> 2017-12-31  Florian Weimer  <fweimer@redhat.com>
> 
> 	* nptl/tst-thread-exit-clobber.cc: New file.
> 	* nptl/Makefile (CFLAGS-tst-thread-exit-clobber.o): Compile in
> 	C++11 mode.
> 	(LDLIBS-tst-thread-exit-clobber): Link with libstdc++.
> 	(tests): Add tst-thread-exit-clobber.
> 	[!CXX] (tests-unsupported): Add tst-thread-exit-clobber.
> 

OK, with comment expansion in the test case.

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

> +/* These constants are magic values which are used to detect whether
> +   registers have been clobbered.  The idea is that these values are
> +   hidden behind a compiler barrier and only present in .rodata
> +   initially, so that it is less likely that they are in a register by
> +   accident.  */
This comment needs to be expanded to explain *how* the test detects the
registers have been clobbered.
Aurelien Jarno Jan. 9, 2018, 11:47 a.m. | #3
On 2017-12-31 20:24, Florian Weimer wrote:
> GCC PR 83641 results in a miscompilation of libpthread, which
> causes pthread_exit not to restore callee-saved registers before
> running destructors for objects on the stack.  This test detects
> this situation:
> 
> info: unsigned int, direct pthread_exit call
> tst-thread-exit-clobber.cc:80: numeric comparison failure
>    left: 4148288912 (0xf741dd90); from: value
>   right: 1600833940 (0x5f6ac994); from: magic_values.v2
> info: double, direct pthread_exit call
> info: unsigned int, indirect pthread_exit call
> info: double, indirect pthread_exit call
> error: 1 test failures
> 
> 2017-12-31  Florian Weimer  <fweimer@redhat.com>
> 
> 	* nptl/tst-thread-exit-clobber.cc: New file.
> 	* nptl/Makefile (CFLAGS-tst-thread-exit-clobber.o): Compile in
> 	C++11 mode.
> 	(LDLIBS-tst-thread-exit-clobber): Link with libstdc++.
> 	(tests): Add tst-thread-exit-clobber.
> 	[!CXX] (tests-unsupported): Add tst-thread-exit-clobber.
> 

Note that this test fails on mipsel-linux-gnu and mips-linux-gnu with:

| info: unsigned int, direct pthread_exit call
| info: double, direct pthread_exit call
| info: unsigned int, indirect pthread_exit call
| info: double, indirect pthread_exit call
| error: tst-thread-exit-clobber.cc:125: not true: value == magic_values_double.v4
| error: tst-thread-exit-clobber.cc:122: not true: value == magic_values_double.v3
| error: tst-thread-exit-clobber.cc:119: not true: value == magic_values_double.v2
| error: tst-thread-exit-clobber.cc:116: not true: value == magic_values_double.v1
| error: tst-thread-exit-clobber.cc:113: not true: value == magic_values_double.v0
| error: 5 test failures

The test looks correct as a first glance, and I confirm that the values
are corrupted by the ones passed in argument of call_pthread_exit_1.
Therefore it looks like a real issue affecting MIPS.

Aurelien

Patch

diff --git a/nptl/Makefile b/nptl/Makefile
index cf2ba8131b..83ecdc6330 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -229,6 +229,8 @@  CFLAGS-pt-system.c += -fexceptions
 LDLIBS-tst-once5 = -lstdc++
 CFLAGS-tst-thread_local1.o = -std=gnu++11
 LDLIBS-tst-thread_local1 = -lstdc++
+CFLAGS-tst-thread-exit-clobber.o = -std=gnu++11
+LDLIBS-tst-thread-exit-clobber = -lstdc++
 
 tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
 	tst-mutex1 tst-mutex2 tst-mutex3 tst-mutex4 tst-mutex5 tst-mutex6 \
@@ -304,7 +306,8 @@  tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
 			    c89 gnu89 c99 gnu99 c11 gnu11) \
 	tst-bad-schedattr \
 	tst-thread_local1 tst-mutex-errorcheck tst-robust10 \
-	tst-robust-fork tst-create-detached tst-memstream
+	tst-robust-fork tst-create-detached tst-memstream \
+	tst-thread-exit-clobber
 
 tests-internal := tst-rwlock19 tst-rwlock20 \
 		  tst-sem11 tst-sem12 tst-sem13 \
@@ -458,7 +461,7 @@  tests-unsupported += tst-cancel24 tst-cancel24-static tst-once5
 endif
 # These tests require a C++ compiler and runtime with thread_local support.
 ifneq ($(have-cxx-thread_local),yes)
-tests-unsupported += tst-thread_local1
+tests-unsupported += tst-thread_local1 tst-thread-exit-clobber
 endif
 
 include ../Rules
diff --git a/nptl/tst-thread-exit-clobber.cc b/nptl/tst-thread-exit-clobber.cc
new file mode 100644
index 0000000000..95b81f1e80
--- /dev/null
+++ b/nptl/tst-thread-exit-clobber.cc
@@ -0,0 +1,234 @@ 
+/* Test that pthread_exit does not clobber callee-saved registers.
+   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/>.  */
+
+/* This test attempts to check that callee-saved registers are
+   restored to their original values when destructors are run after
+   pthread_exit is called.  GCC PR 83641 causes this test to fail.  */
+
+#include <stdio.h>
+#include <support/check.h>
+#include <support/xthread.h>
+
+/* These constants are magic values which are used to detect whether
+   registers have been clobbered.  The idea is that these values are
+   hidden behind a compiler barrier and only present in .rodata
+   initially, so that it is less likely that they are in a register by
+   accident.  */
+
+template <class T>
+struct values
+{
+  T v0;
+  T v1;
+  T v2;
+  T v3;
+  T v4;
+};
+
+static const values<unsigned int> magic_values =
+  {
+    0x57f7fc72,
+    0xe582daba,
+    0x5f6ac994,
+    0x35efddb7,
+    0x1fbf5a74,
+  };
+
+static const values<double> magic_values_double =
+  {
+    0.6764041905675465,
+    0.9533336788140494,
+    0.6091161359041452,
+    0.7668653957125336,
+    0.010374520235509666,
+  };
+
+/* Special index value which tells check_magic that no check should be
+   performed.  */
+enum { no_check = -1 };
+
+/* Check that VALUE is the magic value for INDEX, behind a compiler
+   barrier.  */
+__attribute__ ((noinline, noclone, weak))
+void
+check_magic (int index, unsigned int value)
+{
+  switch (index)
+    {
+    case 0:
+      TEST_COMPARE (value, magic_values.v0);
+      break;
+    case 1:
+      TEST_COMPARE (value, magic_values.v1);
+      break;
+    case 2:
+      TEST_COMPARE (value, magic_values.v2);
+      break;
+    case 3:
+      TEST_COMPARE (value, magic_values.v3);
+      break;
+    case 4:
+      TEST_COMPARE (value, magic_values.v4);
+      break;
+    case no_check:
+      break;
+    default:
+      FAIL_EXIT1 ("invalid magic value index %d", index);
+    }
+}
+
+/* Check that VALUE is the magic value for INDEX, behind a compiler
+   barrier.  Double variant. */
+__attribute__ ((noinline, noclone, weak))
+void
+check_magic (int index, double value)
+{
+  switch (index)
+    {
+    case 0:
+      TEST_VERIFY (value == magic_values_double.v0);
+      break;
+    case 1:
+      TEST_VERIFY (value == magic_values_double.v1);
+      break;
+    case 2:
+      TEST_VERIFY (value == magic_values_double.v2);
+      break;
+    case 3:
+      TEST_VERIFY (value == magic_values_double.v3);
+      break;
+    case 4:
+      TEST_VERIFY (value == magic_values_double.v4);
+      break;
+    case no_check:
+      break;
+    default:
+      FAIL_EXIT1 ("invalid magic value index %d", index);
+    }
+}
+
+/* Store a magic value and check, via the destructor, that it has the
+   expected value.  */
+template <class T, int I>
+struct checker
+{
+  T value;
+
+  checker (T v)
+    : value (v)
+  {
+  }
+
+  ~checker()
+  {
+    check_magic (I, value);
+  }
+};
+
+/* The functions call_pthread_exit_0, call_pthread_exit_1,
+   call_pthread_exit are used to call pthread_exit indirectly, with
+   the intent of clobbering the register values.  */
+
+__attribute__ ((noinline, noclone, weak))
+void
+call_pthread_exit_0 (const values<unsigned int> *pvalues)
+{
+  checker<unsigned int, no_check> c0 (pvalues->v0);
+  checker<unsigned int, no_check> c1 (pvalues->v1);
+  checker<unsigned int, no_check> c2 (pvalues->v2);
+  checker<unsigned int, no_check> c3 (pvalues->v3);
+  checker<unsigned int, no_check> c4 (pvalues->v4);
+
+  pthread_exit (NULL);
+}
+
+__attribute__ ((noinline, noclone, weak))
+void
+call_pthread_exit_1 (const values<double> *pvalues)
+{
+  checker<double, no_check> c0 (pvalues->v0);
+  checker<double, no_check> c1 (pvalues->v1);
+  checker<double, no_check> c2 (pvalues->v2);
+  checker<double, no_check> c3 (pvalues->v3);
+  checker<double, no_check> c4 (pvalues->v4);
+
+  values<unsigned int> other_values = { 0, };
+  call_pthread_exit_0 (&other_values);
+}
+
+__attribute__ ((noinline, noclone, weak))
+void
+call_pthread_exit ()
+{
+  values<double> other_values = { 0, };
+  call_pthread_exit_1 (&other_values);
+}
+
+/* Create on-stack objects and check that their values are restored by
+   pthread_exit.  If Nested is true, call pthread_exit indirectly via
+   call_pthread_exit.  */
+template <class T, bool Nested>
+__attribute__ ((noinline, noclone, weak))
+void *
+threadfunc (void *closure)
+{
+  const values<T> *pvalues = static_cast<const values<T> *> (closure);
+
+  checker<T, 0> c0 (pvalues->v0);
+  checker<T, 1> c1 (pvalues->v1);
+  checker<T, 2> c2 (pvalues->v2);
+  checker<T, 3> c3 (pvalues->v3);
+  checker<T, 4> c4 (pvalues->v4);
+
+  if (Nested)
+    call_pthread_exit ();
+  else
+    pthread_exit (NULL);
+
+  /* This should not be reached.  */
+  return const_cast<char *> ("");
+}
+
+static int
+do_test ()
+{
+  puts ("info: unsigned int, direct pthread_exit call");
+  pthread_t thr
+    = xpthread_create (NULL, &threadfunc<unsigned int, false>,
+                       const_cast<values<unsigned int> *> (&magic_values));
+  TEST_VERIFY (xpthread_join (thr) == NULL);
+
+  puts ("info: double, direct pthread_exit call");
+  thr = xpthread_create (NULL, &threadfunc<double, false>,
+                         const_cast<values<double> *> (&magic_values_double));
+  TEST_VERIFY (xpthread_join (thr) == NULL);
+
+  puts ("info: unsigned int, indirect pthread_exit call");
+  thr = xpthread_create (NULL, &threadfunc<unsigned int, true>,
+                       const_cast<values<unsigned int> *> (&magic_values));
+  TEST_VERIFY (xpthread_join (thr) == NULL);
+
+  puts ("info: double, indirect pthread_exit call");
+  thr = xpthread_create (NULL, &threadfunc<double, true>,
+                         const_cast<values<double> *> (&magic_values_double));
+  TEST_VERIFY (xpthread_join (thr) == NULL);
+
+  return 0;
+}
+
+#include <support/test-driver.c>