[1/7] Fix __PTHREAD_MUTEX_HAVE_ELISION -Wundef warning

ChangeLog:

2014-03-17  Will Newton  <will.newton@linaro.org>

	* nptl/sysdeps/pthread/pthread.h: Check
	__PTHREAD_MUTEX_HAVE_ELISION is defined before testing
	its value.
---
 nptl/sysdeps/pthread/pthread.h | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

On 17-03-2014 09:23, Will Newton wrote:
> ChangeLog:
>
> 2014-03-17  Will Newton  <will.newton@linaro.org>
>
> 	* nptl/sysdeps/pthread/pthread.h: Check
> 	__PTHREAD_MUTEX_HAVE_ELISION is defined before testing
> 	its value.
> ---
>  nptl/sysdeps/pthread/pthread.h | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/nptl/sysdeps/pthread/pthread.h b/nptl/sysdeps/pthread/pthread.h
> index 1e0c5dc..142da63 100644
> --- a/nptl/sysdeps/pthread/pthread.h
> +++ b/nptl/sysdeps/pthread/pthread.h
> @@ -83,12 +83,16 @@ enum
>
>
>  /* Mutex initializers.  */
> -#if __PTHREAD_MUTEX_HAVE_ELISION == 1 /* 64bit layout.  */
> -#define __PTHREAD_SPINS 0, 0
> -#elif __PTHREAD_MUTEX_HAVE_ELISION == 2 /* 32bit layout.  */
> -#define __PTHREAD_SPINS { 0, 0 }
> +#ifdef __PTHREAD_MUTEX_HAVE_ELISION
> +# if __PTHREAD_MUTEX_HAVE_ELISION == 1 /* 64bit layout.  */
> +#  define __PTHREAD_SPINS 0, 0
> +# elif __PTHREAD_MUTEX_HAVE_ELISION == 2 /* 32bit layout.  */
> +#  define __PTHREAD_SPINS { 0, 0 }
> +# else
> +#  error "Unknown value of __PTHREAD_MUTEX_HAVE_ELISION"
> +# endif
>  #else
> -#define __PTHREAD_SPINS 0
> +# define __PTHREAD_SPINS 0
>  #endif
>
>  #ifdef __PTHREAD_MUTEX_HAVE_PREV

Hi Will, I'm ok with this patch (I did the same on ppc builds).

This and some of the others do the style of fix that would be obvious to
someone who hadn't read the discussion here about -Wundef.  I think that in
the general case that is worth than nothing.  Please focus on what you can
fix using typo-proof methods, and discuss explicitly the few cases where
using #idef-like methods is actually necessary.

To clarify, I object to 
commit 9290130a8de3f30970f741c79dfe8459f798e05f
commit f7efd7c3dfffa3c417e9d3c4cb19d9954a3b1421
commit 53f1bed39263541ef7f3d86f9701005524938016
commit 788bba368c2eaf8aa3fd2ca18d269395d6bc8afb

They should be reverted and proper fixes discussed here and reviewed with
more care than these got.

On 17-03-2014 15:37, Roland McGrath wrote:
> To clarify, I object to 
> commit 9290130a8de3f30970f741c79dfe8459f798e05f
> commit f7efd7c3dfffa3c417e9d3c4cb19d9954a3b1421
> commit 53f1bed39263541ef7f3d86f9701005524938016
> commit 788bba368c2eaf8aa3fd2ca18d269395d6bc8afb
>
> They should be reverted and proper fixes discussed here and reviewed with
> more care than these got.
>
For 788bba368c2eaf8aa3fd2ca18d269395d6bc8afb I still think the ifdef for
__PTHREAD_MUTEX_HAVE_ELISION is the way to go, since it fallows the
__PTHREAD_MUTEX_HAVE_PREV logic already in place and avoid adds another
default header or add more arch specific logic for arch that do not 
support it.

> For 788bba368c2eaf8aa3fd2ca18d269395d6bc8afb I still think the ifdef for
> __PTHREAD_MUTEX_HAVE_ELISION is the way to go, since it fallows the
> __PTHREAD_MUTEX_HAVE_PREV logic already in place and avoid adds another
> default header or add more arch specific logic for arch that do not 
> support it.

Someone should audit all the related #if logic and describe what it's
doing.  Then we can consider how best to rework it to be typo-proof.

On 17 March 2014 18:37, Roland McGrath <roland@hack.frob.com> wrote:
> To clarify, I object to
> commit 9290130a8de3f30970f741c79dfe8459f798e05f
> commit f7efd7c3dfffa3c417e9d3c4cb19d9954a3b1421
> commit 53f1bed39263541ef7f3d86f9701005524938016
> commit 788bba368c2eaf8aa3fd2ca18d269395d6bc8afb
>
> They should be reverted and proper fixes discussed here and reviewed with
> more care than these got.

I have reverted these commits as requested.

