Message ID | 20140325162204.GH1850@spoyarek.pnq.redhat.com |
---|---|
State | New |
Headers | show |
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.
> 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.
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.
> 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.
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.
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