diff mbox

[toplevel/config] fix tls.m4 configure race condition (bootstrap/PR43170)

Message ID 4E347EB0-900D-4A65-BC7A-D04E8B9AB241@sandoe-acoustics.co.uk
State New
Headers show

Commit Message

Iain Sandoe June 8, 2010, 11:06 p.m. UTC 
On 8 Jun 2010, at 20:58, Ralf Wildenhues wrote:

> * IainS wrote on Tue, Jun 08, 2010 at 09:41:45PM CEST:
>> On 8 Jun 2010, at 20:21, Ralf Wildenhues wrote:
>>> * Jakub Jelinek wrote on Tue, Jun 08, 2010 at 09:15:57PM CEST:
>>>> The test is testing for a bug in old glibcs which was violating the
>>>> __thread property, and I believe you really need some optimization
>>>> barriers for the test, otherwise some smart optimization in the
>>>> future will optimize the test away and let it succeed on buggy old
>>>> glibcs.
>>>
>>> Thank you, finally we are getting somewhere!  I think this paragraph
>>> should be put in a comment before the test in tls.m4.
>>
>> as a suggestion:
>>
>> # The 'volatile' qualifier on the references coupled with the  
>> positioning of the
>> # reference to 'a' is necessary to achieve an optimization barrier  
>> for compilers
>> # using whole program or other advanced optimizations.
>
> Well, if it doesn't mention that this is really about an old glibc bug
> which caused the __thread property to be violated, then I would never
> understand why the test is written the way it is.  The glibc bug is  
> the
> crucial bit of information; the need to work around compiler
> optimization is only to ensure the test is effective in two  
> directions.
> I'd suggest something like:
>
>  dnl Test for an old glibc bug that violated the __thread property.
>  dnl Use volatile to ensure the compiler won't optimize away pointer
>  dnl accesses it might otherwise know to be different, or reorder them
>  dnl and reuse storage, which may let them point to the same location.

updated patch attached, please check the final wording of the comment  
is OK.

bootstrapped on i686-darwin9, x86_64-darwin10.
Confirmed that libgomp links emutls, that the tls tests give the same  
results across stage1-3,
that the tls-test fragment runs consistently for 10,000 iterations   
and that the testsuites for
  tls.exp and libgomp run as expected.

Obviously it would be nice to collect a result from another emutls  
target - and also it seems that this fix would clear
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42829,  Kai do you agree?

OK now for trunk, and 4.5 ?

Iain.


config/ChangeLog:
	
	PR bootstrap/43170
	* tls.m4 (GCC_CHECK_TLS): Add volatile qualifier to test references.
	Move main () reference ahead of thread_create().  Add comment to
	explain the requirements of the test.

libgomp/ChangeLog:
	
	PR bootstrap/43170
	* configure: Regenerated.

libstdc++-v3/ChangeLog:

	PR bootstrap/43170
	* configure: Regenerated.

libmudflap/ChangeLog:

	PR bootstrap/43170
	* configure: Regenerated.

libjava/ChangeLog:

	PR bootstrap/43170
	* configure: Regenerated.

Comments

Paolo Bonzini June 9, 2010, 6:12 a.m. UTC | #1 
On 06/09/2010 01:06 AM, IainS wrote:
>
> bootstrapped on i686-darwin9, x86_64-darwin10. Confirmed that libgomp
> links emutls, that the tls tests give the same results across
> stage1-3, that the tls-test fragment runs consistently for 10,000
> iterations  and that the testsuites for tls.exp and libgomp run as
> expected.

Ok for trunk and 4.5.

Paolo
Iain Sandoe June 9, 2010, 9:28 a.m. UTC | #2 
On 9 Jun 2010, at 07:12, Paolo Bonzini wrote:

> On 06/09/2010 01:06 AM, IainS wrote:
>>
>> bootstrapped on i686-darwin9, x86_64-darwin10. Confirmed that libgomp
>> links emutls, that the tls tests give the same results across
>> stage1-3, that the tls-test fragment runs consistently for 10,000
>> iterations  and that the testsuites for tls.exp and libgomp run as
>> expected.
>
> Ok for trunk and 4.5.

r160457,
I'll take you up on your kind offer to push to src.
I've tested on my local 4.5 tree but will leave the ci until tomorrow.
Iain
Kai Tietz June 9, 2010, 4:43 p.m. UTC | #3 
Hi Iain,

