Fix devirtualization ICE (PR tree-optimization/59622)

Hi!

Setting gimple_call_set_fndecl of a random method call to
__bultin_unreachable can't work properly, the method call probably
passes in arguments that __builtin_unreachable doesn't expect, and if
the method call has LHS, we'll ICE, because __builtin_unreachable doesn't
have a LHS.  Fixed by not doing anything for fold_stmt_inplace and
for fold_stmt just adding a new __buitin_unreachable () call before the
method call, DCE will DTRT then and we don't have to bother with adding
some dummy stmt to set up the lhs etc.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2013-12-30  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/59622
	* gimple-fold.c (gimple_fold_call): Don't replace OBJ_TYPE_REF
	call fndecl with 0 possible targets with BUILT_IN_UNREACHABLE,
	instead only for !inplace add a __builtin_unreachable () call
	before the call.

	* g++.dg/opt/pr59622.C: New test.


	Jakub

Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>Setting gimple_call_set_fndecl of a random method call to
>__bultin_unreachable can't work properly, the method call probably
>passes in arguments that __builtin_unreachable doesn't expect, and if
>the method call has LHS, we'll ICE, because __builtin_unreachable
>doesn't
>have a LHS.  Fixed by not doing anything for fold_stmt_inplace and
>for fold_stmt just adding a new __buitin_unreachable () call before the
>method call, DCE will DTRT then and we don't have to bother with adding
>some dummy stmt to set up the lhs etc.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

You could re-set the number of arguments and the LHS though.

But with a LHS and a not dead consumer both approaches will probably ice. You'd need to replace all uses with a default def.

Richard.

>2013-12-30  Jakub Jelinek  <jakub@redhat.com>
>
>	PR tree-optimization/59622
>	* gimple-fold.c (gimple_fold_call): Don't replace OBJ_TYPE_REF
>	call fndecl with 0 possible targets with BUILT_IN_UNREACHABLE,
>	instead only for !inplace add a __builtin_unreachable () call
>	before the call.
>
>	* g++.dg/opt/pr59622.C: New test.
>
>--- gcc/gimple-fold.c.jj	2013-12-18 17:32:59.000000000 +0100
>+++ gcc/gimple-fold.c	2013-12-30 11:36:38.093211391 +0100
>@@ -1184,13 +1184,19 @@ gimple_fold_call (gimple_stmt_iterator *
> 	    = possible_polymorphic_call_targets (callee, &final);
> 	  if (final && targets.length () <= 1)
> 	    {
>-	      tree fndecl;
> 	      if (targets.length () == 1)
>-		fndecl = targets[0]->decl;
>-	      else
>-		fndecl = builtin_decl_implicit (BUILT_IN_UNREACHABLE);
>-	      gimple_call_set_fndecl (stmt, fndecl);
>-	      changed = true;
>+		{
>+		  gimple_call_set_fndecl (stmt, targets[0]->decl);
>+		  changed = true;
>+		}
>+	      else if (!inplace)
>+		{
>+		  tree fndecl = builtin_decl_implicit (BUILT_IN_UNREACHABLE);
>+		  gimple new_stmt = gimple_build_call (fndecl, 0);
>+		  gimple_set_location (new_stmt, gimple_location (stmt));
>+		  gsi_insert_before (gsi, new_stmt, GSI_SAME_STMT);
>+		  return true;
>+		}
> 	    }
> 	}
>     }
>--- gcc/testsuite/g++.dg/opt/pr59622.C.jj	2013-12-30 11:44:04.934893747
>+0100
>+++ gcc/testsuite/g++.dg/opt/pr59622.C	2013-12-30 11:43:38.000000000
>+0100
>@@ -0,0 +1,19 @@
>+// PR tree-optimization/59622
>+// { dg-do compile }
>+// { dg-options "-O2" }
>+
>+namespace
>+{
>+  struct A
>+  {
>+    virtual int foo ();
>+    int bar () { return foo (); }
>+  };
>+}
>+
>+int
>+baz ()
>+{
>+  A a;
>+  return a.bar ();
>+}
>
>	Jakub

On Mon, Dec 30, 2013 at 05:56:04PM +0100, Richard Biener wrote:
> Jakub Jelinek <jakub@redhat.com> wrote:
> >Setting gimple_call_set_fndecl of a random method call to
> >__bultin_unreachable can't work properly, the method call probably
> >passes in arguments that __builtin_unreachable doesn't expect, and if
> >the method call has LHS, we'll ICE, because __builtin_unreachable
> >doesn't
> >have a LHS.  Fixed by not doing anything for fold_stmt_inplace and
> >for fold_stmt just adding a new __buitin_unreachable () call before the
> >method call, DCE will DTRT then and we don't have to bother with adding
> >some dummy stmt to set up the lhs etc.
> >
> >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> You could re-set the number of arguments and the LHS though.

