PowerPC: Define __PTHREAD_MUTEX_HAVE_ELISION to 0
diff mbox

Message ID 20140325162204.GH1850@spoyarek.pnq.redhat.com
State New
Headers show

Commit Message

Siddhesh Poyarekar March 25, 2014, 4:22 p.m. UTC
On Tue, Mar 25, 2014 at 01:03:11PM -0300, Adhemerval Zanella wrote:
> I agree with you, but the patch to check if it is defined in nptl/sysdeps/pthread/pthread.h
> was rejected: https://sourceware.org/ml/libc-alpha/2014-03/msg00494.html. So I just folowed
> the way x86_64 and s390 does. If this was just an warning I'd rework it to make it more general,
> as Roland as suggested in https://sourceware.org/ml/libc-alpha/2014-03/msg00501.html; but it is
> breaking 'make check' build. So my preference now is let it as and focus on a proper fix, if
> any, after.

Perhaps it would be acceptable to define __PTHREAD_MUTEX_HAVE_ELISION
unconditionally in pthread.h and then redefine it in x86_64 and s390
like so:


Both approaches ensure that the macro is always defined and hence look
typo-proof to me.

Siddhesh

Comments

Adhemerval Zanella March 25, 2014, 4:44 p.m. UTC | #1
On 25-03-2014 13:22, Siddhesh Poyarekar wrote:
> On Tue, Mar 25, 2014 at 01:03:11PM -0300, Adhemerval Zanella wrote:
>> I agree with you, but the patch to check if it is defined in nptl/sysdeps/pthread/pthread.h
>> was rejected: https://sourceware.org/ml/libc-alpha/2014-03/msg00494.html. So I just folowed
>> the way x86_64 and s390 does. If this was just an warning I'd rework it to make it more general,
>> as Roland as suggested in https://sourceware.org/ml/libc-alpha/2014-03/msg00501.html; but it is
>> breaking 'make check' build. So my preference now is let it as and focus on a proper fix, if
>> any, after.
> Perhaps it would be acceptable to define __PTHREAD_MUTEX_HAVE_ELISION
> unconditionally in pthread.h and then redefine it in x86_64 and s390
> like so:
>
> diff --git a/nptl/sysdeps/pthread/pthread.h b/nptl/sysdeps/pthread/pthread.h
> index 1e0c5dc..524b8c4 100644
> --- a/nptl/sysdeps/pthread/pthread.h
> +++ b/nptl/sysdeps/pthread/pthread.h
> @@ -23,6 +23,8 @@
>  #include <sched.h>
>  #include <time.h>
>  
> +#define __PTHREAD_MUTEX_HAVE_ELISION 0
> +
>  #include <bits/pthreadtypes.h>
>  #include <bits/setjmp.h>
>  #include <bits/wordsize.h>
> diff --git a/nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h b/nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h
> index 28e5144..8548d9d 100644
> --- a/nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h
> +++ b/nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h
> @@ -84,6 +84,7 @@ typedef struct __pthread_internal_slist
>  } __pthread_slist_t;
>  #endif
>  
> +#undef __PTHREAD_MUTEX_HAVE_ELISION
>  
>  /* Data structures for mutex handling.  The structure of the attribute
>     type is not exposed on purpose.  */
>
>
> or alternatively, define __PTHREAD_MUTEX_HAVE_ELISION to 0 if it is
> not yet defined, which is a variant of Will's original fix:
>
> diff --git a/nptl/sysdeps/pthread/pthread.h b/nptl/sysdeps/pthread/pthread.h
> index 1e0c5dc..deeea0d 100644
> --- a/nptl/sysdeps/pthread/pthread.h
> +++ b/nptl/sysdeps/pthread/pthread.h
> @@ -27,6 +27,9 @@
>  #include <bits/setjmp.h>
>  #include <bits/wordsize.h>
>  
> +#ifndef __PTHREAD_MUTEX_HAVE_ELISION
> +# define __PTHREAD_MUTEX_HAVE_ELISION 0
> +#endif
>  
>  /* Detach state.  */
>  enum
>
> Both approaches ensure that the macro is always defined and hence look
> typo-proof to me.
>
> Siddhesh

