diff mbox

-Og bug?

Message ID 8738k83x3n.fsf@kepler.schwinge.homeip.net
State New
Headers show

Commit Message

Thomas Schwinge Jan. 28, 2014, 4:11 p.m. UTC
Hi!

On Tue, 28 Jan 2014 06:52:30 -0800, Ian Lance Taylor <iant@google.com> wrote:
> On Tue, Jan 28, 2014 at 6:36 AM, Thomas Schwinge
> <thomas@codesourcery.com> wrote:
> > Avoid "'dc' may be uninitialized" warning.
> >
> >         libiberty/
> >         * cp-demangle.c (d_demangle_callback): Put __builtin_unreachable
> >         in place, to help the compiler.

For my own education: why is this not considered a GCC trunk bug?  It is
xgcc/cc1 which is coming up with this (bogus?) warning, but only for -Og
and not for -O0, -O1, etc.?

> > --- libiberty/cp-demangle.c
> > +++ libiberty/cp-demangle.c
> > @@ -5824,6 +5824,8 @@ d_demangle_callback (const char *mangled, int options,
> >                           NULL);
> >         d_advance (&di, strlen (d_str (&di)));
> >         break;
> > +      default:
> > +       __builtin_unreachable ();
> 
> You can't call __builtin_unreachable in this code, because libiberty
> in stage 1 will be compiled by the host compiler and
> __builtin_unreachable is specific to GCC.

Right, thanks for catching that.

> This patch is OK if you call abort instead of __builtin_unreachable.

As soon as I'm clear that this is not in fact a GCC bug, I'll commit the
following.  <stdlib.h> already is being included.  Source code comment
snatched from regex.c.

Avoid "'dc' may be uninitialized" warning.

	libiberty/
	* cp-demangle.c (d_demangle_callback): Put an abort call in
	place, to help the compiler.



Grüße,
 Thomas

Comments

Ian Lance Taylor Jan. 28, 2014, 5:12 p.m. UTC | #1
On Tue, Jan 28, 2014 at 8:11 AM, Thomas Schwinge
<thomas@codesourcery.com> wrote:
> On Tue, 28 Jan 2014 06:52:30 -0800, Ian Lance Taylor <iant@google.com> wrote:
>> On Tue, Jan 28, 2014 at 6:36 AM, Thomas Schwinge
>> <thomas@codesourcery.com> wrote:
>> > Avoid "'dc' may be uninitialized" warning.
>> >
>> >         libiberty/
>> >         * cp-demangle.c (d_demangle_callback): Put __builtin_unreachable
>> >         in place, to help the compiler.
>
> For my own education: why is this not considered a GCC trunk bug?  It is
> xgcc/cc1 which is coming up with this (bogus?) warning, but only for -Og
> and not for -O0, -O1, etc.?

I don't really have an opinion on whether this is a bug in GCC or
not.  Since libiberty is compiled by other compilers, I think your
cp-demangle.c patch is reasonable and appropriate either way.

In particular, it's not a bug for the compiler to consider the
possibility that type may take on a value not named in the enum.
C/C++ impose no restrictions on values of enum type.  It's valid to
write code that stores a value that is not an enum constant into a
variable of enum type, so it's reasonable for the compiler to consider
the possibility, even though we can clearly see that it can not
happen.

I don't know why the compiler reports a different warning for -O1 and
-Og.  I encourage you to reduce the code into a standalone test case
and file a bug report.

Ian
Thomas Schwinge Jan. 28, 2014, 9:10 p.m. UTC | #2
Hi!

