Re: [PATCH] Add test case for C++11 thread_local support

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

On 10/05/2015 11:50 AM, Florian Weimer wrote:
> We did not have a test case which tests the thread_local support
> functionality with the C++ compiler.
>
> I tested this on Fedora 22 (x86_64), Red Hat Enterprise Linux 7 (ppc64),
> and Debian wheezy (x86_64, after commenting out AVX512 support in the
> dynamic linker).  The latter correctly marks the test case as UNSUPPORTED.

* Why isn't the C++ compiler testsuite sufficient?

  Is there any reason to have this test in glibc? In a normal bootstrap you'd
  build glibc, you'd test thread_local destructor support via tst-tls-atexit
  (the part implemented by glibc), and then you'd go on to build the full
  set of languages in the compiler and test C++ thread_local support more fully.
  Why do we want to test this here?  

* Testing glibc thread_local destructor support?

  If glibc thread_local support is broken, then libstdc++-v3/configure won't
  detect __cxa_thread_atexit_impl and libstdc++-v3/libsupc++/atexit_thread.cc
  is built with alternate compiler-specific support for destructors. Similarly
  if the compiler you're using was built with old-enough glibc to lack that support.

  Therefore a pre-requisite of this test is that it must know that the existing
  C++ compiler is built to use glibc's __cxa_thread_atexit_impl, otherwise you're
  testing libsupc++'s support for thread_local destructors and the test has nothing
  to do with glibc? 

* Had a quibble with a comment in nptl/Makefile, see below.

Some top-level process nits:

* Don't include ChangeLog in diff.

  Put it into your email at the end please. I promise we'll get to the point
  where it's autogenerated and you won't have to care about it (same with NEWS).

* Don't include regenerated file changes in diff.

  They make it harder to read the patch quickly, and are not needed.
  The committer has to regenerate the file and review what they checkin.

> 0001-Add-a-test-case-for-C-11-thread_local-support.patch
> 
> 
> From b2d51b5b9152d841f3476d7d5588ce302dad1853 Mon Sep 17 00:00:00 2001
> Message-Id: <b2d51b5b9152d841f3476d7d5588ce302dad1853.1444060169.git.fweimer@redhat.com>
> From: Florian Weimer <fweimer@redhat.com>
> Date: Mon, 5 Oct 2015 17:48:12 +0200
> Subject: [PATCH] Add a test case for C++11 thread_local support
> To: libc-alpha@sourceware.org
> 
> This requires a C++ compiler with thread_local support, and a new
> configure check is needed.

OK.

> ---
>  ChangeLog                 |  12 +++
>  config.make.in            |   1 +
>  configure                 |  47 +++++++++++
>  configure.ac              |  25 ++++++
>  nptl/Makefile             |   9 ++-
>  nptl/tst-thread_local1.cc | 200 ++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 293 insertions(+), 1 deletion(-)
>  create mode 100644 nptl/tst-thread_local1.cc
> 
> diff --git a/ChangeLog b/ChangeLog
> index 0bfe2bb..804a2a1 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,15 @@
> +2015-10-05  Florian Weimer  <fweimer@redhat.com>
> +
> +	* configure.ac (libc_cv_cxx_thread_local): Define.
> +	* configure: Regenerate.
> +	* config.make.in (have-cxx-thread_local): Define.
> +	* nptl/Makefile (CFLAGS-tst-thread_local1.o):
> +	(LDLIBS-tst-thread_local1): Define.
> +	(tests): Add tst-thread_local1.
> +	[have-cxx-thread_local != yes] (tests-unsupported): Add
> +	tst-thread_local1.
> +	* nptl/tst-thread_local1.cc: New file.
> +
>  2015-10-03  Paul Pluzhnikov  <ppluzhnikov@google.com>
>  
>  	* sysdeps/x86_64/fpu/libm-test-ulps: Regenerated.
> diff --git a/config.make.in b/config.make.in
> index bea371d..839d86f 100644
> --- a/config.make.in
> +++ b/config.make.in
> @@ -68,6 +68,7 @@ bind-now = @bindnow@
>  have-hash-style = @libc_cv_hashstyle@
>  use-default-link = @use_default_link@
>  output-format = @libc_cv_output_format@
> +have-cxx-thread_local = @libc_cv_cxx_thread_local@

