[PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Andrew_Pinski]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

Since Doug mentioned it would ICE without checking enabled 
(http://gcc.gnu.org/ml/gcc-patches/2007-01/msg00172.html), it seems the 
right thing to just ICE and then watch out for more fall out.  Yes right 
now we know of a testcase where before this patch we will pass but after 
we will now ICE but it seems like a good idea to get this ICEing before we 
get more testcases where we ICE with checking disable.  (I posted the 
reduced testcase for this problem at 
http://gcc.gnu.org/ml/gcc-patches/2007-01/msg00209.html ).

OK? Bootstrapped and tested on i686-linux-gnu with no regressions.

Thanks,
Andrew Pinski

ChangeLog:

        * typeck.c (comptypes): Call error instead of warning for
        "canonical types differ for identical types".  And then
        call gcc_unreachable.
        Likewise for "same canonical type node for different types" case.


[-- Attachment #2: makeinternal_error.diff.txt --]
[-- Type: text/plain, Size: 1192 bytes --]

Index: typeck.c
===================================================================
--- typeck.c	(revision 120428)
+++ typeck.c	(working copy)
@@ -1121,22 +1121,22 @@ comptypes (tree t1, tree t2, int strict)
 	      /* The two types are structurally equivalent, but their
 		 canonical types were different. This is a failure of the
 		 canonical type propagation code.*/
-	      warning(0,
-		      "canonical types differ for identical types %T and %T", 
-		      t1, t2);
+	      error ("canonical types differ for identical types %T and %T", 
+		     t1, t2);
 	      debug_tree (t1);
 	      debug_tree (t2);
+	      gcc_unreachable ();
 	    }
 	  else if (!result && TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2))
 	    {
 	      /* Two types are structurally different, but the canonical
 		 types are the same. This means we were over-eager in
 		 assigning canonical types. */
-	      warning (0, 
-		       "same canonical type node for different types %T and %T",
-		       t1, t2);
+	      error ("same canonical type node for different types %T and %T",
+		     t1, t2);
 	      debug_tree (t1);
 	      debug_tree (t2);
+	      gcc_unreachable ();
 	    }
 	  
 	  return result;

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Mike Stump]