While we have gimple_set_num_ops, I'd find that very ugly and apparently
nothing in the compiler does that for calls right now.

> But with a LHS and a not dead consumer both approaches will probably ice.
> You'd need to replace all uses with a default def.

Also, I think it could be quite surprising if fold_stmt changed other stmts
(users of lhs), plus the lhs doesn't have to be in SSA form yet.
One would need to also unlink vdefs and release the vdef SSA_NAME, and not
sure if fold_stmt_inplace callers would be even prepared for the stmt to
become noreturn, and no longer throwing, etc.

What would be the advantages of doing all that over the posted patch?

	Jakub

Jakub Jelinek <jakub@redhat.com> wrote:
>On Mon, Dec 30, 2013 at 05:56:04PM +0100, Richard Biener wrote:
>> Jakub Jelinek <jakub@redhat.com> wrote:
>> >Setting gimple_call_set_fndecl of a random method call to
>> >__bultin_unreachable can't work properly, the method call probably
>> >passes in arguments that __builtin_unreachable doesn't expect, and
>if
>> >the method call has LHS, we'll ICE, because __builtin_unreachable
>> >doesn't
>> >have a LHS.  Fixed by not doing anything for fold_stmt_inplace and
>> >for fold_stmt just adding a new __buitin_unreachable () call before
>the
>> >method call, DCE will DTRT then and we don't have to bother with
>adding
>> >some dummy stmt to set up the lhs etc.
>> >
>> >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>> 
>> You could re-set the number of arguments and the LHS though.
>
>While we have gimple_set_num_ops, I'd find that very ugly and
>apparently
>nothing in the compiler does that for calls right now.
>
>> But with a LHS and a not dead consumer both approaches will probably
>ice.
>> You'd need to replace all uses with a default def.
>
>Also, I think it could be quite surprising if fold_stmt changed other
>stmts
>(users of lhs), plus the lhs doesn't have to be in SSA form yet.

Yes.  Also affecting other statements isn't allowed.

>One would need to also unlink vdefs and release the vdef SSA_NAME, and
>not
>sure if fold_stmt_inplace callers would be even prepared for the stmt
>to
>become noreturn, and no longer throwing, etc.

That I think they have to.  Vdef handling is done by the caller which is required to update_stmt and to remove eh side effects if required.

>What would be the advantages of doing all that over the posted patch?

Not sure why this transform is in fold at all rather than in a pass that expects this kind of control flow changes.

That said, your patch still requires all callers of statement folding to run control flow graph cleanup. That's undesired I think, so moving the transform elsewhere is better IMHO.  I suggest dce.

Richard.

>	Jakub

> 
> Yes.  Also affecting other statements isn't allowed.
> 
> >One would need to also unlink vdefs and release the vdef SSA_NAME, and
> >not
> >sure if fold_stmt_inplace callers would be even prepared for the stmt
> >to
> >become noreturn, and no longer throwing, etc.
> 
> That I think they have to.  Vdef handling is done by the caller which is required to update_stmt and to remove eh side effects if required.
> 
> >What would be the advantages of doing all that over the posted patch?
> 
> Not sure why this transform is in fold at all rather than in a pass that expects this kind of control flow changes.

It was always in fold.  I wonder what happens when you do the same w/o devirt machinery?
I.e. take function pointer to __builtin_unreachable and call it with a return value + parameters?

Honza

Hi,
this is C version
static int (*p) (int a) = (int (*)(int))__builtin_unreachable;
main()
{
  return p(1);
}

Here we get in CCP:
Folding statement: return _4;
Not folded
Folding statement: _4 = p.0_2 (1);
Folded into: _4 = __builtin_unreachable (1);

Removing dead stmt p.0_2 = __builtin_unreachable;

Removing basic block 3
<bb 3>:
return _4;


main ()
{ 
  <bb 2>:
  __builtin_unreachable (1);

}

