Re: [PATCH roland/Wundef] Compile with -Wundef.

["Carlos O'Donell" <carlos at redhat dot com>]

On 02/28/2014 06:28 PM, Roland McGrath wrote:
> This adds -Wundef and fixes the most egregious cases that were warning on
> just about every file in my x86_64-linux-gnu build.  There are many
> warnings remaining, but I'll let others pursue all those.  Some of them
> seem like they were masking real bugs.
> 
> In general, I think we want to get to a place where we have no #ifdef (or
> "defined ...") tests at all, outside perhaps a few infrastructure headers
> and the occasional special case (and installed headers, of course).  They
> are inherently error-prone: you won't notice if you made a typo or failed
> to #include the header that should define the macro, etc.  Eventually with
> -Werror=undef and the style of always using "#if FOO", we'll get reliable
> build errors for that sort of bug.
> 
> For similar cases, I think the include/stackinfo.h wrapper header is
> probably a good model.  That is, something that makes the requirements on
> the sysdeps header fairly simple, but enforces them strictly to catch bugs
> quickly.  I first tried to do the same with tls.h, but ran into recursive
> inclusion order hell and decided it was simpler for now to just change the
> contract to require each sysdeps/.../tls.h file to define both macros
> (there is nothing that enforces that exactly one of the two evaluates to
> true, which would be ideal).  The mess of early include order that results
> from tls.h has caused plenty of other headaches too.  We really should
> figure out how to clean that up more sensibly at some point.
> 
> I'd be happy for this to go in now, but only if there are people who want
> to commit to tracking down all the warnings and doing the cleanup in the
> next couple of weeks.  As I mentioned before, I think the right way to go
> about this is to choose one macro (or small set of closely related macros)
> and fix up all warnings related to that one macro in one patch, not
> including any other macros in the same patch.
> 
> It's a longer-term project to clean up all our #if uses to eliminate
> #ifdef and its perils.
> 
> 
> Thanks,
> Roland
> 
> 
> 2014-02-28  Roland McGrath  <roland@hack.frob.com>
> 
> 	* Makeconfig (+gccwarn): Add -Wundef.
> 	* include/errno.h [IS_IN_rtld] [!RTLD_PRIVATE_ERRNO]: #error to catch
> 	a dl-sysdep.h breaking its contract.
> 	[!IS_IN_rtld] (RTLD_PRIVATE_ERRNO): Define it to 0.
> 	* include/stackinfo.h: New file.
> 	* sysdeps/aarch64/nptl/tls.h (TLS_TCB_AT_TP): New macro.
> 	* sysdeps/alpha/nptl/tls.h (TLS_TCB_AT_TP): New macro.
> 	* sysdeps/arm/nptl/tls.h (TLS_TCB_AT_TP): New macro.
> 	* sysdeps/ia64/nptl/tls.h (TLS_TCB_AT_TP): New macro.
> 	* sysdeps/m68k/nptl/tls.h (TLS_TCB_AT_TP): New macro.
> 	* sysdeps/mach/hurd/i386/tls.h (TLS_TCB_AT_TP): New macro.
> 	* sysdeps/microblaze/nptl/tls.h (TLS_TCB_AT_TP): New macro.
> 	* sysdeps/mips/nptl/tls.h (TLS_TCB_AT_TP): New macro.
> 	* sysdeps/tile/nptl/tls.h (TLS_TCB_AT_TP): New macro.
> 
> nptl/
> 	* sysdeps/i386/tls.h (TLS_DTV_AT_TP): New macro.
> 	* sysdeps/powerpc/tls.h (TLS_TCB_AT_TP): New macro.
> 	* sysdeps/s390/tls.h (TLS_DTV_AT_TP): New macro.
> 	* sysdeps/sh/tls.h (TLS_TCB_AT_TP): New macro.
> 	* sysdeps/sparc/tls.h (TLS_DTV_AT_TP): New macro.
> 	* sysdeps/x86_64/tls.h (TLS_DTV_AT_TP): New macro.
> 
> ports/ChangeLog.hppa
> 	* sysdeps/hppa/nptl/tls.h (TLS_TCB_AT_TP): New macro.

