Message ID | alpine.DEB.2.10.1411252243090.17237@digraph.polyomino.org.uk |
---|---|
State | New |
Headers | show |
Joseph Myers wrote: > + /* GCC in C99 mode treats %a as the C99 format expecting float *, > + but glibc with _GNU_SOURCE treats %as as the GNU allocation > + extension, so resulting in "warning: format '%a' expects argument > + of type 'float *', but argument 3 has type 'char **'". This > + applies to the other %as, %aS and %a[] formats below as well. */ > + DIAG_PUSH_NEEDS_COMMENT; > + DIAG_IGNORE_NEEDS_COMMENT (4.9, "-Wformat"); > if (sscanf (" 1.25s x", "%as%2c", &sp, c) != 2) > FAIL (); > else > @@ -32,10 +40,14 @@ main (void) > memset (sp, 'x', sizeof "1.25s"); > free (sp); > } > + DIAG_POP_NEEDS_COMMENT; This sort of thing looks intrusive. Instead, how about defining a macro 'C99_sscanf' that does the push and pop and commentary, and then simply replacing warned-about uses of sscanf with C99_scanf? That'd be a lot easier to read, and it's more along the lines of Roland's original suggestion.
On Tue, 25 Nov 2014, Paul Eggert wrote: > This sort of thing looks intrusive. Instead, how about defining a macro > 'C99_sscanf' that does the push and pop and commentary, and then simply > replacing warned-about uses of sscanf with C99_scanf? That'd be a lot easier > to read, and it's more along the lines of Roland's original suggestion. Again, it doesn't work to have the pragmas in the same macro as the code in which the diagnostics are suppressed; they need to be in separate macros for the suppression to be effective.
Joseph Myers wrote:
> it doesn't work to have the pragmas in the same macro as the code
Then how about using an inline function instead of a macro? Something like this:
static inline __attribute__ ((__always_inline__)) int
C99_sscanf (char const *str, char const *format, ...)
{
return sscanf (str, format, __builtin_va_arg_pack ());
}
This should silence the warning more accurately than the pragma does.
Similarly, if we're worried about fread_unlocked complaining about a zero-sized
argument, perhaps we can write an inline wrapper for it; that should silence thw
warning more accurately.
One might reasonably object that I'm going off the deep end here: after all,
it's just a test case and we needn't care all that much about narrowly tailoring
warning-suppression for test cases. I'm sympathetic to that objection. But in
that case, why not just disable the warning for the entire test-case compilation
unit? That'd be simpler yet. There'd be no need for pragma macros or for
pushing or popping diagnostics, and it'd be simpler for others to follow what's
going on.
I'm sorry if I appear to be niggling here, it's just that we've had a lot of
experience dodging these warnings in GNU applications ...
On Wed, 26 Nov 2014, Paul Eggert wrote: > Joseph Myers wrote: > > it doesn't work to have the pragmas in the same macro as the code > > Then how about using an inline function instead of a macro? Something like > this: > > static inline __attribute__ ((__always_inline__)) int > C99_sscanf (char const *str, char const *format, ...) > { > return sscanf (str, format, __builtin_va_arg_pack ()); > } I'm doubtful about hiding warnings in ways that rely on the compiler not being smart enough to see through them - it would be perfectly reasonable for the compiler to warn in this case by doing format checking after optimization. Warnings should be disabled in ways that clearly make it incorrect for the compiler to warn. > Similarly, if we're worried about fread_unlocked complaining about a > zero-sized argument, perhaps we can write an inline wrapper for it; that > should silence thw warning more accurately. An inline wrapper would not be appropriate; these tests are specifically using arguments with side-effects, and arguments with unusual types, to verify that those work with the macro definitions the same way as they would work with a normal function call (i.e. they are evaluated at least once, and the floating-point arguments are converted to the correct argument types). Using an inline wrapper would defect this testing.
Joseph Myers wrote: > I'm doubtful about hiding warnings in ways that rely on the compiler not > being smart enough to see through them ... > An inline wrapper would not be appropriate; these tests are specifically > using arguments with side-effects, and arguments with unusual types Fair enough on both points, but this underscores the suggestion that for test cases let's do just put a simple pragma at the start of the file. To grep it later we can use a stylized comment, like this: /* (GCC 4.9) gcc -Werror -Wformat fails in C99 mode because ... */ #pragma GCC diagnostic ignored "-Wformat" as it's not worth our time (or our readers' time) to get fancier on test cases.
Ping. This patch <https://sourceware.org/ml/libc-alpha/2014-11/msg00736.html> is pending review. Also pending review and depending on it are three other warning suppression patches <https://sourceware.org/ml/libc-alpha/2014-11/msg00752.html>, <https://sourceware.org/ml/libc-alpha/2014-11/msg00785.html> and <https://sourceware.org/ml/libc-alpha/2014-11/msg00791.html>, and the actual -Werror patch <https://sourceware.org/ml/libc-alpha/2014-11/msg00798.html>.
Ping^2. This patch <https://sourceware.org/ml/libc-alpha/2014-11/msg00736.html> is still pending review. If the use for the %a tests is controversial, review of the macro definitions on their own would still be helpful.
That's OK by me. But it would be better if the explanations about how GCC limitations influenced the macro API choices were in comments in the header itself.
On Tue, 9 Dec 2014, Roland McGrath wrote: > That's OK by me. But it would be better if the explanations about how GCC > limitations influenced the macro API choices were in comments in the header > itself. I've committed the patch with this comment. /* 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. */
diff --git a/include/libc-internal.h b/include/libc-internal.h index 78f82da..01e38c3 100644 --- a/include/libc-internal.h +++ b/include/libc-internal.h @@ -70,4 +70,28 @@ extern void __init_misc (int, char **, char **); #define PTR_ALIGN_UP(base, size) \ ((__typeof__ (base)) ALIGN_UP ((uintptr_t) (base), (size))) +/* 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)) + #endif /* _LIBC_INTERNAL */ diff --git a/stdio-common/bug21.c b/stdio-common/bug21.c index d22b9c1..ca27272 100644 --- a/stdio-common/bug21.c +++ b/stdio-common/bug21.c @@ -1,4 +1,5 @@ #include <stdio.h> +#include <libc-internal.h> static int do_test (void) @@ -6,7 +7,15 @@ do_test (void) static const char buf[] = " "; char *str; + /* GCC in C99 mode treats %a as the C99 format expecting float *, + but glibc with _GNU_SOURCE treats %as as the GNU allocation + extension, so resulting in "warning: format '%a' expects argument + of type 'float *', but argument 3 has type 'char **'". This + applies to the other %as, %aS and %a[] formats below as well. */ + DIAG_PUSH_NEEDS_COMMENT; + DIAG_IGNORE_NEEDS_COMMENT (4.9, "-Wformat"); int r = sscanf (buf, "%as", &str); + DIAG_POP_NEEDS_COMMENT; printf ("%d %p\n", r, str); return r != -1 || str != NULL; diff --git a/stdio-common/scanf14.c b/stdio-common/scanf14.c index 6ca5c7c..cffccb0 100644 --- a/stdio-common/scanf14.c +++ b/stdio-common/scanf14.c @@ -2,6 +2,7 @@ #include <stdlib.h> #include <string.h> #include <wchar.h> +#include <libc-internal.h> #define FAIL() \ do { \ @@ -23,6 +24,13 @@ main (void) FAIL (); else if (f != 0.25 || memcmp (c, "s x", 3) != 0) FAIL (); + /* GCC in C99 mode treats %a as the C99 format expecting float *, + but glibc with _GNU_SOURCE treats %as as the GNU allocation + extension, so resulting in "warning: format '%a' expects argument + of type 'float *', but argument 3 has type 'char **'". This + applies to the other %as, %aS and %a[] formats below as well. */ + DIAG_PUSH_NEEDS_COMMENT; + DIAG_IGNORE_NEEDS_COMMENT (4.9, "-Wformat"); if (sscanf (" 1.25s x", "%as%2c", &sp, c) != 2) FAIL (); else @@ -32,10 +40,14 @@ main (void) memset (sp, 'x', sizeof "1.25s"); free (sp); } + DIAG_POP_NEEDS_COMMENT; if (sscanf (" 2.25s x", "%las%2c", &d, c) != 2) FAIL (); else if (d != 2.25 || memcmp (c, " x", 2) != 0) FAIL (); + /* See explanation above. */ + DIAG_PUSH_NEEDS_COMMENT; + DIAG_IGNORE_NEEDS_COMMENT (4.9, "-Wformat"); if (sscanf (" 3.25S x", "%4aS%3c", &lsp, c) != 2) FAIL (); else @@ -54,6 +66,7 @@ main (void) memset (sp, 'x', sizeof "4.25"); free (sp); } + DIAG_POP_NEEDS_COMMENT; if (sscanf ("5.25[0-9.] x", "%la[0-9.]%2c", &d, c) != 2) FAIL (); else if (d != 5.25 || memcmp (c, " x", 2) != 0) @@ -82,6 +95,9 @@ main (void) FAIL (); if (fseek (fp, 0, SEEK_SET) != 0) FAIL (); + /* See explanation above. */ + DIAG_PUSH_NEEDS_COMMENT; + DIAG_IGNORE_NEEDS_COMMENT (4.9, "-Wformat"); if (fscanf (fp, "%as%2c", &sp, c) != 2) FAIL (); else @@ -91,11 +107,15 @@ main (void) memset (sp, 'x', sizeof "1.25s"); free (sp); } + DIAG_POP_NEEDS_COMMENT; if (freopen (fname, "r", stdin) == NULL) FAIL (); else { + /* See explanation above. */ + DIAG_PUSH_NEEDS_COMMENT; + DIAG_IGNORE_NEEDS_COMMENT (4.9, "-Wformat"); if (scanf ("%as%2c", &sp, c) != 2) FAIL (); else @@ -105,6 +125,7 @@ main (void) memset (sp, 'x', sizeof "1.25s"); free (sp); } + DIAG_POP_NEEDS_COMMENT; } fclose (fp);