This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH roland/Wundef] Compile with -Wundef.
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Roland McGrath <roland at hack dot frob dot com>, "GNU C. Library" <libc-alpha at sourceware dot org>
- Date: Fri, 14 Mar 2014 14:26:46 -0400
- Subject: Re: [PATCH roland/Wundef] Compile with -Wundef.
- Authentication-results: sourceware.org; auth=none
- References: <20140228232805 dot 1163D744B4 at topped-with-meat 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.