I would prefer this approach instead of create/install another header.
Roland McGrath March 25, 2014, 4:55 p.m. UTC | #2
> I would prefer this approach instead of create/install another header.

Why?  It leaves a place for typo bugs to sneak in on new architectures.
Adhemerval Zanella March 25, 2014, 5:03 p.m. UTC | #3
On 25-03-2014 13:55, Roland McGrath wrote:
>> I would prefer this approach instead of create/install another header.
> Why?  It leaves a place for typo bugs to sneak in on new architectures.
>
I see a plus header another burden for arch ports to be aware of. GLIBC is already
quite complex of required arch depends files and subly non-arch files that arch
maintainer also need to be aware one. I personally not found of add another one.
Roland McGrath March 25, 2014, 5:09 p.m. UTC | #4
> On 25-03-2014 13:55, Roland McGrath wrote:
> >> I would prefer this approach instead of create/install another header.
> > Why?  It leaves a place for typo bugs to sneak in on new architectures.
> >
> I see a plus header another burden for arch ports to be aware of. GLIBC
> is already quite complex of required arch depends files and subly
> non-arch files that arch maintainer also need to be aware one. I
> personally not found of add another one.

I fail to see how a subtle and typo-prone requirement on some other
existing arch-specific header is better than a typo-proof clear requirement
on a new arch-specific header.
Adhemerval Zanella March 25, 2014, 5:13 p.m. UTC | #5
On 25-03-2014 14:09, Roland McGrath wrote:
>> On 25-03-2014 13:55, Roland McGrath wrote:
>>>> I would prefer this approach instead of create/install another header.
>>> Why?  It leaves a place for typo bugs to sneak in on new architectures.
>>>
>> I see a plus header another burden for arch ports to be aware of. GLIBC
>> is already quite complex of required arch depends files and subly
>> non-arch files that arch maintainer also need to be aware one. I
>> personally not found of add another one.
> I fail to see how a subtle and typo-prone requirement on some other
> existing arch-specific header is better than a typo-proof clear requirement
> on a new arch-specific header.
>
It is a trade-off in my view, but I understand the direction of the -Wundef work
for the typo-proof. I will check on the elision-especific header.

Patch
diff mbox

diff --git a/nptl/sysdeps/pthread/pthread.h b/nptl/sysdeps/pthread/pthread.h
index 1e0c5dc..524b8c4 100644
--- a/nptl/sysdeps/pthread/pthread.h
+++ b/nptl/sysdeps/pthread/pthread.h
@@ -23,6 +23,8 @@ 
 #include <sched.h>
 #include <time.h>
 
+#define __PTHREAD_MUTEX_HAVE_ELISION 0
+
 #include <bits/pthreadtypes.h>
 #include <bits/setjmp.h>
 #include <bits/wordsize.h>
diff --git a/nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h b/nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h
index 28e5144..8548d9d 100644
--- a/nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h
+++ b/nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h
@@ -84,6 +84,7 @@  typedef struct __pthread_internal_slist
 } __pthread_slist_t;
 #endif
 
+#undef __PTHREAD_MUTEX_HAVE_ELISION
 
 /* Data structures for mutex handling.  The structure of the attribute
    type is not exposed on purpose.  */


or alternatively, define __PTHREAD_MUTEX_HAVE_ELISION to 0 if it is
not yet defined, which is a variant of Will's original fix:

diff --git a/nptl/sysdeps/pthread/pthread.h b/nptl/sysdeps/pthread/pthread.h
index 1e0c5dc..deeea0d 100644
--- a/nptl/sysdeps/pthread/pthread.h
+++ b/nptl/sysdeps/pthread/pthread.h
@@ -27,6 +27,9 @@ 
 #include <bits/setjmp.h>
 #include <bits/wordsize.h>
 
+#ifndef __PTHREAD_MUTEX_HAVE_ELISION
+# define __PTHREAD_MUTEX_HAVE_ELISION 0
+#endif
 
 /* Detach state.  */
 enum