Message ID | 546BB1B1.50000@cs.ucla.edu |
---|---|
State | New |
Headers | show |
> My kneejerk reaction is that attempting to minimize the scope of > disabled warnings will make code harder to read and maintain, and the > costs of minimization may outweigh the benefits. I strongly disagree. We should only ever disable warnings with the tightest possible scope around where we know for sure we need to.
On 11/18/2014 01:18 PM, Roland McGrath wrote: > We should only ever disable warnings with the > tightest possible scope around where we know for sure we need to. In that case neither my patch nor Joseph's should be accepted. If I understand Joseph's patch correctly,the maybe-uninitialized warning needs to be disabled only when dereferencing'cp'. For example, in the expression 'cp[cnt]', the warning should be disabled only for the load from 'cp[cnt]'; it should not be disabled for the load from 'cp' itself, or for the load from 'cnt'. So Joseph's proposed patch doesn't have a tight scope here. It should be possible to tighten itto cover only the places where troublesome dereferences actually occur, e.g., by defining a macro 'NO_MAYBE_UNINITIALIZED_DEREFERENCE (p)' that suppresses the warning while dereferencing the pointer P, by using 'NO_MAYBE_UNINITIALIZED_DEREFERENCE (cp + cnt)' in place of 'cp[cnt]', etc. Also, as I understand it the warning should be disabled only on x86-64, and only if optimization is enabled, as we don't know of problems with other platforms or when optimization is turned off. There may be other ways to tighten the scope, too. However, I'm still not convinced that it's worth the trouble to insist on tight scopes like this. Many of these bogus warnings are due to compiler bugs; I'm afraid we've seen this too often in gnulib, when supporting projects that use -Werror. If we start worrying about tight scopes for workarounds for GCC cry-wolf bugs, we will too easily go down the rabbit hole of maintaining a fragile and evolving list of GCC compiler bugs rather than maintaining glibc.
On Tue, 18 Nov 2014, Paul Eggert wrote: > Also, the distinction between GCC version "4, 7" (which is checked) and > version "4.9" (which is not) is bothersome. Inevitably the unchecked version > numbers will become wrong. If we're going to have a "4.9" at all, we should > check it. Checking it means reviewing if the warning still appears when GCC 5 becomes the minimum supported version for building glibc. The relevant version to compare the 4.9 with is the minimum version for building glibc (which is not currently available in C code in glibc), not the version actually being used to build glibc. > +#pragma GCC diagnostic push No, as stated in <https://sourceware.org/ml/libc-alpha/2014-11/msg00326.html> it's desired to go via internal macros for this. > +/* Seen on x86_64 (inlined from fnmatch_loop.c:internal_fnwmatch): > + "'*((void *)&str+4)' may be used uninitialized in this function". */ > +#if __GNUC_PREREQ (4, 7) && !__GNUC_PREPREQ (4, 10) > +# pragma GCC diagnostic ignored "-Wmaybe-uninitialized" > +#endif No, __GNUC_PREPREQ (4, 10) is wrong. We don't want to introduce lots of build failures every time GCC branches and the mainline version increases. We have no evidence that the warning does not appear with GCC 5; the assertion is simply that it was observed with 4.9.
On Tue, 18 Nov 2014, Paul Eggert wrote: > In that case neither my patch nor Joseph's should be accepted. If I > understand Joseph's patch correctly,the maybe-uninitialized warning needs to > be disabled only when dereferencing'cp'. For example, in the expression > 'cp[cnt]', the warning should be disabled only for the load from 'cp[cnt]'; it > should not be disabled for the load from 'cp' itself, or for the load from > 'cnt'. So Joseph's proposed patch doesn't have a tight scope here. It should > be possible to tighten itto cover only the places where troublesome > dereferences actually occur, e.g., by defining a macro > 'NO_MAYBE_UNINITIALIZED_DEREFERENCE (p)' that suppresses the warning while > dereferencing the pointer P, by using 'NO_MAYBE_UNINITIALIZED_DEREFERENCE (cp > + cnt)' in place of 'cp[cnt]', etc. Since these pragmas work, for middle-end warnings such as these, by tracking a range of locations for which a particular diagnostic is handled in a particular way, I don't expect such a use in statement expressions inside macros to work reliably (and it doesn't work in my tests); the smallest granularity I expect to be reliable is that of whole statements in source files outside of macros (with the pragma calls on separate lines). (My test: #define UNINIT(x) \ ({ \ __typeof (x) __tmp; \ _Pragma ("GCC diagnostic push"); \ _Pragma ("GCC diagnostic ignored \"-Wuninitialized\""); \ __tmp = (x); \ _Pragma ("GCC diagnostic pop"); \ __tmp; \ }) int f (void) { int a, b; return UNINIT (a) + UNINIT (b); } ) > Also, as I understand it the warning should be disabled only on x86-64, and No, rather, it's known it needs disabling on x86_64, but we don't want to cause build failures on all other architectures until their maintainers confirm it's needed there. The reference to x86_64 is simply a hint about where to test when GCC 4.9 is no longer a supported compiler for building glibc. > only if optimization is enabled, as we don't know of problems with other > platforms or when optimization is turned off. glibc has to be built with optimization. Disabling for the relevant block of source lines, on all architectures and for all GCC versions that support the relevant -W option, is a pragmatic choice for how to keep down the number of spurious build failures with default -Werror without hiding warnings unduly (basically, the alternative is the existing practice of -Wno- options for whole source files) or requiring excessive work for architecture maintainers (if we decide it's OK to disable a particular warning, this just needs doing once, and then that case doesn't need thinking about again until the GCC version with which the warning was seen is no longer supported for building glibc).
On 11/18/2014 03:00 PM, Joseph Myers wrote: > Checking it means reviewing if the warning still appears... We don't want to introduce lots of build failures every time GCC branches and the mainline version increases. > In that case, we can do the equivalent of the following instead: #if __GNUC_PREREQ (4, 7) # if !__GNUC_PREPREQ (4, 10) # pragma GCC diagnostic ignored "-Wmaybe-uninitialized" # else # pragma GCC diagnostic warning "-Wmaybe-uninitialized" # endif #endif That is, we can check the "4.9" without causing build failures in 5.0. That should be better than having an unchecked 4.9 sitting in the source. (The above should use _Pragma not #pragma given your other comments, but this doesn't affect the main point here.)
On 11/18/2014 03:16 PM, Joseph Myers wrote: > the smallest granularity I expect to be reliable is that of whole statements in source files outside of macros (with the pragma calls on separate lines) It appears that we've been thinking along parallel lines. I tried some of the tests that you evidently did, and found that GCC 4.9.2 doesn't work even then, in that the granularity isn't reliable even for whole statements, or even for whole functions. This is a regression from GCC 4.8. I filed a bug report here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63943 > Disabling for the relevant block of source lines, on all architectures and for all GCC versions that support the relevant -W option, is a pragmatic choice Yes, and I guess my point is that the pragmatic choice we've taken in Gnulib is to disable diagnostics at the top level, as GCC has too many bugs in this area for us to go on wild goose chases trying to fine-tune diagnostics in smaller areas. I'm skeptical whether it'll be worthwhile for glibc to chase those geese either, at least in the near future.
On Tue, 18 Nov 2014, Paul Eggert wrote: > On 11/18/2014 03:00 PM, Joseph Myers wrote: > > Checking it means reviewing if the warning still appears... We don't want > > to introduce lots of build failures every time GCC branches and the mainline > > version increases. > > > In that case, we can do the equivalent of the following instead: > > #if __GNUC_PREREQ (4, 7) > # if !__GNUC_PREPREQ (4, 10) > # pragma GCC diagnostic ignored "-Wmaybe-uninitialized" > # else > # pragma GCC diagnostic warning "-Wmaybe-uninitialized" > # endif > #endif > > > That is, we can check the "4.9" without causing build failures in 5.0. That > should be better than having an unchecked 4.9 sitting in the source. But the basic point of using -Werror is that people don't look at warnings that scroll by, and shouldn't need to; cluttering build logs up with warnings doesn't help. And the relevant time to check is when 4.9 is no longer supported (one person, or one per architecture, doing the checks then), rather than keeping updating the pragma every time a new GCC version branches. Making the pragma turn the diagnostic into a warning is an interesting idea for how to do the checks at the point when we increase the minimum GCC version - it may be more convenient to turn all the cases where 4.9 is specified into warnings to check them all at once, rather than try deleting them and see which need adding back. I don't think, however, that we need to work out now how we'll manage those checks in future and whether macros making more sophisticated use of the version number might be useful then.
On Tue, 18 Nov 2014, Paul Eggert wrote: > > Disabling for the relevant block of source lines, on all architectures and > > for all GCC versions that support the relevant -W option, is a pragmatic > > choice > > Yes, and I guess my point is that the pragmatic choice we've taken in Gnulib > is to disable diagnostics at the top level, as GCC has too many bugs in this > area for us to go on wild goose chases trying to fine-tune diagnostics in > smaller areas. I'm skeptical whether it'll be worthwhile for glibc to chase > those geese either, at least in the near future. This is the one such optimization-related warning I see on x86_64 (building glibc, rather than the testsuite - but in the testsuite we can be quite free with using dumb code, redundant initialization etc. to avoid warnings, without being at all concerned about resulting inefficiencies) with 4.9. I think there are only a handful I've seen on other architectures. (There may of course be some currently disabled with -Wno- options for whole files in the makefiles.)
Joseph Myers wrote: > the relevant time to check is when 4.9 is no > longer supported If that's the case, I'm having trouble seeing why it's worth our time to have that unchecked "4.9" into the code. If the only time the "4.9" matters is when we bump the min GCC release, we can simply reverify all the version-dependent diagnostics when that happens. That way, we don't need to put the "4.9" into the source code now, which is a good thing because the "4.9" is probably wrong and it's confusing even if it's right.
Joseph Myers wrote:
> This is the one such optimization-related warning I see on x86_64
Wow. You're lucky. I've run into quite a few such warnings with coreutils,
Emacs, etc. Perhaps it's because they enable more GCC warnings.
If this is the only warning we run into with glibc, then it doesn't matter much
how it's addressed. If we start running into this more, though, I expect the
draft approach won't scale well.
Joseph Myers <joseph@codesourcery.com> writes:
> This is the one such optimization-related warning I see on x86_64
This is the only other case I see:
res_send.c:794:8: warning: 'resplen' may be used uninitialized in this function [-Wmaybe-uninitialized]
Andreas.
On Tue, 18 Nov 2014, Paul Eggert wrote: > Joseph Myers wrote: > > the relevant time to check is when 4.9 is no > > longer supported > > If that's the case, I'm having trouble seeing why it's worth our time to have > that unchecked "4.9" into the code. If the only time the "4.9" matters is > when we bump the min GCC release, we can simply reverify all the > version-dependent diagnostics when that happens. That way, we don't need to The point is it's something you can grep for so that only a subset of the diagnostic disabling needs checking each time the minimum GCC version increases.
On Wed, 19 Nov 2014, Andreas Schwab wrote: > Joseph Myers <joseph@codesourcery.com> writes: > > > This is the one such optimization-related warning I see on x86_64 > > This is the only other case I see: > > res_send.c:794:8: warning: 'resplen' may be used uninitialized in this function [-Wmaybe-uninitialized] I've seen that on other architectures. My proposal is to get the build and test clean with -Werror (or -Werror -Wno-error=undef if the -Wundef fixes aren't all in on time) for a single (architecture, compiler) pair, before adding the configuration support for enabling -Werror by default with --disable-werror to disable it and leaving it to people seeing other warnings to fix them or add the macros calling the pragmas as appropriate.
Joseph Myers wrote: > The point is it's something you can grep for so that only a subset of the > diagnostic disabling needs checking each time the minimum GCC version > increases. This worries me, because it implies these macros will be so commonly used that the cost in adding (and more importantly, having developers read) GCC version numbers every time the macros are used is worth the benefit of checking only a subset of the uses once in a blue moon. If these macros are rarely used, the version numbers won't help much; and if the macros are used often I expect we will have so many other problems that the version numbers won't help much.
On Fri, 21 Nov 2014, Paul Eggert wrote: > Joseph Myers wrote: > > The point is it's something you can grep for so that only a subset of the > > diagnostic disabling needs checking each time the minimum GCC version > > increases. > > This worries me, because it implies these macros will be so commonly used that > the cost in adding (and more importantly, having developers read) GCC version > numbers every time the macros are used is worth the benefit of checking only a > subset of the uses once in a blue moon. If these macros are rarely used, the > version numbers won't help much; and if the macros are used often I expect we > will have so many other problems that the version numbers won't help much. Even with a few uses, it's still helpful to be able to check for new GCC versions incrementally rather than requiring all the checks for all pragma uses to be done at once, or the state of which have been checked to be tracked externally - as soon as there are pragmas for warnings seen on multiple architectures, it may not be convenient for the person increasing the required GCC version to check whether all the warnings are still applicable, and so the version numbers mean the state of the partial transition is visible in the source tree.
diff --git a/ChangeLog b/ChangeLog index 37b1459..ab9dc88 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +2014-11-18 Paul Eggert <eggert@cs.ucla.edu> + + * locale/weightwc.h (findidx): Disable -Wmaybe-uninitialized. + 2014-11-18 Roland McGrath <roland@hack.frob.com> * nptl/createthread.c: New file. diff --git a/locale/weightwc.h b/locale/weightwc.h index 0f70b00..383d34e 100644 --- a/locale/weightwc.h +++ b/locale/weightwc.h @@ -19,6 +19,14 @@ #ifndef _WEIGHTWC_H_ #define _WEIGHTWC_H_ 1 +#pragma GCC diagnostic push + +/* Seen on x86_64 (inlined from fnmatch_loop.c:internal_fnwmatch): + "'*((void *)&str+4)' may be used uninitialized in this function". */ +#if __GNUC_PREREQ (4, 7) && !__GNUC_PREPREQ (4, 10) +# pragma GCC diagnostic ignored "-Wmaybe-uninitialized" +#endif + /* Find index of weight. */ static inline int32_t __attribute__ ((always_inline)) findidx (const int32_t *table, @@ -115,4 +123,6 @@ findidx (const int32_t *table, return 0x43219876; } +#pragma GCC diagnostic pop + #endif /* weightwc.h */