Re: [PATCH] fix #19444 - build failures with -O1 due to -Wmaybe-uninitialized

[Martin Sebor <msebor at gmail dot com>]

On 01/13/2016 10:50 AM, Mike Frysinger wrote:
> On 13 Jan 2016 10:02, Martin Sebor wrote:
> > In his comments on the bug, Carlos suggests to fix these instances
> > of false positives and to get -O1 to work.  The attached patch
> > does just that.  In the patch, to minimize the impact of the
> > (otherwise unnecessary) initialization, rather than initializing
> > them for all the code paths, I reduced the scope of the local
> > variables that are subject to the warning and added the redundant
> > initialization only for the problem code paths.  This led to more
> > changes that would otherwise be required but resulted in code
> > that's easier to follow.
>
> any time code is changed to address warnings, especially "harmless" ones,
> the first question is "has the compiled output changed".  if gcc produces
> worse code at -O2, then this is probably not the right direction.

I did look at the code emitted for sysdeps/ieee754/dbl-64/e_jn.c
on x86_64, both with the added initialization and with the
default case with __builtin_unreachable.

The code changes in small but but IMO not significant ways with
either approach.  Aside from a few changes in the used general
purpose registers, the patch results in three more SSE2 register
to register moves and a few more bytes of padding.  The default
case approach results in bigger overall changes to the emitted
code but (AFACT) fewer added instructions.

FWIW, between the two approaches, my general preference is to
initialize variables rather than adding special annotation.
It makes code cleaner and easier to follow, and in tricky cases
it takes the "what if?" question off the table.  In the case of
the simple __ieee754_jn function, if adding the default case
with __builtin_unreachable is preferable, that's fine with me
too.

In the other cases, such as do_sincos_1() in
sysdeps/ieee754/dbl-64/s_sin.c, there's no case and switch
statement to add __builtin_unreachable to.  Other similar
functions in this file already initialize the retval local
variable, so doing it consistently makes sense.

> > The patch also adds -Wno-error=maybe-uninitialized to the warning
> > options when -O1 or lower is set in CFLAGS to prevent these false
> > positives from causing build failures.  This change renders the
> > changes above strictly unnecessary.  I include both since I think
> > both are worthwhile but I can remove one or the other if others
> > have a different preference.
>
> i'm in favor of this myself.  we clearly do not test -O1 as the normal
> course of things, so it'll be perpetually broken.
> -mike

Thanks
Martin