yes, PRs 42829 and 42828 are fixed by your patches, too.

Thanks,
Kai
NightStrike June 10, 2010, 3:57 p.m. UTC | #4 
On Wed, Jun 9, 2010 at 12:43 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
> Hi Iain,
>
> yes, PRs 42829 and 42828 are fixed by your patches, too.

Those PRs are still listed as open.
Paolo Bonzini June 10, 2010, 4:10 p.m. UTC | #5 
On 06/10/2010 05:57 PM, NightStrike wrote:
> On Wed, Jun 9, 2010 at 12:43 PM, Kai Tietz<ktietz70@googlemail.com>  wrote:
>> Hi Iain,
>>
>> yes, PRs 42829 and 42828 are fixed by your patches, too.
>
> Those PRs are still listed as open.

Patch committed to src, BTW.

Paolo
Iain Sandoe June 10, 2010, 4:17 p.m. UTC | #6 
On 10 Jun 2010, at 16:57, NightStrike wrote:

> On Wed, Jun 9, 2010 at 12:43 PM, Kai Tietz <ktietz70@googlemail.com>  
> wrote:
>> Hi Iain,
>>
>> yes, PRs 42829 and 42828 are fixed by your patches, too.
>
> Those PRs are still listed as open.

I will close the three PRs tomorrow after the applied patches (for  
trunk and 4.5) have settled down.
cheers,
Iain
diff mbox

Patch

Index: libgomp/configure
===================================================================
--- libgomp/configure	(revision 160446)
+++ libgomp/configure	(working copy)
@@ -15380,7 +15380,7 @@  else
 /* end confdefs.h.  */
 #include <pthread.h>
 		__thread int a;