So fold seems to do the same, but tree-ssa-propagate promptly fixes it up
          /* No point propagating into a stmt whose result is not used,
             but instead we might be able to remove a trivially dead stmt.
             Don't do this when called from VRP, since the SSA_NAME which
             is going to be released could be still referenced in VRP
             ranges.  */
          if (do_dce
              && gimple_get_lhs (stmt)
              && TREE_CODE (gimple_get_lhs (stmt)) == SSA_NAME
              && has_zero_uses (gimple_get_lhs (stmt))
              && !stmt_could_throw_p (stmt)
              && !gimple_has_side_effects (stmt))
            { 
              gimple_stmt_iterator i2;

              if (dump_file && dump_flags & TDF_DETAILS)
                { 
                  fprintf (dump_file, "Removing dead stmt ");
                  print_gimple_stmt (dump_file, stmt, 0, 0);
                  fprintf (dump_file, "\n");
                }

so we do not trigger same ICE, but I think it is more luck than
design?

Honza

On Mon, Dec 30, 2013 at 10:14:16PM +0100, Jan Hubicka wrote:
> this is C version
> static int (*p) (int a) = (int (*)(int))__builtin_unreachable;
> main()
> {
>   return p(1);
> }

Well, that testcase is invalid, as you are calling the function (builtin)
through incompatible function pointer, so all we care there is not ICEing.
Not to mention that the above testcase will not link with -O0.
Plus it is not fold_stmt that folds the above.

IMHO when GCC adds the __builtin_unreachable itself, it shouldn't do the
invalid thing.

That said, fold_stmt callers have to be prepared to handle say a normal
call becoming noreturn call, consider say:

struct A { virtual int foo (); };
struct B : public A { int foo () __attribute__((noreturn)); };
int B::foo () { __builtin_exit (0); }
int bar ()
{
  B b;
  B *p = &b;
  return p->foo ();
}

where fold_stmt changes the OBJ_TYPE_REF based call into B::foo () which
is noreturn.  The bad thing on the folding gimple_fold_call did before the
patch I've posted is mainly that it kept the lhs there even when
__builtin_unreachable has void return type, and very ugly but not fatal
is the passing of extra arguments to the builtin that doesn't take any.

	Jakub

> On Mon, Dec 30, 2013 at 10:14:16PM +0100, Jan Hubicka wrote:
> > this is C version
> > static int (*p) (int a) = (int (*)(int))__builtin_unreachable;
> > main()
> > {
> >   return p(1);
> > }
> 
> Well, that testcase is invalid, as you are calling the function (builtin)
> through incompatible function pointer, so all we care there is not ICEing.
> Not to mention that the above testcase will not link with -O0.
> Plus it is not fold_stmt that folds the above.

Hmm, I would not care much about validity, but indeed the fact that fold_stmt is
not doing this conversion makes me feel better ;)

> where fold_stmt changes the OBJ_TYPE_REF based call into B::foo () which
> is noreturn.  The bad thing on the folding gimple_fold_call did before the
> patch I've posted is mainly that it kept the lhs there even when
> __builtin_unreachable has void return type, and very ugly but not fatal
> is the passing of extra arguments to the builtin that doesn't take any.

Yep, I basically originally added the code for sanity checking (watned to break
programs where GCC fails to identify the correct virtual method) and then
found it is useful in valid testcases because of code specialization.

cgraph_redirect_edge_call_stmt_to_callee can be also used to redirect call
to UNREACHABLE (i.e. via walk_polymorphic_call_targets from cgraphunit.c)
So probably we want to add there a logic updating both return value and
killing the unused arguments (as we may do in gimple fold, I think people
may wire in "abort" call or so into a function pointer taking with incompatible
decl)

Honza
> 
> 	Jakub

Jakub Jelinek <jakub@redhat.com> wrote:
>On Mon, Dec 30, 2013 at 10:14:16PM +0100, Jan Hubicka wrote:
>> this is C version
>> static int (*p) (int a) = (int (*)(int))__builtin_unreachable;
>> main()
>> {
>>   return p(1);
>> }
>
>Well, that testcase is invalid, as you are calling the function
>(builtin)
>through incompatible function pointer, so all we care there is not
>ICEing.
>Not to mention that the above testcase will not link with -O0.
>Plus it is not fold_stmt that folds the above.
>
>IMHO when GCC adds the __builtin_unreachable itself, it shouldn't do
>the
>invalid thing.
>
>That said, fold_stmt callers have to be prepared to handle say a normal
>call becoming noreturn call, consider say:
>
>struct A { virtual int foo (); };
>struct B : public A { int foo () __attribute__((noreturn)); };
>int B::foo () { __builtin_exit (0); }
>int bar ()
>{
>  B b;
>  B *p = &b;
>  return p->foo ();
>}

Is that a valid specialization though?

>where fold_stmt changes the OBJ_TYPE_REF based call into B::foo ()
>which
>is noreturn.

I am quite sure callers of fold_stmt do not schedule cleanup_cfg when it returns true. They mostly do it accidentally.

Richard.

  The bad thing on the folding gimple_fold_call did before
>the
>patch I've posted is mainly that it kept the lhs there even when
>__builtin_unreachable has void return type, and very ugly but not fatal
>is the passing of extra arguments to the builtin that doesn't take any.
>
>	Jakub

On Tue, Dec 31, 2013 at 08:39:02AM +0100, Richard Biener wrote:
> >That said, fold_stmt callers have to be prepared to handle say a normal
> >call becoming noreturn call, consider say:
> >
> >struct A { virtual int foo (); };
> >struct B : public A { int foo () __attribute__((noreturn)); };
> >int B::foo () { __builtin_exit (0); }
> >int bar ()
> >{
> >  B b;
> >  B *p = &b;
> >  return p->foo ();
> >}
> 
> Is that a valid specialization though?

