diff mbox series

[committed] libstdc++: Fix std::jthread assertion and re-enable skipped test

Message ID YKKkNivhziCQks35@redhat.com
State New
Headers show
Series [committed] libstdc++: Fix std::jthread assertion and re-enable skipped test | expand

Commit Message

Jonathan Wakely May 17, 2021, 5:13 p.m. UTC 
libstdc++-v3/ChangeLog:

	* include/std/thread (jthread::_S_create): Fix static assert
	message.
	* testsuite/30_threads/jthread/95989.cc: Re-enable test.
	* testsuite/30_threads/jthread/jthread.cc: Do not require
	pthread effective target.
	* testsuite/30_threads/jthread/2.cc: Moved to...
	* testsuite/30_threads/jthread/version.cc: ...here.

Tested powerpc64le-linux. Committed to trunk.

Let's see if this test is actually fixed, or if it still causes
failures on some targets.
commit 60a156ae53e976dfe44689f7c89e607596e7cf67
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Mon May 17 14:55:22 2021

    libstdc++: Fix std::jthread assertion and re-enable skipped test
    
    libstdc++-v3/ChangeLog:
    
            * include/std/thread (jthread::_S_create): Fix static assert
            message.
            * testsuite/30_threads/jthread/95989.cc: Re-enable test.
            * testsuite/30_threads/jthread/jthread.cc: Do not require
            pthread effective target.
            * testsuite/30_threads/jthread/2.cc: Moved to...
            * testsuite/30_threads/jthread/version.cc: ...here.

Comments

Bernd Edlinger May 18, 2021, 11:55 a.m. UTC | #1 
On 5/17/21 7:13 PM, Jonathan Wakely via Gcc-patches wrote:
> libstdc++-v3/ChangeLog:
> 
> 	* include/std/thread (jthread::_S_create): Fix static assert
> 	message.
> 	* testsuite/30_threads/jthread/95989.cc: Re-enable test.
> 	* testsuite/30_threads/jthread/jthread.cc: Do not require
> 	pthread effective target.
> 	* testsuite/30_threads/jthread/2.cc: Moved to...
> 	* testsuite/30_threads/jthread/version.cc: ...here.
> 
> Tested powerpc64le-linux. Committed to trunk.
> 
> Let's see if this test is actually fixed, or if it still causes
> failures on some targets.
> 
> 

Yes, indeed it is failing on x86_64-pc-linux-gnu.


Bernd.
Bernd Edlinger May 18, 2021, 11:58 a.m. UTC | #2 
On 5/18/21 1:55 PM, Bernd Edlinger wrote:
> On 5/17/21 7:13 PM, Jonathan Wakely via Gcc-patches wrote:
>> libstdc++-v3/ChangeLog:
>>
>> 	* include/std/thread (jthread::_S_create): Fix static assert
>> 	message.
>> 	* testsuite/30_threads/jthread/95989.cc: Re-enable test.
>> 	* testsuite/30_threads/jthread/jthread.cc: Do not require
>> 	pthread effective target.
>> 	* testsuite/30_threads/jthread/2.cc: Moved to...
>> 	* testsuite/30_threads/jthread/version.cc: ...here.
>>
>> Tested powerpc64le-linux. Committed to trunk.
>>
>> Let's see if this test is actually fixed, or if it still causes
>> failures on some targets.
>>
>>
> 
> Yes, indeed it is failing on x86_64-pc-linux-gnu.
> 

that means only this one:

FAIL: 30_threads/jthread/95989.cc execution test

> 
> Bernd.
>
Jonathan Wakely May 19, 2021, 1:27 p.m. UTC | #3 
On 18/05/21 13:58 +0200, Bernd Edlinger wrote:
>On 5/18/21 1:55 PM, Bernd Edlinger wrote:
>> On 5/17/21 7:13 PM, Jonathan Wakely via Gcc-patches wrote:
>>> libstdc++-v3/ChangeLog:
>>>
>>> 	* include/std/thread (jthread::_S_create): Fix static assert
>>> 	message.
>>> 	* testsuite/30_threads/jthread/95989.cc: Re-enable test.
>>> 	* testsuite/30_threads/jthread/jthread.cc: Do not require
>>> 	pthread effective target.
>>> 	* testsuite/30_threads/jthread/2.cc: Moved to...
>>> 	* testsuite/30_threads/jthread/version.cc: ...here.
>>>
>>> Tested powerpc64le-linux. Committed to trunk.
>>>
>>> Let's see if this test is actually fixed, or if it still causes
>>> failures on some targets.
>>>
>>>
>>
>> Yes, indeed it is failing on x86_64-pc-linux-gnu.
>>
>
>that means only this one:
>
>FAIL: 30_threads/jthread/95989.cc execution test

