diff mbox

Fix devirt from dropping lhs with TREE_ADDRESSABLE type on noreturn calls (PR c++/71210)

Message ID 20160520113122.GI28550@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek May 20, 2016, 11:31 a.m. UTC 
Hi!

This is another case in the never ending story of dropping lhs of noreturn
calls when we shouldn't.

Though, in this case, while we can optimize a call to a direct call to
normal [[noreturn]] method, we can also optimize into __cxa_pure_virtual
or __builtin_unreachable.  And in those cases IMHO it is desirable to
not have the lhs, but we should also adjust gimple_call_set_fntype,
because we are now calling something different, we've just reused the
same call stmt for that.

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

2016-05-20  Jakub Jelinek  <jakub@redhat.com>

	PR c++/71210
	* gimple-fold.c (gimple_fold_call): Do not remove lhs of noreturn
	calls if the LHS is variable length or has addressable type.
	If targets[0]->decl is a noreturn call with void return type and
	zero arguments, adjust fntype and remove lhs in that case.

	* g++.dg/opt/pr71210-1.C: New test.
	* g++.dg/opt/pr71210-2.C: New test.


	Jakub

Comments

Marek Polacek May 20, 2016, 11:40 a.m. UTC | #1 
On Fri, May 20, 2016 at 01:31:22PM +0200, Jakub Jelinek wrote:
> Hi!
> 
> This is another case in the never ending story of dropping lhs of noreturn
> calls when we shouldn't.
> 
> Though, in this case, while we can optimize a call to a direct call to
> normal [[noreturn]] method, we can also optimize into __cxa_pure_virtual
> or __builtin_unreachable.  And in those cases IMHO it is desirable to
> not have the lhs, but we should also adjust gimple_call_set_fntype,
> because we are now calling something different, we've just reused the
> same call stmt for that.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2016-05-20  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/71210
> 	* gimple-fold.c (gimple_fold_call): Do not remove lhs of noreturn
> 	calls if the LHS is variable length or has addressable type.
> 	If targets[0]->decl is a noreturn call with void return type and
> 	zero arguments, adjust fntype and remove lhs in that case.
> 
> 	* g++.dg/opt/pr71210-1.C: New test.
> 	* g++.dg/opt/pr71210-2.C: New test.
> 
> --- gcc/gimple-fold.c.jj	2016-05-03 14:12:19.000000000 +0200
> +++ gcc/gimple-fold.c	2016-05-20 10:18:22.818728240 +0200
> @@ -3039,10 +3039,25 @@ gimple_fold_call (gimple_stmt_iterator *
>  		}
>  	      if (targets.length () == 1)
>  		{
> -		  gimple_call_set_fndecl (stmt, targets[0]->decl);
> +		  tree fndecl = targets[0]->decl;
> +		  gimple_call_set_fndecl (stmt, fndecl);
>  		  changed = true;
> +		  /* If changing the call to __cxa_pure_virtual
> +		     or similar noreturn function, adjust gimple_call_fntype
> +		     too.  */
> +		  if ((gimple_call_flags (stmt) & ECF_NORETURN)
> +		      && VOID_TYPE_P (TREE_TYPE (TREE_TYPE (fndecl)))
> +		      && TYPE_ARG_TYPES (TREE_TYPE (fndecl))
> +		      && (TREE_VALUE (TYPE_ARG_TYPES (TREE_TYPE (fndecl)))
> +			  == void_type_node))
> +		    gimple_call_set_fntype (stmt, TREE_TYPE (fndecl));
>  		  /* If the call becomes noreturn, remove the lhs.  */
> -		  if (lhs && (gimple_call_flags (stmt) & ECF_NORETURN))
> +		  if (lhs
> +		      && (gimple_call_flags (stmt) & ECF_NORETURN)
> +		      && (VOID_TYPE_P (TREE_TYPE (gimple_call_fntype (stmt)))
> +			  || ((TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (lhs)))
> +			       == INTEGER_CST)
> +			      && !TREE_ADDRESSABLE (TREE_TYPE (lhs)))))

Do you think it would be worth it to factor out this check into a new
predicate and use it throughout the codebase?

	Marek
Richard Biener May 20, 2016, 11:45 a.m. UTC | #2 
On Fri, 20 May 2016, Jakub Jelinek wrote:

> Hi!
> 
> This is another case in the never ending story of dropping lhs of noreturn
> calls when we shouldn't.
> 
> Though, in this case, while we can optimize a call to a direct call to
> normal [[noreturn]] method, we can also optimize into __cxa_pure_virtual
> or __builtin_unreachable.  And in those cases IMHO it is desirable to
> not have the lhs, but we should also adjust gimple_call_set_fntype,
> because we are now calling something different, we've just reused the
> same call stmt for that.

Hmm, I think for devirt we can unconditionally adjust fntype.  It should
be impossible to create a wrongly typed virtual call in source
(maybe with the help of LTO and some ODR violations).

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

Ok with doing the set_fntype unconditionally.  If you think it's safer
the way you did it that's also ok.