On Jan 3, 2007, at 9:30 PM, Andrew_Pinski@PlayStation.Sony.Com wrote:
> Since Doug mentioned it would ICE without checking enabled
> (http://gcc.gnu.org/ml/gcc-patches/2007-01/msg00172.html), it seems  
> the
> right thing to just ICE and then watch out for more fall out.

No, that's wrong.  If enable checking is off, we normally don't run  
this code, so changing it in that case, isn't interesting.  Further,  
when we normally do run that code, checking it on.  We want soft fail  
in that case, and your patch removes that feature.

> Yes right now we know of a testcase where before this patch we will  
> pass

Pass?  I get:

mrs2 $ ./xgcc -B./ -x c++ -S t.c
t.c: In instantiation of 'CentX<3>':
t.c:8:   instantiated from here
t.c:6: warning: canonical types differ for identical types Mesh<3>  
and Mesh<3>

Is this what you meant by pass?

If so, well, that was the design intent.  To be able to compile up,  
say an entire Linux distribution, have it build (not error out), and  
then go back and review the logs for any of these `warnings'.  If any  
are found, fix them, and then repeat.

When the next compiler is released, we plan on asking users (or, they  
might find out via word of mouth or google) if running the canonical  
verifier fixes their problem, if it does, they have an instant work  
around for any bug in this area, and we can quickly know that the  
area to fix is type canonicalization.

> OK?

No.

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Andrew_Pinski]

Mike Stump <mrs@apple.com> wrote on 01/04/2007 11:23:26 AM:

> When the next compiler is released, we plan on asking users (or, they 
> might find out via word of mouth or google) if running the canonical 
> verifier fixes their problem, if it does, they have an instant work 
> around for any bug in this area, and we can quickly know that the 
> area to fix is type canonicalization.

Except two things,
One: you cannot turn off the warning without turning off all warnings.
Two: when you turn off all the warnings, you still get gabarbe to the 
screen.

Also this is an internal verifier and not one that is verifier of the C++ 
code.

It seems better to ICE rather than just warn as we trying to catch 
problems early
on, rather than going crazy when people start really testing the compiler.

One more thing, we have lots of verifiers inside GCC which we turn on/off 
while
development.

Maybe the code should be written as:

#ifdef ENABLE_CHECKING
              /* The two types are structurally equivalent, but their
                 canonical types were different. This is a failure of the
                 canonical type propagation code.*/
              error ("canonical types differ for identical types %T and 
%T", 
                     t1, t2);
              debug_tree (t1);
              debug_tree (t2);
              gcc_unreachable ();
#else
              warning(0,
                      "canonical types differ for identical types %T and 
%T", 
                      t1, t2);
#endif

And then we don't have to worry about the weird debugging trees getting in 
the way.

Thanks,
Andrew Pinski

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Doug Gregor]

On 1/4/07, Andrew_Pinski@playstation.sony.com
> Maybe the code should be written as:
>
> #ifdef ENABLE_CHECKING
>               /* The two types are structurally equivalent, but their
>                  canonical types were different. This is a failure of the
>                  canonical type propagation code.*/
>               error ("canonical types differ for identical types %T and
> %T",
>                      t1, t2);
>               debug_tree (t1);
>               debug_tree (t2);
>               gcc_unreachable ();
> #else
>               warning(0,
>                       "canonical types differ for identical types %T and
> %T",
>                       t1, t2);
> #endif
>
> And then we don't have to worry about the weird debugging trees getting in
> the way.

That's not a bad solution... ICEing when we run into a problem and
ENABLE_CHECKING is defined means that we'll catch more bugs earlier,
which is probably a good thing, and release compilers
(--disable-checking) will still have the graceful degradation behavior
with the warning.

  Cheers,
  Doug

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Mike Stump]

On Jan 4, 2007, at 12:59 PM, Andrew_Pinski@PlayStation.Sony.Com wrote:
> One: you cannot turn off the warning without turning off all warnings.

And is that a feature, or a bug?  Sounds kinda like a feature to me.

> Two: when you turn off all the warnings, you still get gabarbe to the
> screen.

I can live with that.  If someone wanted to conditionalize the two  
tree dumpers on warnings coming out, that'd be better of course.

> It seems better to ICE rather than just warn as we trying to catch  
> problems early on,

Consider there are 20 bugs a person could find for us building an  
entire linux world.  Ad I engineered it, they get to do the whole  
build, and report the 20 translation units that produced the warning  
and report (at the same time) that when those 20 are fixed, there are  
no more bugs in the entire build.

If we did it your way, they would get a compiler from us, do a build,  
it would die, the would report it, they would wait, we'd fix it, the  
would download a new compiler, build it, install it, and rebuild the  
world, find the next bug, and so on.

In your scheme they download 20 times, build the compiler 20 times  
and they do 20 world builds.  In addition, they have to wait for us  
20 times, and they have to allocate 20 chunks of time to do 20  
separate tests.  There isn't any visibility into how many bugs there  
will be in the end, until the very last bug is fixed.

In my scheme, one download, one world build, one test, one bug  
report.  Find 5 engineers that run world builds, and ask them what  
scheme they would rather do.  Let us know the results.  I'm gonna go  
out on a limb and say that our's would rather do 1 build, but I can  
ask if you don't want to take my word for it.

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Andrew_Pinski]

Mike Stump <mrs@apple.com> wrote on 01/04/2007 02:36:26 PM:

> On Jan 4, 2007, at 12:59 PM, Andrew_Pinski@PlayStation.Sony.Com wrote:
> > One: you cannot turn off the warning without turning off all warnings.
> 
> And is that a feature, or a bug?  Sounds kinda like a feature to me.

It would be a feature if the user could fix their code if it was broken
but their code is not the issue here but GCC is broken.  So users get a
warning which is bogus to them and not helpful at all.  What they will do
is just move to an older version of GCC and give up on the newer ones. 
This
is why this needs to be an internal error and not a warning.  I don't 
think we
really need to warn at all even without checking enabled.
 
> > Two: when you turn off all the warnings, you still get gabarbe to the
> > screen.
> 
> I can live with that.  If someone wanted to conditionalize the two 
> tree dumpers on warnings coming out, that'd be better of course.

Considering this is all internal to GCC rather than an issue with people's 
code.
I rather not warn at all because people might think their code is broken 
when
instead GCC is broken.

The real question is why was this a warning with junk after it in the 
first place,
it seems wrong to be a warning which the user is not able to resolve at 
all.



To an user this warning is not helpful at all since it says nothing about 
their code
except GCC is broken internal so what can the user do, nothing except 
report a bug.  They
cannot turn off just that warning.

I am thinking about applying this patch as obvious and then letting people 
file more bugs
about problems internally to GCC.


-- Pinski

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Daniel Jacobowitz]

On Thu, Jan 04, 2007 at 03:42:36PM -0800, Andrew_Pinski@PlayStation.Sony.Com wrote:
> It would be a feature if the user could fix their code if it was broken
> but their code is not the issue here but GCC is broken.  So users get a
> warning which is bogus to them and not helpful at all.  What they will do
> is just move to an older version of GCC and give up on the newer ones. 

I can only conclude that you did not read or think about anything
that Mike wrote.  Please try again.

> The real question is why was this a warning with junk after it in the 
> first place,
> it seems wrong to be a warning which the user is not able to resolve at 
> all.

So why not fix that instead?  Format the extra info into string buffers
and use %s.

> I am thinking about applying this patch as obvious and then letting people 
> file more bugs
> about problems internally to GCC.

Do not do that.

A patch which draws immediate disagreement is _NEVER_ obvious, no
matter how thoroughly you ignore the response.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Andrew Pinski]

> 
> On Thu, Jan 04, 2007 at 03:42:36PM -0800, Andrew_Pinski@PlayStation.Sony.Com wrote:
> Do not do that.
> 
> A patch which draws immediate disagreement is _NEVER_ obvious, no
> matter how thoroughly you ignore the response.

Except what Mike is talking about is not really an issue we should
be worrying about right now because again this is an internal check.
If GCC is broken, why try to continue?  This is just like any other
internal check inside GCC we have.  Do you want to continue to compile
when a RTL instruction is invalid for a target and then get wrong code
which is harder to debug?  I don't think so.  Then why have a warning
which is not really helpful at all to users saying GCC is broken and
might miscompile their programs.  Really this should have been an 
internal compiler error to begin with and not a warning.

Also what Mike is talking about is doing a compile farm for a world build
which is a different issue and seperate issue.  It should not be an issue
with a patch which actually just corrects a mistake of generating a bogus
warning.


Again the warning is useless to the user as it is an internal GCC issue
rather than a problem with their code.  The warning should be turned into
an internal error as it is an internal check and not a problem with the
source that is being compiled.  warning should only be used for problems
with the source being compiled and not internal GCC problems, those should
be internal errors.

Also I am about to fix more internal problems and then post more internal
checkers do you want a warning or an error?  Warnings like this are all but
useless to the user.  Maybe we should have -Winternal-checking but that seems
like a stupid idea since the internal problems need to be fixed inside GCC
and not in the sources that are being compiled.

-- Pinski

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Daniel Jacobowitz]

On Thu, Jan 04, 2007 at 07:00:22PM -0500, Andrew Pinski wrote:
> Except what Mike is talking about is not really an issue we should
> be worrying about right now because again this is an internal check.
> If GCC is broken, why try to continue?

You haven't thought enough about the reason the warning was added as a
warning.  Please go read it again, and then think about it (and don't
copy me about it again).  Thanks.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Andrew_Pinski]

Daniel Jacobowitz <drow@false.org> wrote on 01/04/2007 04:03:39 PM:

> On Thu, Jan 04, 2007 at 07:00:22PM -0500, Andrew Pinski wrote:
> > Except what Mike is talking about is not really an issue we should
> > be worrying about right now because again this is an internal check.
> > If GCC is broken, why try to continue?
> 
> You haven't thought enough about the reason the warning was added as a
> warning.  Please go read it again, and then think about it (and don't
> copy me about it again).  Thanks.

The only thing the warning says to me, is that GCC is broken.  And if an 
user
gets it, what can they do except complain or ignore it.  With an internal
compiler error, they are more willing to complain about that than a 
warning
being there.  If it was an internal error, we would have caught the issue 
with
tramp3d earlier.

The warning is useless to the common user of GCC really.  I don't see why
I am getting push back on a simple change that is actually obviously the 
correct
thing to do for an internal check.  Can I now then change all the internal 
checking
ICEs into warnings because that is what the real issue here?  To me, 
Canonical
types was not thought through enough if this was a warning and the 
reviewer of
the patch should have caught this issue when reviewing the patch in the 
first place.

There are multiple issues with Canonical types right now, since it causes 
memory usage
to go up around 1-2% (in C and other langauge code) which seems like not a 
good thing
when most of the problems are really just internal to the C++ front-end 
rather than 
in generic code.

Thanks,
Andrew Pinski

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Mike Stump]

On Jan 4, 2007, at 3:42 PM, Andrew_Pinski@PlayStation.Sony.Com wrote:
> Mike Stump <mrs@apple.com> wrote on 01/04/2007 02:36:26 PM:
>
>> On Jan 4, 2007, at 12:59 PM, Andrew_Pinski@PlayStation.Sony.Com  
>> wrote:
>>> One: you cannot turn off the warning without turning off all  
>>> warnings.
>>
>> And is that a feature, or a bug?  Sounds kinda like a feature to me.
>
> It would be a feature if the user could fix their code if it was  
> broken
> but their code is not the issue here but GCC is broken.

Correct.

> So users get a warning

Correct.

> which is bogus

We can reword the warning anyway you want, further, we can make it a  
note if you prefer.  Does the wording, "note: internal consistency  
failure in type canonicalization, recovering" rock your world?  If so,  
let's change the wording if you don't like the present wording.

> to them and not helpful at all.

Go poll 20 users, ask them what is more useful to them, a compiler  
that core dumps immediately when trying to compile their software, or  
one that compiles it.  Tell us what your results are.

I'd be happy to go poll 20 people if you think they will claim that  
failing to compile is better than compiling.


> What they will do is just move to an older version of GCC and give  
> up on the newer ones.

And how does this differ from a compiler that ICEs?

> This is why this needs to be an internal error and not a warning.

Explain that slowly again, I missed your entire argument in favor of  
an ICE.

> I rather not warn at all because people might think their code is  
> broken
> when instead GCC is broken.

So, you object to the spelling.  We look forward to any enhancements  
to the wording you'd care to contribute.

> The real question is why was this a warning with junk after it in the
> first place, it seems wrong to be a warning which the user is not  
> able to resolve at all.

I'm happy to explain this again.  The answer is so that they can file  
a problem report for it, and so that we can trivially know that the  
problem is with type canonicalization.

> I am thinking about applying this patch as obvious

I'm happy for you to do that, if I'm wrong and you're right.

On Jan 4, 2007, at 4:00 PM, Andrew Pinski wrote:
> Except what Mike is talking about is not really an issue we should
> be worrying about right now

So, testing isn't something we should worry about now?  Nor, large  
scale testing on large source bases?  Why is it important to avoid  
testing now?


> If GCC is broken, why try to continue?

What part of my previous explanation was unclear?

> This is just like any other internal check inside GCC we have.

It could be.  It need not be.  In our design for this one, we choose  
to use software fault tolerance.  99.9% of the other checks, don't.   
If you want to argue that software is no place to practice fault  
tolerance, fine, do that.  Feel free to cite references if you want.   
We don't have to, but, for the transition hump, I do think it is  
better to offer fault tolerance.

> Do you want to continue to compile when a RTL instruction is invalid  
> for a target

Yes, why.

> and then get wrong code

Trivially, software fault tolerance is about correctly handling  
faults, not replacing one fault with a harder to diagnose fault.  To  
correctly practice this art, one would necessarily get good code, not  
wrong code.

> which is harder to debug?

Having a compiler tell you that that it failed an internal consistency  
check is easier to debug.  That is what the code does.

> Then why have a warning which is not really helpful at all to users  
> saying GCC is broken and might miscompile

Why diagnose internal consistency problems, well, because it makes for  
faster/better software development.  Why practice software fault  
tolerance?  To reduce the occurrences of software faults.  Why is that  
good?  Because users like software that works as opposed to software  
that faults.

> their programs.  Really this should have been an internal compiler  
> error to begin with and not a warning.

In software fault tolerance, producing a fault for error recovery  
isn't valid, the only valid option it to correctly compile the code,  
if we ICE, we can't do that.

> Also what Mike is talking about is doing a compile farm for a world  
> build
> which is a different issue and seperate issue.

No, gcc-4.3 is undergoing testing now by people that download and  
build and install it.

> Again the warning is useless to the user as it is an internal GCC  
> issue
> rather than a problem with their code.

Ok, please design a better system that the users will like, that will  
get them to file a bug report with us, and that will compile their  
code.  Do up those patches and propose that.

> Also I am about to fix more internal problems and then post more  
> internal
> checkers do you want a warning or an error?

If you feel that software fault tolerance is appropriate, then I'd  
encourage it.  As CPUs get faster, there will be (should be) more  
software fault tolerance in the world.

Ideally, it would be nice it when we took the fault, we wrapped up the  
bug report, squirted it into the bug database, and didn't say a thing  
to the user.  We can't do that, yet.



On Jan 4, 2007, at 4:16 PM, Andrew_Pinski@PlayStation.Sony.Com wrote:
> The only thing the warning says to me, is that GCC is broken.

Excellent, that's what we wanted to convey.

> And if an user gets it, what can they do except complain or ignore  
> it.  With an internal compiler error, they are more willing to  
> complain about that than a warning being there.

History has shown to me that users will file bug reports.  I've done  
this before.

> If it was an internal error, we would have caught the issue  with  
> tramp3d earlier.

Ok, but catching it in 1 day v 3 days isn't very interesting to me.   
It was caught, long before the release of the 4.3 compiler.

> The warning is useless to the common user of GCC really.

A common user isn't downloading and using gcc-4.3.  Why is an ICE is  
more useful to them then compiling?  Do you know of any users that say  
that?

> I don't see why I am getting push back on a simple change

Because you've violated a design principal we thought was important  
enough to engineer in.

> Can I now then change all the internal checking ICEs into warnings

Yes, if you make them all correctly compile with no fault.  I look  
forward to your contribution.

> There are multiple issues with Canonical types right now, since it  
> causes memory usage to go up around 1-2% (in C and other langauge  
> code) which seems like not a good thing when most of the problems  
> are really just internal to the C++ front-end rather than in generic  
> code.

We care about compilation speed more than memory usage.  A compile  
that is 10x faster is more interesting that one that is 1 or 2%  
smaller.  Do you favor a compiler that is 10x  slower if we can make  
it 1% smaller?  If so, why?

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Brooks Moses]

Mike Stump wrote:
> On Jan 4, 2007, at 3:42 PM, Andrew_Pinski@PlayStation.Sony.Com wrote:
>> This is just like any other internal check inside GCC we have.
> 
> It could be.  It need not be.  In our design for this one, we choose  
> to use software fault tolerance.  99.9% of the other checks, don't.   
> If you want to argue that software is no place to practice fault  
> tolerance, fine, do that.  Feel free to cite references if you want.   
> We don't have to, but, for the transition hump, I do think it is  
> better to offer fault tolerance.

There's one thing that I'm not sure I'm understanding here, in trying to 
follow this discussion.

Normally, on a random compiler ICE, the presence of the condition that 
caused the ICE is an indication that something has happened that does 
not fit our understanding of how the code works, and thus we are in a 
situation where we have no reason to expect that the generated results 
will be correct -- and, in fact, chances are pretty good that it will be 
incorrect and buggy.

Thus, in all of those cases, there _is_ no fault tolerance, regardless 
of whether the ICE is an error or a warning -- in either case, usable 
results are not produced.  The distinction is in how we tell the user of 
this failure, and how clear we make it that the results are unusable. 
An ICE and no output makes this blantantly clear; a warning and 
probably-buggy output (which masquerades as a valid executable) makes it 
far less clear, and it's possible that some users will overlook the 
failure entirely.

In that case, an ICE and error-out is almost certainly the correct answer.

My impression from your argument is that this case is not like that; 
that here we know that a failure to pass this particular internal check 
will never result in invalid code.  And thus a warning is appropriate 
here, because the user will still get valid usable code if they ignore 
the warning.

Is this a correct impression?  I haven't seen any mention of this 
distinction, but it seems to me that it's the only way that the argument 
for a warning instead of a hard error would make sense.

- Brooks

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Gabriel Dos Reis]

Andrew_Pinski@PlayStation.Sony.Com writes:

| Since Doug mentioned it would ICE without checking enabled 
| (http://gcc.gnu.org/ml/gcc-patches/2007-01/msg00172.html), it seems the 
| right thing to just ICE and then watch out for more fall out.  Yes right 
| now we know of a testcase where before this patch we will pass but after 
| we will now ICE but it seems like a good idea to get this ICEing before we 
| get more testcases where we ICE with checking disable.  (I posted the 
| reduced testcase for this problem at 
| http://gcc.gnu.org/ml/gcc-patches/2007-01/msg00209.html ).
| 
| OK? Bootstrapped and tested on i686-linux-gnu with no regressions.
| 
| Thanks,
| Andrew Pinski
| 
| ChangeLog:
| 
|         * typeck.c (comptypes): Call error instead of warning for

Calling error() is wrong -- there is no error in the user code.

You can either gcc_assert(), or sorry(), or fatal() if you want the
notification be unambiguous.

-- Gaby

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Gabriel Dos Reis]

Mike Stump <mrs@apple.com> writes:

| On Jan 3, 2007, at 9:30 PM, Andrew_Pinski@PlayStation.Sony.Com wrote:
| > Since Doug mentioned it would ICE without checking enabled
| > (http://gcc.gnu.org/ml/gcc-patches/2007-01/msg00172.html), it seems
| > the
| > right thing to just ICE and then watch out for more fall out.
| 
| No, that's wrong.  If enable checking is off, we normally don't run
| this code, so changing it in that case, isn't interesting.  Further,
| when we normally do run that code, checking it on.  We want soft fail
| in that case, and your patch removes that feature.
| 
| > Yes right now we know of a testcase where before this patch we will
| > pass
| 
| Pass?  I get:
| 
| mrs2 $ ./xgcc -B./ -x c++ -S t.c
| t.c: In instantiation of 'CentX<3>':
| t.c:8:   instantiated from here
| t.c:6: warning: canonical types differ for identical types Mesh<3>
| and Mesh<3>
| 
| Is this what you meant by pass?
| 
| If so, well, that was the design intent.

Now that I think about it, warning() is also wrong, because it leds
user believe something is wrong with her code, while in fact it is the
compiler which is defective.  We can invent a new diagnotic category
for this kind of thing or sorry().

-- Gaby

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Gabriel Dos Reis]

Andrew Pinski <pinskia@physics.uc.edu> writes:

[...]

| Again the warning is useless to the user as it is an internal GCC issue
| rather than a problem with their code.

Labeling the diagnotics "warning: " is indeed confusing; I should have
caught that earlier.  Still the diagnotic is useful to us, however
useless you think it is for user.  So we want to know about it.

Consequently, I think warning() or error() is NOT a good thing.  We
should either issue sorry() or invent a new diagnostic category -- we
only need an enumerator and a forwarding function for that.

-- Gaby

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Gabriel Dos Reis]

Andrew_Pinski@PlayStation.Sony.Com writes:

[...]

| The warning is useless to the common user of GCC really.  I don't see why
| I am getting push back on a simple change that is actually obviously the 
| correct
| thing to do for an internal check.

"obvious" means different thing for different people.

Please, invent a new non-fatal diagnostic category 
("internal consistency check: "?) and resubmit your patch -- I'll
review it.

-- Gaby

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Doug Gregor]

On 1/4/07, Brooks Moses <brooks.moses@codesourcery.com> wrote:
> My impression from your argument is that this case is not like that;
> that here we know that a failure to pass this particular internal check
> will never result in invalid code.  And thus a warning is appropriate
> here, because the user will still get valid usable code if they ignore
> the warning.
>
> Is this a correct impression?  I haven't seen any mention of this
> distinction, but it seems to me that it's the only way that the argument
> for a warning instead of a hard error would make sense.

Yes, your impression is correct.

The canonical types implementation is a compile-time optimization that
makes comparing types for equality (e.g., is "int*" the same as
"my_int_t*"?) faster. Previously, we would have to look at the
structure of the types to determine if they are equivalent; with
canonical types, we only do a single pointer comparison. However, the
code that looks at the structure of types is still needed for some
comparisons, so it's still available for us to use when checking.

As it stands now, there is a parameter "verify-canonical-types". When
it is zero (the default for --disable-checking builds), we trust the
canonical type system to be correct and get our 5-10% speedup on
template-heavy code. When it is one (the default for --enable-checking
builds), we compare the results of the older, slower structure-based
comparison against the results of the canonical type comparison. If
they differ, we complain. Then, we can recover from the failure of the
canonical-types system by reporting the result that the
structure-based comparison determined.

  Cheers,
  Doug

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Doug Gregor]

On 05 Jan 2007 07:44:01 +0100, Gabriel Dos Reis
<gdr@integrable-solutions.net> wrote:
> Please, invent a new non-fatal diagnostic category
> ("internal consistency check: "?) and resubmit your patch -- I'll
> review it.

Good idea.

  Cheers,
  Doug

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Richard Kenner]

> Please, invent a new non-fatal diagnostic category ("internal
> consistency check: "?) and resubmit your patch -- I'll review it.

I've been following this discussion without any opinions so far, but I like
that approach: that way we have plenty of opportunities to decide how to
handle that set of diagnostics, including via command-line options or
doing different things for release and development branches, or ...

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Richard Guenther]

On 1/5/07, Doug Gregor <doug.gregor@gmail.com> wrote:
> On 1/4/07, Brooks Moses <brooks.moses@codesourcery.com> wrote:
> > My impression from your argument is that this case is not like that;
> > that here we know that a failure to pass this particular internal check
> > will never result in invalid code.  And thus a warning is appropriate
> > here, because the user will still get valid usable code if they ignore
> > the warning.
> >
> > Is this a correct impression?  I haven't seen any mention of this
> > distinction, but it seems to me that it's the only way that the argument
> > for a warning instead of a hard error would make sense.
>
> Yes, your impression is correct.
>
> The canonical types implementation is a compile-time optimization that
> makes comparing types for equality (e.g., is "int*" the same as
> "my_int_t*"?) faster. Previously, we would have to look at the
> structure of the types to determine if they are equivalent; with
> canonical types, we only do a single pointer comparison. However, the
> code that looks at the structure of types is still needed for some
> comparisons, so it's still available for us to use when checking.
>
> As it stands now, there is a parameter "verify-canonical-types". When
> it is zero (the default for --disable-checking builds), we trust the
> canonical type system to be correct and get our 5-10% speedup on
> template-heavy code. When it is one (the default for --enable-checking
> builds), we compare the results of the older, slower structure-based
> comparison against the results of the canonical type comparison. If
> they differ, we complain. Then, we can recover from the failure of the
> canonical-types system by reporting the result that the
> structure-based comparison determined.

So for --disable-checking builds we can get the wrong answer for
T1 == T2 and so generate wrong code?  In this case the error should
be fatal, so we get each error reported and not get silent failures
at the time 4.3 is released.

Richard.

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Doug Gregor]

On 1/5/07, Richard Guenther <richard.guenther@gmail.com> wrote:
> So for --disable-checking builds we can get the wrong answer for
> T1 == T2 and so generate wrong code?  In this case the error should
> be fatal, so we get each error reported and not get silent failures
> at the time 4.3 is released.

I might be able to come up with a twisted example where we get wrong
code from a wrong answer with canonical types... more likely, we'll
just error or ICE on legal code.
When users  report such problems, we ask them to add "--param
verify-canonical-types 1"... if that fixes the problem, they can
continue to get work done and we can fix the problem.

  Cheers,
  Doug

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Mike Stump]

On Jan 5, 2007, at 6:20 AM, Richard Guenther wrote:
> So for --disable-checking builds we can get the wrong answer for
> T1 == T2 and so generate wrong code?

There are two cases, one the does checking, and one that doesn't.  -- 
disable-checking merely sets the default for the checking code but  
that can be further refined by the user.

If there is no bug in the compiler, the right answer is always  
obtained, without checking or with checking.

If there is a bug in the compiler, the right answer is always  
obtained with checking.  The wrong answer is obtained without  
checking, and there is no warning/error and also no way to know that  
the the bug existed.

> In this case the error should be fatal,

Can't, see above.

> so we get each error reported and not get silent failures at the  
> time 4.3 is released.

One cannot achieve compilation time benefits while still running the  
checking code, they are fundamentally at odds with each other.  You  
can either have good code in the face of bugs or a faster compiler,  
pick one.

The current plan is to release 4.3 with the checking code off by  
default,  it is hoped by that time that all bugs will be found and  
fixed.

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Richard Guenther]

On 1/5/07, Mike Stump <mrs@apple.com> wrote:
> On Jan 5, 2007, at 6:20 AM, Richard Guenther wrote:
> > So for --disable-checking builds we can get the wrong answer for
> > T1 == T2 and so generate wrong code?
>
> There are two cases, one the does checking, and one that doesn't.  --
> disable-checking merely sets the default for the checking code but
> that can be further refined by the user.
>
> If there is no bug in the compiler, the right answer is always
> obtained, without checking or with checking.
>
> If there is a bug in the compiler, the right answer is always
> obtained with checking.  The wrong answer is obtained without
> checking, and there is no warning/error and also no way to know that
> the the bug existed.
>
> > In this case the error should be fatal,
>
> Can't, see above.
>
> > so we get each error reported and not get silent failures at the
> > time 4.3 is released.
>
> One cannot achieve compilation time benefits while still running the
> checking code, they are fundamentally at odds with each other.  You
> can either have good code in the face of bugs or a faster compiler,
> pick one.
>
> The current plan is to release 4.3 with the checking code off by
> default,  it is hoped by that time that all bugs will be found and
> fixed.

Sure - my point is that if honza didn't look at the tramp3d logfiles
we wouldn't have noticed the type comparison mismatch.  If it ICEd
we would.  So if we make it an error/ICE with checking enabled we're
sure people will look into their logfiles because build actually fails.

Richard.

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Eric Botcazou]

> That's not a bad solution... ICEing when we run into a problem and
> ENABLE_CHECKING is defined means that we'll catch more bugs earlier,
> which is probably a good thing, and release compilers
> (--disable-checking) will still have the graceful degradation behavior
> with the warning.

Released compilers are configured with --enable-checking=release nowadays.

-- 
Eric Botcazou

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Andrew Pinski]

On Fri, 2007-01-05 at 11:02 -0800, Mike Stump wrote:
> If there is a bug in the compiler, the right answer is always  
> obtained with checking.  The wrong answer is obtained without  
> checking, and there is no warning/error and also no way to know that  
> the the bug existed.

Right which is why this should not be just a warning for checking.  This
case is that we can recover from but only at expense of compile time
speed.  If we had it as an ICE before, we would have noticed the
problem, the same day the patch went in.  So we get only a warning if
checking defaults or the user explicitly turns on the checking.  So
really most people will not see the ICE at all unless someone tells them
to turn on the checking.  I don't see why this case is special compared
to all other checking code inside GCC, some of which we can recover from
if we wanted to but we don't currently.

Thanks,
Andrew Pinski

Re: [PATCH, C++] Make Canonical ICE instead of just warn when mismatching

[Mark Mitchell]

Gabriel Dos Reis wrote:
> Andrew_Pinski@PlayStation.Sony.Com writes:
> 
> | Since Doug mentioned it would ICE without checking enabled 
> | (http://gcc.gnu.org/ml/gcc-patches/2007-01/msg00172.html), it seems the 
> | right thing to just ICE and then watch out for more fall out.  Yes right 
> | now we know of a testcase where before this patch we will pass but after 
> | we will now ICE but it seems like a good idea to get this ICEing before we 
> | get more testcases where we ICE with checking disable.  (I posted the 
> | reduced testcase for this problem at 
> | http://gcc.gnu.org/ml/gcc-patches/2007-01/msg00209.html ).
> | 
> | OK? Bootstrapped and tested on i686-linux-gnu with no regressions.

I think we should leave this as is for now, and turn it into a sorry()
or internal_error() before the 4.3 release.  For now, it's nice to be
able to work around any failures.  You can file a P1 bug against 4.3 to
make that change, if you like.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713