Add macros for diagnostic control, use them in locale/weightwc.h

  On 11/18/2014 10:04 AM, Joseph Myers wrote:
> +	  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;

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.

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.

How about the attached (untested) patch instead?  This is simpler and 
less intrusive and should scale better.  It disables the diagnostic in 
some places that it doesn't absolutely need to, but it separates 
concerns better and overall I expect it would have a better 
cost-to-benefit ratio.
  

> 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.