On 17-03-2014 16:53, Roland McGrath wrote:
>> For 788bba368c2eaf8aa3fd2ca18d269395d6bc8afb I still think the ifdef for
>> __PTHREAD_MUTEX_HAVE_ELISION is the way to go, since it fallows the
>> __PTHREAD_MUTEX_HAVE_PREV logic already in place and avoid adds another
>> default header or add more arch specific logic for arch that do not 
>> support it.
> Someone should audit all the related #if logic and describe what it's
> doing.  Then we can consider how best to rework it to be typo-proof.
>
So your idea is also to refactor __PTHREAD_MUTEX_HAVE_PREV to use as #if ... along with
__PTHREAD_MUTEX_HAVE_ELISION then? Both defines are set by arch specific headers,
so would be the idea to just a generic pthread header (or use an existing one) to 
define them to default values?

> So your idea is also to refactor __PTHREAD_MUTEX_HAVE_PREV to use as #if
> ... along with __PTHREAD_MUTEX_HAVE_ELISION then? Both defines are set by
> arch specific headers, so would be the idea to just a generic pthread
> header (or use an existing one) to define them to default values?

I haven't examined that particular set of macros, so I don't have something
specific in mind.  It's just the general rule that we should organize our
macros (including wholesale reshuffling of what we have today, if need be)
so that every use under our control if typo-proof.

For cases like this in general (sysdeps headers making the choices), I
think when it's straightforward enough to simply require that each variant
of the sysdeps header #define the full set to 1/0 explicitly, then that is
the way to go.  Building with -Wundef ensures that if a new macro is added
and some sysdeps file gets out of date, the next person doing a build of
that configuration will notice with loud and obvious -Wundef warnings.  We
should also establish a clear and regimented convention for comments
describing what macros the sysdeps file is obliged to define.

It's entirely plausible that there will be cases where it seems cleaner to
have a generic header that includes a sysdeps one and then uses #ifndef to
provide defaults.  But that style is prone to typo bugs, so it should be a
high burden to say that this is the preferable approach for any given
header and set of macros.  If there are cases where we decide to do that,
it should be treated like the predefines case: a single block of #if hooey,
clearly isolated from all other logic and in an obvious place like near the
beginning of a header, should do all the #ifndef testing in one place that
is easy to verify by eye, does nothing at all but yield a set of
always-defined macros to be tested later with #if, and has a comments
following a clear convention to say what all the macros are.


Thanks,
Roland

On Tue, 18 Mar 2014, Roland McGrath wrote:

> I haven't examined that particular set of macros, so I don't have something
> specific in mind.  It's just the general rule that we should organize our
> macros (including wholesale reshuffling of what we have today, if need be)
> so that every use under our control if typo-proof.

Typo-proofing also provides another case for replacing the __need_* 
scheme, as I suggested in 
<https://sourceware.org/ml/libc-alpha/2012-08/msg00510.html> and 
<https://sourceware.org/ml/libc-alpha/2012-11/msg00045.html>, so that we 
do

#include <bits/time_t.h>

instead of

#define __need_time_t
#include <time.h>

(the former being typo-proof, the latter not).

> On Tue, 18 Mar 2014, Roland McGrath wrote:
> 
> > I haven't examined that particular set of macros, so I don't have something
> > specific in mind.  It's just the general rule that we should organize our
> > macros (including wholesale reshuffling of what we have today, if need be)
> > so that every use under our control if typo-proof.
> 
> Typo-proofing also provides another case for replacing the __need_* 
> scheme, as I suggested in 
> <https://sourceware.org/ml/libc-alpha/2012-08/msg00510.html> and 
> <https://sourceware.org/ml/libc-alpha/2012-11/msg00045.html>, so that we 
> do
> 
> #include <bits/time_t.h>
> 
> instead of
> 
> #define __need_time_t
> #include <time.h>
> 
> (the former being typo-proof, the latter not).

I didn't recall you suggesting that, but it's been on my list for some
time.  (I think we should use a convention other than plain bits/, but
that's just trivia.)

On Thu, 20 Mar 2014, Roland McGrath wrote:

> > Typo-proofing also provides another case for replacing the __need_* 
> > scheme, as I suggested in 
> > <https://sourceware.org/ml/libc-alpha/2012-08/msg00510.html> and 
> > <https://sourceware.org/ml/libc-alpha/2012-11/msg00045.html>, so that we 
> > do
> > 
> > #include <bits/time_t.h>
> > 
> > instead of
> > 
> > #define __need_time_t
> > #include <time.h>
> > 
> > (the former being typo-proof, the latter not).
> 
> I didn't recall you suggesting that, but it's been on my list for some
> time.  (I think we should use a convention other than plain bits/, but
> that's just trivia.)

Note that for the cases of

#define __need_size_t
#include <stddef.h>

and other cases involving stddef.h, an improvement would require new 
separate headers to be installed by GCC, so that glibc's header for size_t 
could do

#if __GNUC_PREREQ (whatever)
# include <gcc-size_t.h>
#else
# define __need_size_t
# include <stddef.h>
#endif

or similar.