OK.
  
>  static-libgcc = @libc_cv_gcc_static_libgcc@
>  

[cut diff of configure]

> diff --git a/configure.ac b/configure.ac
> index 95d700e..68e6777 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1982,6 +1982,31 @@ if test $libc_cv_builtin_trap = yes; then
>    AC_DEFINE([HAVE_BUILTIN_TRAP])
>  fi
>  
> +dnl C++ feature tests.
> +AC_LANG_PUSH([C++])
> +
> +AC_CACHE_CHECK([whether the C++ compiler supports thread_local],
> +	       libc_cv_cxx_thread_local, [
> +old_CXXFLAGS="$CXXFLAGS"
> +CXXFLAGS="$CXXFLAGS -std=gnu++11"
> +AC_COMPILE_IFELSE([AC_LANG_SOURCE([
> +struct S
> +{
> +  S ();
> +  ~S ();
> +};
> +thread_local S s;
> +S * get () { return &s; }
> +])],
> +	       [libc_cv_cxx_thread_local=yes],
> +	       [libc_cv_cxx_thread_local=no])
> +CXXFLAGS="$old_CXXFLAGS"
> +])
> +AC_SUBST(libc_cv_cxx_thread_local)
> +
> +AC_LANG_POP([C++])
> +dnl End of C++ feature tests.

OK.

> +
>  ### End of automated tests.
>  ### Now run sysdeps configure fragments.
>  
> diff --git a/nptl/Makefile b/nptl/Makefile
> index aaca0a4..b27bef1 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -212,6 +212,8 @@ CFLAGS-recvfrom.c = -fexceptions -fasynchronous-unwind-tables
>  CFLAGS-pt-system.c = -fexceptions
>  
>  LDLIBS-tst-once5 = -lstdc++
> +CFLAGS-tst-thread_local1.o = -std=gnu++11
> +LDLIBS-tst-thread_local1 = -lstdc++

OK.

>  
>  tests = tst-typesizes \
>  	tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
> @@ -283,7 +285,8 @@ tests = tst-typesizes \
>  	tst-getpid3 \
>  	tst-setuid3 \
>  	tst-initializers1 $(addprefix tst-initializers1-,c89 gnu89 c99 gnu99) \
> -	tst-bad-schedattr
> +	tst-bad-schedattr \
> +	tst-thread_local1

OK.

>  xtests = tst-setuid1 tst-setuid1-static tst-setuid2 \
>  	tst-mutexpp1 tst-mutexpp6 tst-mutexpp10
>  test-srcs = tst-oddstacklimit
> @@ -403,6 +406,10 @@ ifeq (,$(CXX))
>  # These tests require a C++ compiler and runtime.
>  tests-unsupported += tst-cancel24 tst-cancel24-static tst-once5
>  endif
> +# These tests require a C++ compiler with thread_local support.

This should say:
# These tests require a C++ compiler and runtime with thread_local support.

