Add macros for diagnostic control, use for scanf %a tests
Commit Message
[Differences relative to version 1
<https://sourceware.org/ml/libc-alpha/2014-11/msg00459.html>: macros
are now named DIAG_*_NEEDS_COMMENT; the option name is now a string
argument to the macro rather than being stringized; the GCC version
number is now the first argument to DIAG_IGNORE_NEEDS_COMMENT rather
than the second, to reduce the risk that greppability would require
long source lines given the new longer macro name; the example uses of
the macros are now in scanf tests.]
In <https://sourceware.org/ml/libc-alpha/2014-11/msg00326.html>,
Roland requested internal macros for use of "#pragma GCC diagnostic".
This patch adds such macros and uses them to disable -Wformat warnings
for some code testing GNU scanf %as where GCC expects C99 scanf %a
(several other stdio tests currently use -Wno-format to disable
warnings). Limitations in GCC's diagnostic pragmas require separate
macros before and after the code generating the warnings, rather than
a single macro taking that code as an argument.
The macros are named DIAG_*_NEEDS_COMMENT to emphasise to reviewers
the need for a comment accompanying any use of them (such comments may
however just appear once for several uses of the macros for the same
issue in the same file). I put a GCC version in the arguments to
DIAG_IGNORE_NEEDS_COMMENT, as that seems something useful to grep for
when obsoleting support for an old GCC version and needing to decide
if warning-disabling code is still relevant.
These macros should be usable for replacing existing -Wno-* use in
makefiles (as also suggested by Roland), though I have no plans to
work on that (only on use of the macros in cases where warnings are
currently present that need disabling to use -Werror).
Tested for x86_64.
2014-11-25 Joseph Myers <joseph@codesourcery.com>
* include/libc-internal.h (DIAG_PUSH_NEEDS_COMMENT): New macro.
(DIAG_POP_NEEDS_COMMENT): Likewise.
(_DIAG_STR1): Likewise.
(_DIAG_STR): Likewise.
(DIAG_IGNORE_NEEDS_COMMENT): Likewise.
* stdio-common/bug21.c: Include <libc-internal.h>.
(do_test): Disable -Wformat around call to sscanf.
* stdio-common/scanf14.c: Include <libc-internal.h>.
(main): Disable -Wformat around some calls to scanf functions.
Comments
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. */
@@ -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 */
@@ -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;
@@ -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);