| Message ID | alpine.DEB.2.10.1411181803130.11642@digraph.polyomino.org.uk |
|---|---|
| State | New |
| Headers | show |
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.).
diff --git a/include/libc-internal.h b/include/libc-internal.h index 78f82da..0be89e6 100644 --- a/include/libc-internal.h +++ b/include/libc-internal.h @@ -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 */ diff --git a/locale/weightwc.h b/locale/weightwc.h index 0f70b00..d79bd21 100644 --- a/locale/weightwc.h +++ b/locale/weightwc.h @@ -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];