Add macros for diagnostic control, use them in locale/weightwc.h
Commit Message
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
-Wmaybe-uninitialized warnings for some code in locale/weightwc.h. I
put a GCC version in the arguments to DIAG_IGNORE, 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.
The use in weightwc.h has a __GNUC_PREREQ (4, 7) conditional because
4.6 doesn't have -Wmaybe-uninitialized (split out of -Wuninitialized
for 4.7). I didn't include a #else case to disable -Wuninitialized
for 4.6 because I don't know if 4.6 produces these warnings (but if
someone does see them with 4.6, such a #else case should be added -
with 4.6 the version number in the DIAG_IGNORE call).
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 that installed stripped shared libraries are
unchanged by this patch.
2014-11-18 Joseph Myers <joseph@codesourcery.com>
* include/libc-internal.h (DIAG_PUSH): New macro.
(DIAG_POP): Likewise.
(_DIAG_STR1): Likewise.
(_DIAG_STR): Likewise.
(DIAG_IGNORE): Likewise.
* locale/weightwc.h: Include <libc-internal.h>.
(findidx): Disable -Wmaybe-uninitialized around some dereferences
of CP.
Comments
Does anyone else wish to comment on any aspect of this patch
<https://sourceware.org/ml/libc-alpha/2014-11/msg00459.html>?
A couple of other thoughts.
First, the syntax DIAG_IGNORE (-Wmaybe-uninitialized, ...) is too clever and
brittle. Too clever, because in C code it looks like it's subtracting
'uninitialized' from the negative of 'Wmaybe'. Too brittle, because suppose GCC
adds a warning option syntax with commas in it?
"-Wsuggest-attribute=format,noreturn", say? Then DIAG_IGNORE
(-Wsuggest-attribute=format,noreturn, ...) won't work because of C's
tokenization rules. For both of these reasions it would be better to use a
string, e.g., DIAG_IGNORE ("-Wmaybe-uninitialized", ...).
More important, I looked through some of the Gnulib and GNU utilities code that
deals with working around bugs in GCC's diagnostics generation, and
-Wmaybe-uninitialized in particular has caused us so much grief that we
typically silence it in a different way, which I should mention. We use a macro
definition like this:
#ifdef lint
# define IF_LINT(Code) Code
#else
# define IF_LINT(Code) /* empty */
#endif
and then write declarations like this:
off_t size IF_LINT ( = 0);
when GCC wrongly complains that 'size' may be used unininitialized. That way,
people who want to use -Wmaybe-uninitialized can get clean compiles by compiling
with '-Wmaybe-uninitialized -Dlint', and people who don't care about this stuff
can omit the options and get slightly more-efficient code. Obviously there are
some pitfalls in this approach but overall its benefits outweigh its cost for
us. In particular, it lets us carefully isolate the uninitialized-warning bug,
whereas the DIAG_IGNORE approach is more of a blunderbuss that disables the
warning in a too-large region because of GCC's bugs.
On Fri, 21 Nov 2014, Paul Eggert wrote:
> A couple of other thoughts.
>
> First, the syntax DIAG_IGNORE (-Wmaybe-uninitialized, ...) is too clever and
> brittle. Too clever, because in C code it looks like it's subtracting
> 'uninitialized' from the negative of 'Wmaybe'. Too brittle, because suppose
> GCC adds a warning option syntax with commas in it?
> "-Wsuggest-attribute=format,noreturn", say? Then DIAG_IGNORE
> (-Wsuggest-attribute=format,noreturn, ...) won't work because of C's
> tokenization rules. For both of these reasions it would be better to use a
> string, e.g., DIAG_IGNORE ("-Wmaybe-uninitialized", ...).
That seems reasonable, but does anyone else have comments on it?
> off_t size IF_LINT ( = 0);
>
> when GCC wrongly complains that 'size' may be used unininitialized. That way,
> people who want to use -Wmaybe-uninitialized can get clean compiles by
> compiling with '-Wmaybe-uninitialized -Dlint', and people who don't care about
> this stuff can omit the options and get slightly more-efficient code.
> Obviously there are some pitfalls in this approach but overall its benefits
> outweigh its cost for us. In particular, it lets us carefully isolate the
> uninitialized-warning bug, whereas the DIAG_IGNORE approach is more of a
> blunderbuss that disables the warning in a too-large region because of GCC's
> bugs.
I believe it's established glibc practice not to add initializations that
would change generated code to silence warnings. In any case, that
doesn't appear to help for the case this patch relates to; the
uninitialized object '*((void *)&str+4)' is outside the bounds of "UCHAR
str[1]" (this is a case where for these particular findidx calls the
out-of-bounds access is unreachable, but GCC doesn't see that - very much
like -Warray-bounds bogus warnings sometimes seen on some architectures
for one or two files), and initializing within the bounds doesn't stop the
warning.
Joseph Myers wrote:
> it's established glibc practice not to add initializations that
> would change generated code to silence warnings.
Sure, and that's what the IF_LINT macro calls do in coreutils, Emacs, etc.: they
don't alter the code used in production. In practice this leads to code that is
definitely easier to maintain and arguably more resistant against faults than
the DIAG_IGNORE approach being proposed.
> that doesn't appear to help for the case this patch relates to; the
> uninitialized object '*((void *)&str+4)' is outside the bounds of "UCHAR
> str[1]"
If GCC is griping about (say) some component of the array xyz not being
initialized, then in the worst case one can pacify it by initializing the array
in its declaration, e.g., 'int xyz[100] IF_LINT (= {0});'. Of course if GCC is
griping only about xyz[2], one can tighten this by initializing just xyz[2],
which is preferable. In my experience, it's rare to need to do anything so
drastic as to initialize an entire array, as GCC warning bugs generally have to
do with local scalars.
I intend to follow up, but I ran out of time this week.
I'll follow up by the end of Monday.
On Fri, 21 Nov 2014, Paul Eggert wrote:
> Joseph Myers wrote:
> > it's established glibc practice not to add initializations that
> > would change generated code to silence warnings.
>
> Sure, and that's what the IF_LINT macro calls do in coreutils, Emacs, etc.:
> they don't alter the code used in production. In practice this leads to code
> that is definitely easier to maintain and arguably more resistant against
> faults than the DIAG_IGNORE approach being proposed.
The idea for glibc is to use -Werror in production (as per
<https://sourceware.org/ml/libc-alpha/2012-08/msg00404.html> - "It's
especially important to catch subtle regressions when building releases.")
- which means warnings need to be avoided in production as well.
> > that doesn't appear to help for the case this patch relates to; the
> > uninitialized object '*((void *)&str+4)' is outside the bounds of "UCHAR
> > str[1]"
>
> If GCC is griping about (say) some component of the array xyz not being
> initialized, then in the worst case one can pacify it by initializing the
> array in its declaration, e.g., 'int xyz[100] IF_LINT (= {0});'. Of course if
> GCC is griping only about xyz[2], one can tighten this by initializing just
> xyz[2], which is preferable. In my experience, it's rare to need to do
> anything so drastic as to initialize an entire array, as GCC warning bugs
> generally have to do with local scalars.
As I understand this warning, it's warning about str[1] for an array
wint_t str[1] for which only str[0] is valid and only str[0] exists to be
initialized (but the - unreachable - code is, after propagating some
constants, accessing str[1]). So it's not possible to initialize the
element warned about without also increasing the size of the array (and
experimentally, initializing the existing element makes no change to the
warning).
I should add: I do *not* think we should have any sort of -Werror or
diagnostic control policy that involves this level of analysis for each
patch disabling warnings; it should be sufficient that either the warning
appears to be a false positive without an obvious easy and safe fix or
there is some other reason it's hard or inappropriate to fix (e.g. tests
of deprecated interfaces, _FORTIFY_SOURCE checks that also generate
compile-time warnings, etc.).
@@ -70,4 +70,26 @@ 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 _Pragma ("GCC diagnostic push")
+
+/* Pop diagnostic state. */
+#define DIAG_POP _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. 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(option, version) \
+ _Pragma (_DIAG_STR (GCC diagnostic ignored _DIAG_STR (option)))
+
#endif /* _LIBC_INTERNAL */
@@ -19,6 +19,8 @@
#ifndef _WEIGHTWC_H_
#define _WEIGHTWC_H_ 1
+#include <libc-internal.h>
+
/* Find index of weight. */
static inline int32_t __attribute__ ((always_inline))
findidx (const int32_t *table,
@@ -90,6 +92,16 @@ findidx (const int32_t *table,
continue;
}
+ /* Seen on x86_64 (inlined from
+ fnmatch_loop.c:internal_fnwmatch): "'*((void *)&str+4)'
+ may be used uninitialized in this function" (the
+ diagnostic can apply to multiple dereferences of CP, not
+ just one, so all affected dereferences need to be between
+ DIAG_PUSH and DIAG_POP). */
+ DIAG_PUSH;
+#if __GNUC_PREREQ (4, 7)
+ DIAG_IGNORE (-Wmaybe-uninitialized, 4.9);
+#endif
if (cp[nhere - 1] > usrc[nhere -1])
{
cp += 2 * nhere;
@@ -105,6 +117,7 @@ findidx (const int32_t *table,
/* This range matches the next characters. Now find
the offset in the indirect table. */
offset = usrc[nhere - 1] - cp[nhere - 1];
+ DIAG_POP;
*cpp += nhere;
return indirect[-i + offset];