On Tue, 28 Jan 2014 09:12:44 -0800, Ian Lance Taylor <iant@google.com> wrote:
> On Tue, Jan 28, 2014 at 8:11 AM, Thomas Schwinge
> <thomas@codesourcery.com> wrote:
> > On Tue, 28 Jan 2014 06:52:30 -0800, Ian Lance Taylor <iant@google.com> wrote:
> >> On Tue, Jan 28, 2014 at 6:36 AM, Thomas Schwinge
> >> <thomas@codesourcery.com> wrote:
> >> > Avoid "'dc' may be uninitialized" warning.
> >> >
> >> >         libiberty/
> >> >         * cp-demangle.c (d_demangle_callback): Put __builtin_unreachable
> >> >         in place, to help the compiler.
> >
> > For my own education: why is this not considered a GCC trunk bug?  It is
> > xgcc/cc1 which is coming up with this (bogus?) warning, but only for -Og
> > and not for -O0, -O1, etc.?
> 
> I don't really have an opinion on whether this is a bug in GCC or
> not.  Since libiberty is compiled by other compilers, I think your
> cp-demangle.c patch is reasonable and appropriate either way.
> 
> In particular, it's not a bug for the compiler to consider the
> possibility that type may take on a value not named in the enum.
> C/C++ impose no restrictions on values of enum type.  It's valid to
> write code that stores a value that is not an enum constant into a
> variable of enum type, so it's reasonable for the compiler to consider
> the possibility, even though we can clearly see that it can not
> happen.

OK, I agree to all of that, but I'd assume that if the compiler doesn't
do such value tracking to see whether all cases have been covered, it
also souldn't emit such possibly unitialized warning, to not cause false
positive warnings.

> I don't know why the compiler reports a different warning for -O1 and
> -Og.  I encourage you to reduce the code into a standalone test case
> and file a bug report.

<http://gcc.gnu.org/PR59970>.  Will try to continue with that one, but at
low priority.


Grüße,
 Thomas
Ian Lance Taylor Jan. 28, 2014, 9:24 p.m. UTC | #3
`On Tue, Jan 28, 2014 at 1:10 PM, Thomas Schwinge
<thomas@codesourcery.com> wrote:
>
> OK, I agree to all of that, but I'd assume that if the compiler doesn't
> do such value tracking to see whether all cases have been covered, it
> also souldn't emit such possibly unitialized warning, to not cause false
> positive warnings.

The -Wuninitialized warning is full of false positives.

It is the canonical example of why warnings that are driven by
optimizations are difficult for users in practice.

Ian
Richard Biener Jan. 29, 2014, 9:37 a.m. UTC | #4
On Tue, Jan 28, 2014 at 10:24 PM, Ian Lance Taylor <iant@google.com> wrote:
> `On Tue, Jan 28, 2014 at 1:10 PM, Thomas Schwinge
> <thomas@codesourcery.com> wrote:
>>
>> OK, I agree to all of that, but I'd assume that if the compiler doesn't
>> do such value tracking to see whether all cases have been covered, it
>> also souldn't emit such possibly unitialized warning, to not cause false
>> positive warnings.
>
> The -Wuninitialized warning is full of false positives.
>
> It is the canonical example of why warnings that are driven by
> optimizations are difficult for users in practice.

Indeed.  In this case it's of course the "optimistic" data-flow done by
the -Wuninit pass - if it were to assume that a value is initialized if
it cannot prove it isn't then we'd get no false positives but also a lot
of false negatives.  Currently if it cannot prove it is initialized on a path
the pass assumes it is uninitialized on it.

As always you could do both dataflow kinds and add an extra "maybe"
before cases where both analyses do not agree.

Note that the current "maybe" is supposed to mean that there exists
a path to the use where the value seems to be(!) uninitialized.  In contrast
to there exists a path to the use where the value _is_ uninitialized or
even "all paths to the use have the value uninitialized" (the cases where
no maybe is emitted).

Richard.

> Ian
diff mbox

Patch

--- libiberty/cp-demangle.c
+++ libiberty/cp-demangle.c
@@ -5824,6 +5824,8 @@  d_demangle_callback (const char *mangled, int options,
 			  NULL);
 	d_advance (&di, strlen (d_str (&di)));
 	break;
+      default:
+	abort (); /* We have listed all the cases.  */
       }
 
     /* If DMGL_PARAMS is set, then if we didn't consume the entire