What's your glibc version?
Bernd Edlinger May 19, 2021, 1:37 p.m. UTC | #4 
On 5/19/21 3:27 PM, Jonathan Wakely wrote:
> On 18/05/21 13:58 +0200, Bernd Edlinger wrote:
>> On 5/18/21 1:55 PM, Bernd Edlinger wrote:
>>> On 5/17/21 7:13 PM, Jonathan Wakely via Gcc-patches wrote:
>>>> libstdc++-v3/ChangeLog:
>>>>
>>>>     * include/std/thread (jthread::_S_create): Fix static assert
>>>>     message.
>>>>     * testsuite/30_threads/jthread/95989.cc: Re-enable test.
>>>>     * testsuite/30_threads/jthread/jthread.cc: Do not require
>>>>     pthread effective target.
>>>>     * testsuite/30_threads/jthread/2.cc: Moved to...
>>>>     * testsuite/30_threads/jthread/version.cc: ...here.
>>>>
>>>> Tested powerpc64le-linux. Committed to trunk.
>>>>
>>>> Let's see if this test is actually fixed, or if it still causes
>>>> failures on some targets.
>>>>
>>>>
>>>
>>> Yes, indeed it is failing on x86_64-pc-linux-gnu.
>>>
>>
>> that means only this one:
>>
>> FAIL: 30_threads/jthread/95989.cc execution test
> 
> What's your glibc version?
> 
> 

that is ubuntu 20.04 with latest patches:

$ ldd --version
ldd (Ubuntu GLIBC 2.31-0ubuntu9.3) 2.31
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Written by Roland McGrath and Ulrich Drepper.


Bernd.
Bernd Edlinger May 27, 2021, 12:10 p.m. UTC | #5 
On 5/19/21 3:37 PM, Bernd Edlinger wrote:
> On 5/19/21 3:27 PM, Jonathan Wakely wrote:
>> On 18/05/21 13:58 +0200, Bernd Edlinger wrote:
>>> On 5/18/21 1:55 PM, Bernd Edlinger wrote:
>>>> On 5/17/21 7:13 PM, Jonathan Wakely via Gcc-patches wrote:
>>>>> libstdc++-v3/ChangeLog:
>>>>>
>>>>>     * include/std/thread (jthread::_S_create): Fix static assert
>>>>>     message.
>>>>>     * testsuite/30_threads/jthread/95989.cc: Re-enable test.
>>>>>     * testsuite/30_threads/jthread/jthread.cc: Do not require
>>>>>     pthread effective target.
>>>>>     * testsuite/30_threads/jthread/2.cc: Moved to...
>>>>>     * testsuite/30_threads/jthread/version.cc: ...here.
>>>>>
>>>>> Tested powerpc64le-linux. Committed to trunk.
>>>>>
>>>>> Let's see if this test is actually fixed, or if it still causes
>>>>> failures on some targets.
>>>>>
>>>>>
>>>>
>>>> Yes, indeed it is failing on x86_64-pc-linux-gnu.
>>>>
>>>
>>> that means only this one:
>>>
>>> FAIL: 30_threads/jthread/95989.cc execution test
>>
>> What's your glibc version?
>>
>>
> 
> that is ubuntu 20.04 with latest patches:
> 
> $ ldd --version
> ldd (Ubuntu GLIBC 2.31-0ubuntu9.3) 2.31
> Copyright (C) 2020 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions.  There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> Written by Roland McGrath and Ulrich Drepper.
> 

The glibc version is probably not the reason,
I've got the same effect with Ubuntu EGLIBC 2.19-0ubuntu6.15 as well.

But I use binutils-2.36.1.

I think the problem is here:

libstdc++-v3/src/c++11/thread.cc:      __e = __gthread_join(_M_id._M_thread, 0);

which calls