-		static int *a_in_other_thread;
+		static int *volatile a_in_other_thread;
 		static void *
 		thread_func (void *arg)
 		{
@@ -15392,11 +15392,11 @@  main ()
 {
 pthread_t thread;
 		void *thread_retval;
-		int *a_in_main_thread;
+		int *volatile a_in_main_thread;
+		a_in_main_thread = &a;
 		if (pthread_create (&thread, (pthread_attr_t *)0,
 				    thread_func, (void *)0))
 		  return 0;
-		a_in_main_thread = &a;
 		if (pthread_join (thread, &thread_retval))
 		  return 0;
 		return (a_in_other_thread == a_in_main_thread);
Index: libjava/configure
===================================================================
--- libjava/configure	(revision 160446)
+++ libjava/configure	(working copy)
@@ -24390,7 +24390,7 @@  else
 /* end confdefs.h.  */
 #include <pthread.h>
 		__thread int a;
-		static int *a_in_other_thread;
+		static int *volatile a_in_other_thread;
 		static void *
 		thread_func (void *arg)
 		{
@@ -24402,11 +24402,11 @@  main ()
 {
 pthread_t thread;
 		void *thread_retval;
-		int *a_in_main_thread;
+		int *volatile a_in_main_thread;
+		a_in_main_thread = &a;
 		if (pthread_create (&thread, (pthread_attr_t *)0,
 				    thread_func, (void *)0))
 		  return 0;
-		a_in_main_thread = &a;
 		if (pthread_join (thread, &thread_retval))
 		  return 0;
 		return (a_in_other_thread == a_in_main_thread);
Index: libmudflap/configure
===================================================================
--- libmudflap/configure	(revision 160446)
+++ libmudflap/configure	(working copy)
@@ -11479,7 +11479,7 @@  else
 /* end confdefs.h.  */
 #include <pthread.h>
 		__thread int a;
-		static int *a_in_other_thread;
+		static int *volatile a_in_other_thread;
 		static void *
 		thread_func (void *arg)
 		{
@@ -11491,11 +11491,11 @@  main ()
 {
 pthread_t thread;
 		void *thread_retval;
-		int *a_in_main_thread;
+		int *volatile a_in_main_thread;
+		a_in_main_thread = &a;
 		if (pthread_create (&thread, (pthread_attr_t *)0,
 				    thread_func, (void *)0))
 		  return 0;
-		a_in_main_thread = &a;
 		if (pthread_join (thread, &thread_retval))
 		  return 0;
 		return (a_in_other_thread == a_in_main_thread);
Index: libstdc++-v3/configure
===================================================================
--- libstdc++-v3/configure	(revision 160446)
+++ libstdc++-v3/configure	(working copy)
@@ -25548,7 +25548,7 @@  else
 /* end confdefs.h.  */
 #include <pthread.h>
 		__thread int a;
-		static int *a_in_other_thread;
+		static int *volatile a_in_other_thread;
 		static void *
 		thread_func (void *arg)
 		{
@@ -25560,11 +25560,11 @@  main ()
 {
 pthread_t thread;
 		void *thread_retval;
-		int *a_in_main_thread;
+		int *volatile a_in_main_thread;
+		a_in_main_thread = &a;
 		if (pthread_create (&thread, (pthread_attr_t *)0,
 				    thread_func, (void *)0))
 		  return 0;
-		a_in_main_thread = &a;
 		if (pthread_join (thread, &thread_retval))
 		  return 0;
 		return (a_in_other_thread == a_in_main_thread);
@@ -44486,7 +44486,7 @@  else
 /* end confdefs.h.  */
 #include <pthread.h>
 		__thread int a;
-		static int *a_in_other_thread;
+		static int *volatile a_in_other_thread;
 		static void *
 		thread_func (void *arg)
 		{
@@ -44498,11 +44498,11 @@  main ()
 {
 pthread_t thread;
 		void *thread_retval;
-		int *a_in_main_thread;
+		int *volatile a_in_main_thread;
+		a_in_main_thread = &a;
 		if (pthread_create (&thread, (pthread_attr_t *)0,
 				    thread_func, (void *)0))
 		  return 0;
-		a_in_main_thread = &a;
 		if (pthread_join (thread, &thread_retval))
 		  return 0;
 		return (a_in_other_thread == a_in_main_thread);
@@ -50571,7 +50571,7 @@  else
 /* end confdefs.h.  */
 #include <pthread.h>
 		__thread int a;
-		static int *a_in_other_thread;
+		static int *volatile a_in_other_thread;
 		static void *
 		thread_func (void *arg)
 		{
@@ -50583,11 +50583,11 @@  main ()
 {
 pthread_t thread;
 		void *thread_retval;
-		int *a_in_main_thread;
+		int *volatile a_in_main_thread;
+		a_in_main_thread = &a;
 		if (pthread_create (&thread, (pthread_attr_t *)0,
 				    thread_func, (void *)0))
 		  return 0;
-		a_in_main_thread = &a;
 		if (pthread_join (thread, &thread_retval))
 		  return 0;
 		return (a_in_other_thread == a_in_main_thread);
Index: config/tls.m4
===================================================================
--- config/tls.m4	(revision 160446)
+++ config/tls.m4	(working copy)
@@ -38,11 +38,16 @@  AC_DEFUN([GCC_CHECK_TLS], [
 	CFLAGS="$chktls_save_CFLAGS"
 	if test "X$thread_CFLAGS" != Xfailed; then
 	  CFLAGS="$thread_CFLAGS $chktls_save_CFLAGS"
+ 	  dnl Test for an old glibc bug that violated the __thread property.
+	  dnl Use volatile to ensure the compiler won't optimize away pointer
+	  dnl accesses it might otherwise assume to be redundant, or reorder 
+	  dnl them and reuse storage, which might lead to them pointing to
+	  dnl the same location.
 	  AC_RUN_IFELSE(
 	    [AC_LANG_PROGRAM(
 	       [#include <pthread.h>
 		__thread int a;
-		static int *a_in_other_thread;
+		static int *volatile a_in_other_thread;
 		static void *
 		thread_func (void *arg)
 		{
@@ -51,11 +56,11 @@  AC_DEFUN([GCC_CHECK_TLS], [
 		}],
 	       [pthread_t thread;
 		void *thread_retval;
-		int *a_in_main_thread;
+		int *volatile a_in_main_thread;
+		a_in_main_thread = &a;
 		if (pthread_create (&thread, (pthread_attr_t *)0,
 				    thread_func, (void *)0))
 		  return 0;
-		a_in_main_thread = &a;
 		if (pthread_join (thread, &thread_retval))
 		  return 0;
 		return (a_in_other_thread == a_in_main_thread);])],