This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 1/4] Split DIAG_* macros to new header libc-diag.h.
- From: Carlos O'Donell <carlos at redhat dot com>
- To: Zack Weinberg <zackw at panix dot com>, libc-alpha at sourceware dot org
- Cc: joseph at codesourcery dot com, adhemerval dot zanella at linaro dot org
- Date: Mon, 20 Feb 2017 08:52:28 -0500
- Subject: Re: [PATCH 1/4] Split DIAG_* macros to new header libc-diag.h.
- Authentication-results: sourceware.org; auth=none
- References: <20170220130342.6373-1-zackw@panix.com> <20170220130342.6373-2-zackw@panix.com>
On 02/20/2017 08:03 AM, Zack Weinberg wrote:
> Quite a few tests include libc-internal.h just for the DIAG_* macros.
> Split those macros to their own file, which can be included safely in
> _ISOMAC mode. I also moved ignore_value, since it seems logically
> related, even though I didn't notice any tests needing it.
>
> Compiling the testsuite in _ISOMAC mode revealed that somehow the
> __nonnull annotations on pthread_mutexattr_destroy and unsetenv, in
> the public headers, are ignored for internal code; because of this,
> two tests need, but do not have, a -Wnonnull suppression. Those
> additions are also included in this patch, to keep everything
> involving libc-diag.h together.
>
> (Query, should libc-diag.h have a copyright header? libc-internal.h
> doesn't. (Should it?))
This patch looks good to me.
Yes, libc-diag.h should have a copyright header.
Yes, libc-internal.h should have a copyright header.
> zw
>
> * include/libc-diag.h: New file which defines ignore_value,
> DIAG_PUSH_NEEDS_COMMENT, DIAG_POP_NEEDS_COMMENT,
> DIAG_IGNORE_NEEDS_COMMENT, and DIAG_IGNORE_Os_NEEDS_COMMENT...
> * include/libc-internal.h: ...moved from here; include libc-diag.h.
>
> * malloc/tst-malloc.c, malloc/tst-memcheck.c, malloc/tst-realloc.c
> * misc/tst-error1.c, posix/tst-dir.c, stdio-common/bug21.c
> * stdio-common/scanf14.c, stdio-common/scanf4.c, stdio-common/scanf7.c
> * stdio-common/test-vfprintf.c, stdio-common/tst-printf.c
> * stdio-common/tst-printfsz.c, stdio-common/tst-sprintf.c
> * stdio-common/tst-unlockedio.c, stdio-common/tstdiomisc.c
> * stdlib/bug-getcontext.c, string/tester.c, string/tst-endian.c
> * time/tst-strptime2.c, wcsmbs/tst-wcstof.c:
> Include libc-diag.h instead of libc-internal.h.
>
> * stdlib/tst-environ.c: Include libc-diag.h. Suppress -Wnonnull for
> call to unsetenv (NULL).
> * nptl/tst-mutex1.c: Include libc-diag.h. Suppress -Wnonnull for
> call to pthread_mutexattr_destroy (NULL).
> ---
> include/libc-diag.h | 57 +++++++++++++++++++++++++++++++++++++++++++
> include/libc-internal.h | 52 +--------------------------------------
> malloc/tst-malloc.c | 2 +-
> malloc/tst-mcheck.c | 2 +-
> malloc/tst-realloc.c | 2 +-
> misc/tst-error1.c | 2 +-
> nptl/tst-mutex1.c | 7 +++++-
> posix/tst-dir.c | 2 +-
> stdio-common/bug21.c | 2 +-
> stdio-common/scanf14.c | 2 +-
> stdio-common/scanf4.c | 2 +-
> stdio-common/scanf7.c | 2 +-
> stdio-common/test-vfprintf.c | 2 +-
> stdio-common/tst-printf.c | 2 +-
> stdio-common/tst-printfsz.c | 2 +-
> stdio-common/tst-sprintf.c | 2 +-
> stdio-common/tst-unlockedio.c | 2 +-
> stdio-common/tstdiomisc.c | 2 +-
> stdlib/bug-getcontext.c | 2 +-
> stdlib/tst-environ.c | 7 +++++-
> string/tester.c | 2 +-
> string/tst-endian.c | 2 +-
> time/tst-strptime2.c | 2 +-
> wcsmbs/tst-wcstof.c | 2 +-
> 24 files changed, 90 insertions(+), 73 deletions(-)
> create mode 100644 include/libc-diag.h
>
> diff --git a/include/libc-diag.h b/include/libc-diag.h
> new file mode 100644
> index 0000000000..a6a413c567
> --- /dev/null
> +++ b/include/libc-diag.h
> @@ -0,0 +1,57 @@
> +/* Macros for controlling diagnostic output from the compiler. */
> +#ifndef _LIBC_DIAG_H
> +#define _LIBC_DIAG_H 1
> +
> +/* Ignore the value of an expression when a cast to void does not
> + suffice (in particular, for a call to a function declared with
> + attribute warn_unused_result). */
> +#define ignore_value(x) \
> + ({ __typeof__ (x) __ignored_value = (x); (void) __ignored_value; })
> +
> +/* The macros to control diagnostics are structured like this, rather
> + than a single macro that both pushes and pops diagnostic state and
> + takes the affected code as an argument, because the GCC pragmas
> + work by disabling the diagnostic for a range of source locations
> + and do not work when all the pragmas and the affected code are in a
> + single macro expansion. */
> +
> +/* Push diagnostic state. */
> +#define DIAG_PUSH_NEEDS_COMMENT _Pragma ("GCC diagnostic push")
> +
> +/* Pop diagnostic state. */
> +#define DIAG_POP_NEEDS_COMMENT _Pragma ("GCC diagnostic pop")
> +
> +#define _DIAG_STR1(s) #s
> +#define _DIAG_STR(s) _DIAG_STR1(s)
> +
> +/* Ignore the diagnostic OPTION. VERSION is the most recent GCC
> + version for which the diagnostic has been confirmed to appear in
> + the absence of the pragma (in the form MAJOR.MINOR for GCC 4.x,
> + just MAJOR for GCC 5 and later). Uses of this pragma should be
> + reviewed when the GCC version given is no longer supported for
> + building glibc; the version number should always be on the same
> + source line as the macro name, so such uses can be found with grep.
> + Uses should come with a comment giving more details of the
> + diagnostic, and an architecture on which it is seen if possibly
> + optimization-related and not in architecture-specific code. This
> + macro should only be used if the diagnostic seems hard to fix (for
> + example, optimization-related false positives). */
> +#define DIAG_IGNORE_NEEDS_COMMENT(version, option) \
> + _Pragma (_DIAG_STR (GCC diagnostic ignored option))
> +
> +/* Similar to DIAG_IGNORE_NEEDS_COMMENT the following macro ignores the
> + diagnostic OPTION but only if optimizations for size are enabled.
> + This is required because different warnings may be generated for
> + different optimization levels. For example a key piece of code may
> + only generate a warning when compiled at -Os, but at -O2 you could
> + still want the warning to be enabled to catch errors. In this case
> + you would use DIAG_IGNORE_Os_NEEDS_COMMENT to disable the warning
> + only for -Os. */
> +#ifdef __OPTIMIZE_SIZE__
> +# define DIAG_IGNORE_Os_NEEDS_COMMENT(version, option) \
> + _Pragma (_DIAG_STR (GCC diagnostic ignored option))
> +#else
> +# define DIAG_IGNORE_Os_NEEDS_COMMENT(version, option)
> +#endif
> +
> +#endif /* libc-diag.h */
> diff --git a/include/libc-internal.h b/include/libc-internal.h
> index e4395dd5c5..a38a35d87c 100644
> --- a/include/libc-internal.h
> +++ b/include/libc-internal.h
> @@ -74,56 +74,6 @@ extern __typeof (__profile_frequency) __profile_frequency attribute_hidden;
> #define PTR_ALIGN_UP(base, size) \
> ((__typeof__ (base)) ALIGN_UP ((uintptr_t) (base), (size)))
>
> -/* Ignore the value of an expression when a cast to void does not
> - suffice (in particular, for a call to a function declared with
> - attribute warn_unused_result). */
> -#define ignore_value(x) \
> - ({ __typeof__ (x) __ignored_value = (x); (void) __ignored_value; })
> -
> -/* The macros to control diagnostics are structured like this, rather
> - than a single macro that both pushes and pops diagnostic state and
> - takes the affected code as an argument, because the GCC pragmas
> - work by disabling the diagnostic for a range of source locations
> - and do not work when all the pragmas and the affected code are in a
> - single macro expansion. */
> -
> -/* Push diagnostic state. */
> -#define DIAG_PUSH_NEEDS_COMMENT _Pragma ("GCC diagnostic push")
> -
> -/* Pop diagnostic state. */
> -#define DIAG_POP_NEEDS_COMMENT _Pragma ("GCC diagnostic pop")
> -
> -#define _DIAG_STR1(s) #s
> -#define _DIAG_STR(s) _DIAG_STR1(s)
> -
> -/* Ignore the diagnostic OPTION. VERSION is the most recent GCC
> - version for which the diagnostic has been confirmed to appear in
> - the absence of the pragma (in the form MAJOR.MINOR for GCC 4.x,
> - just MAJOR for GCC 5 and later). Uses of this pragma should be
> - reviewed when the GCC version given is no longer supported for
> - building glibc; the version number should always be on the same
> - source line as the macro name, so such uses can be found with grep.
> - Uses should come with a comment giving more details of the
> - diagnostic, and an architecture on which it is seen if possibly
> - optimization-related and not in architecture-specific code. This
> - macro should only be used if the diagnostic seems hard to fix (for
> - example, optimization-related false positives). */
> -#define DIAG_IGNORE_NEEDS_COMMENT(version, option) \
> - _Pragma (_DIAG_STR (GCC diagnostic ignored option))
> -
> -/* Similar to DIAG_IGNORE_NEEDS_COMMENT the following macro ignores the
> - diagnostic OPTION but only if optimizations for size are enabled.
> - This is required because different warnings may be generated for
> - different optimization levels. For example a key piece of code may
> - only generate a warning when compiled at -Os, but at -O2 you could
> - still want the warning to be enabled to catch errors. In this case
> - you would use DIAG_IGNORE_Os_NEEDS_COMMENT to disable the warning
> - only for -Os. */
> -#ifdef __OPTIMIZE_SIZE__
> -# define DIAG_IGNORE_Os_NEEDS_COMMENT(version, option) \
> - _Pragma (_DIAG_STR (GCC diagnostic ignored option))
> -#else
> -# define DIAG_IGNORE_Os_NEEDS_COMMENT(version, option)
> -#endif
> +#include <libc-diag.h>
>
> #endif /* _LIBC_INTERNAL */
> diff --git a/malloc/tst-malloc.c b/malloc/tst-malloc.c
> index 740ac6ce31..dbc8d4ab56 100644
> --- a/malloc/tst-malloc.c
> +++ b/malloc/tst-malloc.c
> @@ -19,7 +19,7 @@
> #include <errno.h>
> #include <malloc.h>
> #include <stdio.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
> static int errors = 0;
>
> diff --git a/malloc/tst-mcheck.c b/malloc/tst-mcheck.c
> index 2e3cba96b8..5a66bab331 100644
> --- a/malloc/tst-mcheck.c
> +++ b/malloc/tst-mcheck.c
> @@ -19,7 +19,7 @@
> #include <errno.h>
> #include <stdio.h>
> #include <stdlib.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
> static int errors = 0;
>
> diff --git a/malloc/tst-realloc.c b/malloc/tst-realloc.c
> index 7f1f228c06..31a58bd026 100644
> --- a/malloc/tst-realloc.c
> +++ b/malloc/tst-realloc.c
> @@ -19,7 +19,7 @@
> #include <malloc.h>
> #include <stdio.h>
> #include <string.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
> static int errors = 0;
>
> diff --git a/misc/tst-error1.c b/misc/tst-error1.c
> index a97a22ce9d..9c4a62fbd0 100644
> --- a/misc/tst-error1.c
> +++ b/misc/tst-error1.c
> @@ -3,7 +3,7 @@
> #include <stdio.h>
> #include <string.h>
> #include <wchar.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
> static int
> do_test (int argc, char *argv[])
> diff --git a/nptl/tst-mutex1.c b/nptl/tst-mutex1.c
> index 8a4f2e6e4e..b2a4b5492a 100644
> --- a/nptl/tst-mutex1.c
> +++ b/nptl/tst-mutex1.c
> @@ -20,7 +20,7 @@
> #include <stdio.h>
> #include <errno.h>
> #include <stdbool.h>
> -
> +#include <libc-diag.h>
>
> #ifndef ATTR
> # define ATTR NULL
> @@ -45,11 +45,16 @@ do_test (void)
> return 1;
> }
>
> + /* This deliberately tests supplying a null pointer to a function whose
> + argument is marked __attribute__ ((nonnull)). */
> + DIAG_PUSH_NEEDS_COMMENT;
> + DIAG_IGNORE_NEEDS_COMMENT (5, "-Wnonnull");
> if (!ATTR_NULL && pthread_mutexattr_destroy (ATTR) != 0)
> {
> puts ("mutexattr_destroy failed");
> return 1;
> }
> + DIAG_POP_NEEDS_COMMENT;
>
> if (pthread_mutex_lock (&m) != 0)
> {
> diff --git a/posix/tst-dir.c b/posix/tst-dir.c
> index 1a9c7dfe97..fee79b32a0 100644
> --- a/posix/tst-dir.c
> +++ b/posix/tst-dir.c
> @@ -26,7 +26,7 @@
> #include <string.h>
> #include <unistd.h>
> #include <sys/stat.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
> /* We expect four arguments:
> - source directory name
> diff --git a/stdio-common/bug21.c b/stdio-common/bug21.c
> index ca27272ba1..7a8c6a3542 100644
> --- a/stdio-common/bug21.c
> +++ b/stdio-common/bug21.c
> @@ -1,5 +1,5 @@
> #include <stdio.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
> static int
> do_test (void)
> diff --git a/stdio-common/scanf14.c b/stdio-common/scanf14.c
> index cffccb0b19..2bcd9c9893 100644
> --- a/stdio-common/scanf14.c
> +++ b/stdio-common/scanf14.c
> @@ -2,7 +2,7 @@
> #include <stdlib.h>
> #include <string.h>
> #include <wchar.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
> #define FAIL() \
> do { \
> diff --git a/stdio-common/scanf4.c b/stdio-common/scanf4.c
> index 9bb14bb16b..7a2abec89b 100644
> --- a/stdio-common/scanf4.c
> +++ b/stdio-common/scanf4.c
> @@ -1,6 +1,6 @@
> #include <stdio.h>
> #include <stdlib.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
> int
> main(int arc, char *argv[])
> diff --git a/stdio-common/scanf7.c b/stdio-common/scanf7.c
> index 53ddf4cb09..f568738d7e 100644
> --- a/stdio-common/scanf7.c
> +++ b/stdio-common/scanf7.c
> @@ -1,6 +1,6 @@
> #include <stdio.h>
> #include <stdlib.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
> int
> main (int argc, char *argv[])
> diff --git a/stdio-common/test-vfprintf.c b/stdio-common/test-vfprintf.c
> index f1805d5d00..f8bb9cee58 100644
> --- a/stdio-common/test-vfprintf.c
> +++ b/stdio-common/test-vfprintf.c
> @@ -25,7 +25,7 @@
> #include <string.h>
> #include <unistd.h>
> #include <sys/stat.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
>
> const char *locs[] =
> diff --git a/stdio-common/tst-printf.c b/stdio-common/tst-printf.c
> index 719b3eb08c..b6d62a5a2f 100644
> --- a/stdio-common/tst-printf.c
> +++ b/stdio-common/tst-printf.c
> @@ -26,7 +26,7 @@
> #endif
>
> #include <float.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
> /* This whole file is picayune tests of corner cases of printf format strings.
> The compiler warnings are not useful here. */
> diff --git a/stdio-common/tst-printfsz.c b/stdio-common/tst-printfsz.c
> index 8a3385f94d..47aa8536b3 100644
> --- a/stdio-common/tst-printfsz.c
> +++ b/stdio-common/tst-printfsz.c
> @@ -2,7 +2,7 @@
> #include <printf.h>
> #include <stdio.h>
> #include <string.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
> #define V 12345678.12345678
>
> diff --git a/stdio-common/tst-sprintf.c b/stdio-common/tst-sprintf.c
> index d5284b9697..cbd35447a9 100644
> --- a/stdio-common/tst-sprintf.c
> +++ b/stdio-common/tst-sprintf.c
> @@ -2,7 +2,7 @@
> #include <stdlib.h>
> #include <locale.h>
> #include <string.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
>
> static int
> diff --git a/stdio-common/tst-unlockedio.c b/stdio-common/tst-unlockedio.c
> index 6eec6fdbac..35652ce772 100644
> --- a/stdio-common/tst-unlockedio.c
> +++ b/stdio-common/tst-unlockedio.c
> @@ -20,7 +20,7 @@
> #include <stdlib.h>
> #include <stdio.h>
> #include <string.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
> int fd;
> static void do_prepare (void);
> diff --git a/stdio-common/tstdiomisc.c b/stdio-common/tstdiomisc.c
> index 9c7342d55f..89009e0549 100644
> --- a/stdio-common/tstdiomisc.c
> +++ b/stdio-common/tstdiomisc.c
> @@ -3,7 +3,7 @@
> #include <stdio.h>
> #include <string.h>
> #include <wchar.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
> static int
> t1 (void)
> diff --git a/stdlib/bug-getcontext.c b/stdlib/bug-getcontext.c
> index c4072129a6..163400acba 100644
> --- a/stdlib/bug-getcontext.c
> +++ b/stdlib/bug-getcontext.c
> @@ -5,7 +5,7 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <ucontext.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
> static int
> do_test (void)
> diff --git a/stdlib/tst-environ.c b/stdlib/tst-environ.c
> index 6a29fed62a..b2301641f5 100644
> --- a/stdlib/tst-environ.c
> +++ b/stdlib/tst-environ.c
> @@ -19,7 +19,7 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> -
> +#include <libc-diag.h>
>
> #define VAR "FOOBAR"
>
> @@ -196,12 +196,17 @@ do_test (void)
> result = 1;
> }
>
> + /* This deliberately tests supplying a null pointer to a function whose
> + argument is marked __attribute__ ((nonnull)). */
> + DIAG_PUSH_NEEDS_COMMENT;
> + DIAG_IGNORE_NEEDS_COMMENT(5, "-Wnonnull");
> errno = 0;
> if (unsetenv (NULL) >= 0 || errno != EINVAL)
> {
> puts ("unsetenv #1 failed");
> result = 1;
> }
> + DIAG_POP_NEEDS_COMMENT;
>
> errno = 0;
> if (unsetenv ("") >= 0 || errno != EINVAL)
> diff --git a/string/tester.c b/string/tester.c
> index ec350243eb..4b928b4f5e 100644
> --- a/string/tester.c
> +++ b/string/tester.c
> @@ -32,7 +32,7 @@
> #include <string.h>
> #include <strings.h>
> #include <fcntl.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
>
> #define STREQ(a, b) (strcmp((a), (b)) == 0)
> diff --git a/string/tst-endian.c b/string/tst-endian.c
> index 7d39131a68..d3c7c2c4c5 100644
> --- a/string/tst-endian.c
> +++ b/string/tst-endian.c
> @@ -3,7 +3,7 @@
> #include <inttypes.h>
> #include <stdio.h>
> #include <stdint.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
> #if __GNUC_PREREQ (6, 0)
> /* GCC 6.0 warns on big endian systems about:
> diff --git a/time/tst-strptime2.c b/time/tst-strptime2.c
> index 9273568b6f..8019e7f5d8 100644
> --- a/time/tst-strptime2.c
> +++ b/time/tst-strptime2.c
> @@ -4,7 +4,7 @@
> #include <stdbool.h>
> #include <stdio.h>
> #include <time.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
> /* Dummy string is used to match strptime's %s specifier. */
>
> diff --git a/wcsmbs/tst-wcstof.c b/wcsmbs/tst-wcstof.c
> index 861f65905a..d3d75f8f60 100644
> --- a/wcsmbs/tst-wcstof.c
> +++ b/wcsmbs/tst-wcstof.c
> @@ -3,7 +3,7 @@
> #include <stdio.h>
> #include <string.h>
> #include <wctype.h>
> -#include <libc-internal.h>
> +#include <libc-diag.h>
>
> static int
> do_test (void)
>
--
Cheers,
Carlos.