This is the mail archive of the mailing list for the glibc project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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

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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]