> +ifneq ($(have-cxx-thread_local),yes)
> +tests-unsupported += tst-thread_local1
> +endif
>  
>  include ../Rules
>  
> diff --git a/nptl/tst-thread_local1.cc b/nptl/tst-thread_local1.cc
> new file mode 100644
> index 0000000..65d374b
> --- /dev/null
> +++ b/nptl/tst-thread_local1.cc
> @@ -0,0 +1,200 @@
> +/* Test basic thread_local support.

It's a fine test, but I'm not sure why this needs to be in glibc and not
the libstdc++ testsuite. I count 31 thread_local related tests in gcc,
with 27 being specifically about thread_local.

> +   Copyright (C) 2015 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +   Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
> +
> +   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 <pthread.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include <functional>
> +#include <string>
> +#include <thread>
> +
> +struct counter
> +{
> +  int constructed {};
> +  int destructed {};
> +
> +  void reset ();
> +};
> +
> +void
> +counter::reset ()
> +{
> +  constructed = 0;
> +  destructed = 0;
> +}
> +
> +static std::string
> +to_string (const counter &c)
> +{
> +  char buf[128];
> +  snprintf (buf, sizeof (buf), "%d/%d",
> +            c.constructed, c.destructed);
> +  return buf;
> +}
> +
> +template <counter *Counter>
> +struct counting
> +{
> +  counting () __attribute__ ((noinline, noclone));
> +  ~counting () __attribute__ ((noinline, noclone));
> +  void operation () __attribute__ ((noinline, noclone));
> +};
> +
> +template<counter *Counter>
> +__attribute__ ((noinline, noclone))
> +counting<Counter>::counting ()
> +{
> +  ++Counter->constructed;
> +}
> +
> +template<counter *Counter>
> +__attribute__ ((noinline, noclone))
> +counting<Counter>::~counting ()
> +{
> +  ++Counter->destructed;
> +}
> +
> +template<counter *Counter>
> +void __attribute__ ((noinline, noclone))
> +counting<Counter>::operation ()
> +{
> +  // Optimization barrier.
> +  asm ("");
> +}
> +
> +static counter counter_static;
> +static counter counter_anonymous_namespace;
> +static counter counter_extern;
> +static counter counter_function_local;
> +static bool errors (false);
> +
> +static std::string
> +all_counters ()
> +{
> +  return to_string (counter_static)
> +    + ' ' + to_string (counter_anonymous_namespace)
> +    + ' ' + to_string (counter_extern)
> +    + ' ' + to_string (counter_function_local);
> +}
> +
> +static void
> +check_counters (const char *name, const char *expected)
> +{
> +  std::string actual{all_counters ()};
> +  if (actual != expected)
> +    {
> +      printf ("error: %s: (%s) != (%s)\n",
> +              name, actual.c_str (), expected);
> +      errors = true;
> +    }
> +}
> +
> +static void
> +reset_all ()
> +{
> +  counter_static.reset ();
> +  counter_anonymous_namespace.reset ();
> +  counter_extern.reset ();
> +  counter_function_local.reset ();
> +}
> +
> +static thread_local counting<&counter_static> counting_static;
> +namespace {
> +  thread_local counting<&counter_anonymous_namespace>
> +    counting_anonymous_namespace;
> +}
> +extern thread_local counting<&counter_extern> counting_extern;
> +thread_local counting<&counter_extern> counting_extern;
> +
> +static void *
> +thread_without_access (void *)
> +{
> +  return nullptr;
> +}
> +
> +static void *
> +thread_with_access (void *)
> +{
> +  thread_local counting<&counter_function_local> counting_function_local;
> +  counting_function_local.operation ();
> +  check_counters ("early in thread_with_access", "0/0 0/0 0/0 1/0");
> +  counting_static.operation ();
> +  counting_anonymous_namespace.operation ();
> +  counting_extern.operation ();
> +  check_counters ("in thread_with_access", "1/0 1/0 1/0 1/0");
> +  return nullptr;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  std::function<void (void *(void *))> do_pthread =
> +    [](void *(func) (void *))
> +    {
> +      pthread_t thr;
> +      int ret = pthread_create (&thr, nullptr, func, nullptr);
> +      if (ret != 0)
> +        {
> +          errno = ret;
> +          printf ("error: pthread_create: %m\n");
> +          errors = true;
> +          return;
> +        }
> +      ret = pthread_join (thr, nullptr);
> +      if (ret != 0)
> +        {
> +          errno = ret;
> +          printf ("error: pthread_join: %m\n");
> +          errors = true;
> +          return;
> +        }
> +    };
> +  std::function<void (void *(void *))> do_std_thread =
> +    [](void *(func) (void *))
> +    {
> +      std::thread thr{[func] {func (nullptr);}};
> +      thr.join ();
> +    };
> +
> +  std::array<std::pair<const char *, std::function<void (void *(void *))>>, 2>
> +    do_thread_X
> +      {{
> +        {"pthread_create", do_pthread},
> +        {"std::thread", do_std_thread},
> +      }};
> +
> +  for (auto do_thread : do_thread_X)
> +    {
> +      printf ("info: testing %s\n", do_thread.first);
> +      check_counters ("initial", "0/0 0/0 0/0 0/0");
> +      do_thread.second (thread_without_access);
> +      check_counters ("after thread_without_access", "0/0 0/0 0/0 0/0");
> +      reset_all ();
> +      do_thread.second (thread_with_access);
> +      check_counters ("after thread_with_access", "1/1 1/1 1/1 1/1");
> +      reset_all ();
> +    }
> +
> +  return errors;
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> -- 2.4.3

Cheers,
Carlos.