This looks good to me.

I'll help cleanup once you check this in.

> --- a/Makeconfig
> +++ b/Makeconfig
> @@ -685,6 +685,7 @@ ifeq ($(all-warnings),yes)
>  else
>  +gccwarn := -Wall -Wwrite-strings -Winline
>  endif
> ++gccwarn += -Wundef

OK.

>  +gccwarn-c = -Wstrict-prototypes
>  
>  # We do not depend on the address of constants in different files to be
> --- a/include/errno.h
> +++ b/include/errno.h
> @@ -6,6 +6,11 @@
>  
>  # ifdef IS_IN_rtld
>  #  include <dl-sysdep.h>
> +#  ifndef RTLD_PRIVATE_ERRNO
> +#   error "dl-sysdep.h must define RTLD_PRIVATE_ERRNO!"
> +#  endif
> +# else
> +#  define RTLD_PRIVATE_ERRNO	0
>  # endif

OK.
 
>  # if RTLD_PRIVATE_ERRNO
> --- /dev/null
> +++ b/include/stackinfo.h
> @@ -0,0 +1,42 @@
> +/* Details about the machine's stack: wrapper header.
> +   Copyright (C) 2014 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef _INCLUDE_STACKINFO_H
> +#define _INCLUDE_STACKINFO_H	1
> +
> +/* A sysdeps/.../stackinfo.h file defines details for the CPU.
> +   It is obliged to define either _STACK_GROWS_DOWN or _STACK_GROWS_UP.  */
> +#include_next <stackinfo.h>
> +
> +#if defined _STACK_GROWS_DOWN && _STACK_GROWS_DOWN
> +# ifdef _STACK_GROWS_UP
> +#  error "stackinfo.h should not define both!"
> +# else
> +#  define _STACK_GROWS_UP	0
> +# endif
> +#elif defined _STACK_GROWS_UP && _STACK_GROWS_UP
> +# ifdef _STACK_GROWS_DOWN
> +#  error "stackinfo.h should not define both!"
> +# else
> +#  define _STACK_GROWS_DOWN	0
> +# endif
> +#else
> +# error "stackinfo.h must define _STACK_GROWS_UP or _STACK_GROWS_DOWN!"
> +#endif
> +
> +#endif  /* include/stackinfo.h */

OK.