Richard.

> 2016-05-20  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/71210
> 	* gimple-fold.c (gimple_fold_call): Do not remove lhs of noreturn
> 	calls if the LHS is variable length or has addressable type.
> 	If targets[0]->decl is a noreturn call with void return type and
> 	zero arguments, adjust fntype and remove lhs in that case.
> 
> 	* g++.dg/opt/pr71210-1.C: New test.
> 	* g++.dg/opt/pr71210-2.C: New test.
> 
> --- gcc/gimple-fold.c.jj	2016-05-03 14:12:19.000000000 +0200
> +++ gcc/gimple-fold.c	2016-05-20 10:18:22.818728240 +0200
> @@ -3039,10 +3039,25 @@ gimple_fold_call (gimple_stmt_iterator *
>  		}
>  	      if (targets.length () == 1)
>  		{
> -		  gimple_call_set_fndecl (stmt, targets[0]->decl);
> +		  tree fndecl = targets[0]->decl;
> +		  gimple_call_set_fndecl (stmt, fndecl);
>  		  changed = true;
> +		  /* If changing the call to __cxa_pure_virtual
> +		     or similar noreturn function, adjust gimple_call_fntype
> +		     too.  */
> +		  if ((gimple_call_flags (stmt) & ECF_NORETURN)
> +		      && VOID_TYPE_P (TREE_TYPE (TREE_TYPE (fndecl)))
> +		      && TYPE_ARG_TYPES (TREE_TYPE (fndecl))
> +		      && (TREE_VALUE (TYPE_ARG_TYPES (TREE_TYPE (fndecl)))
> +			  == void_type_node))
> +		    gimple_call_set_fntype (stmt, TREE_TYPE (fndecl));
>  		  /* If the call becomes noreturn, remove the lhs.  */
> -		  if (lhs && (gimple_call_flags (stmt) & ECF_NORETURN))
> +		  if (lhs
> +		      && (gimple_call_flags (stmt) & ECF_NORETURN)
> +		      && (VOID_TYPE_P (TREE_TYPE (gimple_call_fntype (stmt)))
> +			  || ((TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (lhs)))
> +			       == INTEGER_CST)
> +			      && !TREE_ADDRESSABLE (TREE_TYPE (lhs)))))
>  		    {
>  		      if (TREE_CODE (lhs) == SSA_NAME)
>  			{
> --- gcc/testsuite/g++.dg/opt/pr71210-1.C.jj	2016-05-20 09:30:13.119724805 +0200
> +++ gcc/testsuite/g++.dg/opt/pr71210-1.C	2016-05-20 09:29:42.000000000 +0200
> @@ -0,0 +1,14 @@
> +// PR c++/71210
> +// { dg-do compile }
> +// { dg-options "-O2" }
> +
> +#include <typeinfo>
> +
> +void f1 (const std::type_info&) __attribute__((noreturn));
> +struct S1 { ~S1 (); };
> +struct S2
> +{
> +  virtual S1 f2 () const { f1 (typeid (*this)); }
> +  S1 f3 () const { return f2 (); }
> +};
> +void f4 () { S2 a; a.f3 (); }
> --- gcc/testsuite/g++.dg/opt/pr71210-2.C.jj	2016-05-20 10:20:00.918402232 +0200
> +++ gcc/testsuite/g++.dg/opt/pr71210-2.C	2016-05-20 10:19:48.000000000 +0200
> @@ -0,0 +1,23 @@
> +// PR c++/71210
> +// { dg-do compile }
> +// { dg-options "-O2" }
> +
> +struct C { int a; int b; C (); ~C (); };
> +
> +namespace
> +{
> +  struct A
> +  {
> +    A () {}
> +    virtual C bar (int) = 0;
> +    C baz (int x) { return bar (x); }
> +  };
> +}
> +
> +A *a;
> +
> +void
> +foo ()
> +{
> +  C c = a->baz (0);
> +}
> 
> 	Jakub
> 
>
Jakub Jelinek May 20, 2016, 11:59 a.m. UTC | #3 
On Fri, May 20, 2016 at 01:40:01PM +0200, Marek Polacek wrote:
> > +		  if (lhs
> > +		      && (gimple_call_flags (stmt) & ECF_NORETURN)
> > +		      && (VOID_TYPE_P (TREE_TYPE (gimple_call_fntype (stmt)))
> > +			  || ((TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (lhs)))
> > +			       == INTEGER_CST)
> > +			      && !TREE_ADDRESSABLE (TREE_TYPE (lhs)))))
> 
> Do you think it would be worth it to factor out this check into a new
> predicate and use it throughout the codebase?

I think it would be worthwhile.  Are you willing to write a patch for this?
Otherwise I can add it to my todo list, but it will take a while.

	Jakub
Richard Biener May 20, 2016, 12:01 p.m. UTC | #4 
On Fri, 20 May 2016, Jakub Jelinek wrote:

> On Fri, May 20, 2016 at 01:40:01PM +0200, Marek Polacek wrote:
> > > +		  if (lhs
> > > +		      && (gimple_call_flags (stmt) & ECF_NORETURN)
> > > +		      && (VOID_TYPE_P (TREE_TYPE (gimple_call_fntype (stmt)))
> > > +			  || ((TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (lhs)))
> > > +			       == INTEGER_CST)
> > > +			      && !TREE_ADDRESSABLE (TREE_TYPE (lhs)))))
> > 
> > Do you think it would be worth it to factor out this check into a new
> > predicate and use it throughout the codebase?
> 
> I think it would be worthwhile.  Are you willing to write a patch for this?
> Otherwise I can add it to my todo list, but it will take a while.

Maybe even make it a maybe_drop_lhs_from_noreturn_call () helper.

Richard.
Marek Polacek May 20, 2016, 12:02 p.m. UTC | #5 
On Fri, May 20, 2016 at 01:59:48PM +0200, Jakub Jelinek wrote:
> On Fri, May 20, 2016 at 01:40:01PM +0200, Marek Polacek wrote:
> > > +		  if (lhs
> > > +		      && (gimple_call_flags (stmt) & ECF_NORETURN)
> > > +		      && (VOID_TYPE_P (TREE_TYPE (gimple_call_fntype (stmt)))
> > > +			  || ((TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (lhs)))
> > > +			       == INTEGER_CST)
> > > +			      && !TREE_ADDRESSABLE (TREE_TYPE (lhs)))))
> > 
> > Do you think it would be worth it to factor out this check into a new
> > predicate and use it throughout the codebase?
> 
> I think it would be worthwhile.  Are you willing to write a patch for this?

Yeah, sure.  can_remove_lhs_p?

	Marek
diff mbox

Patch

--- gcc/gimple-fold.c.jj	2016-05-03 14:12:19.000000000 +0200
+++ gcc/gimple-fold.c	2016-05-20 10:18:22.818728240 +0200
@@ -3039,10 +3039,25 @@  gimple_fold_call (gimple_stmt_iterator *
 		}
 	      if (targets.length () == 1)
 		{
-		  gimple_call_set_fndecl (stmt, targets[0]->decl);
+		  tree fndecl = targets[0]->decl;
+		  gimple_call_set_fndecl (stmt, fndecl);
 		  changed = true;
+		  /* If changing the call to __cxa_pure_virtual
+		     or similar noreturn function, adjust gimple_call_fntype
+		     too.  */
+		  if ((gimple_call_flags (stmt) & ECF_NORETURN)
+		      && VOID_TYPE_P (TREE_TYPE (TREE_TYPE (fndecl)))
+		      && TYPE_ARG_TYPES (TREE_TYPE (fndecl))
+		      && (TREE_VALUE (TYPE_ARG_TYPES (TREE_TYPE (fndecl)))
+			  == void_type_node))
+		    gimple_call_set_fntype (stmt, TREE_TYPE (fndecl));
 		  /* If the call becomes noreturn, remove the lhs.  */
-		  if (lhs && (gimple_call_flags (stmt) & ECF_NORETURN))
+		  if (lhs
+		      && (gimple_call_flags (stmt) & ECF_NORETURN)
+		      && (VOID_TYPE_P (TREE_TYPE (gimple_call_fntype (stmt)))
+			  || ((TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (lhs)))
+			       == INTEGER_CST)
+			      && !TREE_ADDRESSABLE (TREE_TYPE (lhs)))))
 		    {
 		      if (TREE_CODE (lhs) == SSA_NAME)
 			{
--- gcc/testsuite/g++.dg/opt/pr71210-1.C.jj	2016-05-20 09:30:13.119724805 +0200
+++ gcc/testsuite/g++.dg/opt/pr71210-1.C	2016-05-20 09:29:42.000000000 +0200
@@ -0,0 +1,14 @@ 
+// PR c++/71210
+// { dg-do compile }
+// { dg-options "-O2" }
+
+#include <typeinfo>
+
+void f1 (const std::type_info&) __attribute__((noreturn));
+struct S1 { ~S1 (); };
+struct S2
+{
+  virtual S1 f2 () const { f1 (typeid (*this)); }
+  S1 f3 () const { return f2 (); }
+};
+void f4 () { S2 a; a.f3 (); }
--- gcc/testsuite/g++.dg/opt/pr71210-2.C.jj	2016-05-20 10:20:00.918402232 +0200
+++ gcc/testsuite/g++.dg/opt/pr71210-2.C	2016-05-20 10:19:48.000000000 +0200
@@ -0,0 +1,23 @@ 
+// PR c++/71210
+// { dg-do compile }
+// { dg-options "-O2" }
+
+struct C { int a; int b; C (); ~C (); };
+
+namespace
+{
+  struct A
+  {
+    A () {}
+    virtual C bar (int) = 0;
+    C baz (int x) { return bar (x); }
+  };
+}
+
+A *a;
+
+void
+foo ()
+{
+  C c = a->baz (0);
+}