I think so, after all don't we set noreturn attribute automatically
even if it is missing when IPA can prove the function never returns?
If we fold_stmt after that, the above testcase even without explicit
noreturn attribute would need cfg cleanup.

Perhaps gimple_fold_call should punt and not change fndecl if !inplace
if some call flags have changed that would require cfg cleanup, making
at least fold_stmt_inplace callers not having to deal with it, and make
sure fold_stmt callers schedule cleanup_cfg when fold_stmt returns true?

	Jakub

Jakub Jelinek <jakub@redhat.com> wrote:
>On Tue, Dec 31, 2013 at 08:39:02AM +0100, Richard Biener wrote:
>> >That said, fold_stmt callers have to be prepared to handle say a
>normal
>> >call becoming noreturn call, consider say:
>> >
>> >struct A { virtual int foo (); };
>> >struct B : public A { int foo () __attribute__((noreturn)); };
>> >int B::foo () { __builtin_exit (0); }
>> >int bar ()
>> >{
>> >  B b;
>> >  B *p = &b;
>> >  return p->foo ();
>> >}
>> 
>> Is that a valid specialization though?
>
>I think so, after all don't we set noreturn attribute automatically
>even if it is missing when IPA can prove the function never returns?
>If we fold_stmt after that, the above testcase even without explicit
>noreturn attribute would need cfg cleanup.
>
>Perhaps gimple_fold_call should punt and not change fndecl if !inplace
>if some call flags have changed that would require cfg cleanup, making
>at least fold_stmt_inplace callers not having to deal with it, and make
>sure fold_stmt callers schedule cleanup_cfg when fold_stmt returns
>true?

It would be nice to audit callers and finally document what callers are required to do ... I can look at this next week.  I agree that the in place variant should avoid these kind of side-effects.

Meanwhile your patch is ok.

Thanks,
Richard.

>	Jakub

> >I think so, after all don't we set noreturn attribute automatically
> >even if it is missing when IPA can prove the function never returns?
> >If we fold_stmt after that, the above testcase even without explicit
> >noreturn attribute would need cfg cleanup.
> >
> >Perhaps gimple_fold_call should punt and not change fndecl if !inplace
> >if some call flags have changed that would require cfg cleanup, making
> >at least fold_stmt_inplace callers not having to deal with it, and make
> >sure fold_stmt callers schedule cleanup_cfg when fold_stmt returns
> >true?
> 
> It would be nice to audit callers and finally document what callers are required to do ... I can look at this next week.  I agree that the in place variant should avoid these kind of side-effects.

Yep, I think with in-place-fold we really can't do this.
This make me wonder, how we arrange the statement to be folded later if we don't fold
statements we did not touched by a given pass?
Note that with LTO and invalid C++ program, I think devirtualization can return
a function of a different type just because the slots in virtual tables are used
differently in each unit.  We should not ICE here.

Honza
> 
> Meanwhile your patch is ok.
> 
> Thanks,
> Richard.
> 
> >	Jakub
>

Jan Hubicka <hubicka@ucw.cz> wrote:
>> >I think so, after all don't we set noreturn attribute automatically
>> >even if it is missing when IPA can prove the function never returns?
>> >If we fold_stmt after that, the above testcase even without explicit
>> >noreturn attribute would need cfg cleanup.
>> >
>> >Perhaps gimple_fold_call should punt and not change fndecl if
>!inplace
>> >if some call flags have changed that would require cfg cleanup,
>making
>> >at least fold_stmt_inplace callers not having to deal with it, and
>make
>> >sure fold_stmt callers schedule cleanup_cfg when fold_stmt returns
>> >true?
>> 
>> It would be nice to audit callers and finally document what callers
>are required to do ... I can look at this next week.  I agree that the
>in place variant should avoid these kind of side-effects.
>
>Yep, I think with in-place-fold we really can't do this.
>This make me wonder, how we arrange the statement to be folded later if
>we don't fold
>statements we did not touched by a given pass?

We don't fold all statements, that is a general issue. I have old patches that ensure we at least fold all statements after gimplifying. Propagators should fold what they propagate into. That leaves us with lto which could fold each statement after reading it. I've not done this to avoid differences with regarding to the gimplification issue.

Richard.

>Note that with LTO and invalid C++ program, I think devirtualization
>can return
>a function of a different type just because the slots in virtual tables
>are used
>differently in each unit.  We should not ICE here.
>
>Honza
>> 
>> Meanwhile your patch is ok.
>> 
>> Thanks,
>> Richard.
>> 
>> >	Jakub
>>