> --- a/nptl/sysdeps/i386/tls.h
> +++ b/nptl/sysdeps/i386/tls.h
> @@ -104,9 +104,6 @@ union user_desc_init
>  };
>  
>  
> -/* Get the thread descriptor definition.  */
> -# include <nptl/descr.h>
> -
>  /* This is the size of the initial TCB.  Can't be just sizeof (tcbhead_t),
>     because NPTL getpid, __libc_alloca_cutoff etc. need (almost) the whole
>     struct pthread even when not linked with -lpthread.  */
> @@ -124,6 +121,10 @@ union user_desc_init
>  /* The TCB can have any size and the memory following the address the
>     thread pointer points to is unspecified.  Allocate the TCB there.  */
>  # define TLS_TCB_AT_TP	1
> +# define TLS_DTV_AT_TP	0
> +
> +/* Get the thread descriptor definition.  */
> +# include <nptl/descr.h>
>  
>  
>  /* Install the dtv pointer.  The pointer passed is to the element with
> --- a/nptl/sysdeps/powerpc/tls.h
> +++ b/nptl/sysdeps/powerpc/tls.h
> @@ -49,6 +49,7 @@ typedef union dtv
>  
>  /* The TP points to the start of the thread blocks.  */
>  # define TLS_DTV_AT_TP	1
> +# define TLS_TCB_AT_TP	0
>  
>  /* We use the multiple_threads field in the pthread struct */
>  #define TLS_MULTIPLE_THREADS_IN_TCB	1
> @@ -56,6 +57,7 @@ typedef union dtv
>  /* Get the thread descriptor definition.  */
>  # include <nptl/descr.h>
>  
> +
>  /* The stack_guard is accessed directly by GCC -fstack-protector code,
>     so it is a part of public ABI.  The dtv and pointer_guard fields
>     are private.  */
> --- a/nptl/sysdeps/s390/tls.h
> +++ b/nptl/sysdeps/s390/tls.h
> @@ -73,9 +73,6 @@ typedef struct
>  /* Get system call information.  */
>  # include <sysdep.h>
>  
> -/* Get the thread descriptor definition.  */
> -# include <nptl/descr.h>
> -
>  /* This is the size of the initial TCB.  Can't be just sizeof (tcbhead_t),
>     because NPTL getpid, __libc_alloca_cutoff etc. need (almost) the whole
>     struct pthread even when not linked with -lpthread.  */
> @@ -93,6 +90,10 @@ typedef struct
>  /* The TCB can have any size and the memory following the address the
>     thread pointer points to is unspecified.  Allocate the TCB there.  */
>  # define TLS_TCB_AT_TP	1
> +# define TLS_DTV_AT_TP	0
> +
> +/* Get the thread descriptor definition.  */
> +# include <nptl/descr.h>
>  
>  
>  /* Install the dtv pointer.  The pointer passed is to the element with
> --- a/nptl/sysdeps/sh/tls.h
> +++ b/nptl/sysdeps/sh/tls.h
> @@ -76,6 +76,7 @@ typedef struct
>  
>  /* The TLS blocks start right after the TCB.  */
>  # define TLS_DTV_AT_TP	1
> +# define TLS_TCB_AT_TP	0
>  
>  /* Get the thread descriptor definition.  */
>  # include <nptl/descr.h>
> --- a/nptl/sysdeps/sparc/tls.h
> +++ b/nptl/sysdeps/sparc/tls.h
> @@ -69,9 +69,6 @@ typedef struct
>  /* Get system call information.  */
>  # include <sysdep.h>
>  
> -/* Get the thread descriptor definition.  */
> -# include <nptl/descr.h>
> -
>  register struct pthread *__thread_self __asm__("%g7");
>  
>  /* This is the size of the initial TCB.  Can't be just sizeof (tcbhead_t),
> @@ -91,6 +88,10 @@ register struct pthread *__thread_self __asm__("%g7");
>  /* The TCB can have any size and the memory following the address the
>     thread pointer points to is unspecified.  Allocate the TCB there.  */
>  # define TLS_TCB_AT_TP	1
> +# define TLS_DTV_AT_TP	0
> +
> +/* Get the thread descriptor definition.  */
> +# include <nptl/descr.h>
>  
>  /* Install the dtv pointer.  The pointer passed is to the element with
>     index -1 which contain the length.  */
> --- a/nptl/sysdeps/x86_64/tls.h
> +++ b/nptl/sysdeps/x86_64/tls.h
> @@ -92,10 +92,6 @@ typedef struct
>  /* Get system call information.  */
>  # include <sysdep.h>
>  
> -
> -/* Get the thread descriptor definition.  */
> -# include <nptl/descr.h>
> -
>  #ifndef LOCK_PREFIX
>  # ifdef UP
>  #  define LOCK_PREFIX	/* nothing */
> @@ -121,6 +117,10 @@ typedef struct
>  /* The TCB can have any size and the memory following the address the
>     thread pointer points to is unspecified.  Allocate the TCB there.  */
>  # define TLS_TCB_AT_TP	1
> +# define TLS_DTV_AT_TP	0
> +
> +/* Get the thread descriptor definition.  */
> +# include <nptl/descr.h>
>  
>  
>  /* Install the dtv pointer.  The pointer passed is to the element with
> --- a/ports/sysdeps/hppa/nptl/tls.h
> +++ b/ports/sysdeps/hppa/nptl/tls.h
> @@ -51,6 +51,7 @@ typedef union dtv
>  
>  /* The TP points to the start of the thread blocks.  */
>  # define TLS_DTV_AT_TP	1
> +# define TLS_TCB_AT_TP	0
>  
>  /* Get the thread descriptor definition.  */
>  # include <nptl/descr.h>
> --- a/sysdeps/aarch64/nptl/tls.h
> +++ b/sysdeps/aarch64/nptl/tls.h
> @@ -48,6 +48,7 @@ typedef union dtv
>  
>  /* The TP points to the start of the thread blocks.  */
>  # define TLS_DTV_AT_TP	1
> +# define TLS_TCB_AT_TP	0
>  
>  /* Get the thread descriptor definition.  */
>  # include <nptl/descr.h>
> --- a/sysdeps/alpha/nptl/tls.h
> +++ b/sysdeps/alpha/nptl/tls.h
> @@ -42,6 +42,7 @@ typedef union dtv
>  
>  /* The TP points to the start of the thread blocks.  */
>  # define TLS_DTV_AT_TP	1
> +# define TLS_TCB_AT_TP	0
>  
>  /* Get the thread descriptor definition.  */
>  # include <nptl/descr.h>
> --- a/sysdeps/arm/nptl/tls.h
> +++ b/sysdeps/arm/nptl/tls.h
> @@ -49,6 +49,7 @@ typedef union dtv
>  
>  /* The TP points to the start of the thread blocks.  */
>  # define TLS_DTV_AT_TP	1
> +# define TLS_TCB_AT_TP	0
>  
>  /* Get the thread descriptor definition.  */
>  # include <nptl/descr.h>
> --- a/sysdeps/ia64/nptl/tls.h
> +++ b/sysdeps/ia64/nptl/tls.h
> @@ -87,6 +87,7 @@ register struct pthread *__thread_self __asm__("r13");
>  
>  /* The DTV is allocated at the TP; the TCB is placed elsewhere.  */
>  # define TLS_DTV_AT_TP	1
> +# define TLS_TCB_AT_TP	0
>  
>  /* Get the thread descriptor definition.  */
>  # include <nptl/descr.h>
> --- a/sysdeps/m68k/nptl/tls.h
> +++ b/sysdeps/m68k/nptl/tls.h
> @@ -49,6 +49,7 @@ typedef union dtv
>  
>  /* The TP points to the start of the thread blocks.  */
>  # define TLS_DTV_AT_TP	1
> +# define TLS_TCB_AT_TP	0
>  
>  /* Get the thread descriptor definition.  */
>  # include <nptl/descr.h>
> --- a/sysdeps/mach/hurd/i386/tls.h
> +++ b/sysdeps/mach/hurd/i386/tls.h
> @@ -26,6 +26,7 @@
>  /* The TCB can have any size and the memory following the address the
>     thread pointer points to is unspecified.  Allocate the TCB there.  */
>  #define TLS_TCB_AT_TP	1
> +#define TLS_TCB_AT_TP	0

