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

[Florian Weimer <fweimer at redhat dot com>]

On 10/05/2015 08:19 PM, Carlos O'Donell wrote:
> 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?  

I want to prevent regressions due to glibc changes.

This test case covers only the basics, we also need to check interaction
with other termination callback facilities in glibc (atexit, POSIX
thread-local storage), and the interaction with cancellation and
dlclose.  We also need white-box testing for memory allocation failures.
 I think the dependency on thread_local inside glibc is harmless against
all the baggage these tests would pull into libstdc++.

This test is just a start.  Basically, this test is the minimal version
I was sufficiently confident would actually pass testing.

> * 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?

Hmm, you are right.  It is actually a libstdc++ matter.  I guess I can
work around that by interposing __cxxabiv1::__cxa_thread_atexit.  So
maybe we should run this test twice, once as it is now, and once with
the interposed symbol.

Or we can change the fallback implementation of
__cxxabiv1::__cxa_thread_atexit in libstdc++ to check a weak symbol
__cxxabiv1::__cxa_thread_atexit_impl first and use that if it is
available.  Maybe this is useful on its own?

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

I've been told before that either way is fine. :-/

I can certainly script something to extract the changelog and place it
outside the diff.

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

Fixed.

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

See above.

Florian