libgcc/gthr-posix.h:  return __gthrw_(pthread_join) (__threadid, __value_ptr);

but this is only a weak reference to pthread_join:

669	  return __gthrw_(pthread_join) (__threadid, __value_ptr);
(gdb) disass
Dump of assembler code for function std::thread::join():
   0x0000000000405e20 <+0>:	endbr64 
   0x0000000000405e24 <+4>:	push   %rbx
   0x0000000000405e25 <+5>:	mov    %rdi,%rbx
   0x0000000000405e28 <+8>:	mov    (%rdi),%rdi
   0x0000000000405e2b <+11>:	test   %rdi,%rdi
   0x0000000000405e2e <+14>:	je     0x405e44 <std::thread::join()+36>
=> 0x0000000000405e30 <+16>:	xor    %esi,%esi
   0x0000000000405e32 <+18>:	callq  0x0
   0x0000000000405e37 <+23>:	test   %eax,%eax
   0x0000000000405e39 <+25>:	jne    0x405e49 <std::thread::join()+41>
   0x0000000000405e3b <+27>:	movq   $0x0,(%rbx)
   0x0000000000405e42 <+34>:	pop    %rbx
   0x0000000000405e43 <+35>:	retq   
   0x0000000000405e44 <+36>:	mov    $0x16,%eax
   0x0000000000405e49 <+41>:	mov    %eax,%edi
   0x0000000000405e4b <+43>:	callq  0x4012a9 <std::__throw_system_error(int)>
End of assembler dump.

but the pthread_join symbol is not checked for potentially being null.

The test case passes if I change this:

diff --git a/libstdc++-v3/testsuite/30_threads/jthread/95989.cc b/libstdc++-v3/testsuite/30_threads/jthread/95989.cc
index fb3f43bc722..61a3ff76aa1 100644
--- a/libstdc++-v3/testsuite/30_threads/jthread/95989.cc
+++ b/libstdc++-v3/testsuite/30_threads/jthread/95989.cc
@@ -52,4 +52,5 @@ main()
   test01();
   test02();
   test03();
+  pthread_join(NULL, NULL);
 }


Thanks
Bernd.
diff mbox series

Patch

diff --git a/libstdc++-v3/include/std/thread b/libstdc++-v3/include/std/thread
index 886994c1320..f51392ab42c 100644
--- a/libstdc++-v3/include/std/thread
+++ b/libstdc++-v3/include/std/thread
@@ -219,7 +219,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  {
 	    static_assert(is_invocable_v<decay_t<_Callable>,
 					 decay_t<_Args>...>,
-			  "std::thread arguments must be invocable after"
+			  "std::jthread arguments must be invocable after"
 			  " conversion to rvalues");
 	    return thread{std::forward<_Callable>(__f),
 			  std::forward<_Args>(__args)...};
diff --git a/libstdc++-v3/testsuite/30_threads/jthread/95989.cc b/libstdc++-v3/testsuite/30_threads/jthread/95989.cc
index 53f90827f2e..fb3f43bc722 100644
--- a/libstdc++-v3/testsuite/30_threads/jthread/95989.cc
+++ b/libstdc++-v3/testsuite/30_threads/jthread/95989.cc
@@ -20,7 +20,6 @@ 
 // { dg-require-gthreads {} }
 // { dg-additional-options "-pthread" { target pthread } }
 // { dg-additional-options "-static" { target static } }
-// { dg-skip-if "broken" { *-*-* } }
 
 #include <thread>
 
diff --git a/libstdc++-v3/testsuite/30_threads/jthread/jthread.cc b/libstdc++-v3/testsuite/30_threads/jthread/jthread.cc
index 6adc4981175..799787088ac 100644
--- a/libstdc++-v3/testsuite/30_threads/jthread/jthread.cc
+++ b/libstdc++-v3/testsuite/30_threads/jthread/jthread.cc
@@ -16,9 +16,9 @@ 
 // <http://www.gnu.org/licenses/>.
 
 // { dg-options "-std=gnu++2a -pthread" }
-// { dg-add-options libatomic }
 // { dg-do run { target c++2a } }
-// { dg-require-effective-target pthread }
+// { dg-add-options libatomic }
+// { dg-additional-options "-pthread" { target pthread } }
 // { dg-require-gthreads "" }
 
 #include <thread>