As Thomas pointed out that's wrong.

>  
>  #ifndef __ASSEMBLER__
>  
> --- a/sysdeps/microblaze/nptl/tls.h
> +++ b/sysdeps/microblaze/nptl/tls.h
> @@ -48,6 +48,7 @@ typedef union dtv
>  
>  /* The TP points to the start of the thread blocks.  */
>  # define TLS_DTV_AT_TP  1
> +# define TLS_TCB_AT_TP  0
>  
>  /* Get the thread descriptor definition.  */
>  # include <nptl/descr.h>
> --- a/sysdeps/mips/nptl/tls.h
> +++ b/sysdeps/mips/nptl/tls.h
> @@ -67,6 +67,7 @@ typedef union dtv
>  
>  /* The TP points to the start of the thread blocks.  */
>  # define TLS_DTV_AT_TP	1
> +# define TLS_TCB_AT_TP	0
>  
>  /* Get the thread descriptor definition.  */
>  # include <nptl/descr.h>
> --- a/sysdeps/tile/nptl/tls.h
> +++ b/sysdeps/tile/nptl/tls.h
> @@ -49,6 +49,7 @@ typedef union dtv
>  
>  /* The TP points to the start of the thread blocks.  */
>  # define TLS_DTV_AT_TP	1
> +# define TLS_TCB_AT_TP	0
>  
>  /* We use the multiple_threads field in the pthread struct */
>  #define TLS_MULTIPLE_THREADS_IN_TCB	1
> 

OK.

Cheers,
Carlos.