[PATCH] Fix devirtualization ICE (PR tree-optimization/59622, take 2)

Richard Biener rguenther@suse.de
Fri Jan 3 09:24:00 GMT 2014


Jakub Jelinek <jakub@redhat.com> wrote:
>On Tue, Dec 31, 2013 at 11:30:12AM +0100, Richard Biener wrote:
>> Meanwhile your patch is ok.
>
>As discussed in the PR, the patch wasn't sufficient, __cxa_pure_virtual
>can appear in the vtable and it has pretty much the same properties
>as __builtin_unreachable (void return value, no arguments, noreturn,
>DCE or cfg cleanup being able to remove anything after it because of
>noreturn).
>
>Additionally, the patch attempts to punt on invalid type changes
>(ODR violations?, apparently none appear during bootstrap/regtest as
>verified by additional logging added there) or inplace changes of
>gimple_call_flags that would require some cleanups caller isn't
>expected to
>do (again, doesn't happen during bootstrap/regtest).
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

I'd rather do nothing in the !inplace case, it's rare enough to not care.

Also there is nothing wrong with wrong types et al - we have a perfect mechanism for dealing with this. Change the fndecl but not the fntype.

I'd like to see the code simplified with keeping the above in mind.

Thanks,
Richard.

>2014-01-03  Jakub Jelinek  <jakub@redhat.com>
>
>	PR tree-optimization/59622
>	* gimple-fold.c: Include calls.h.
>	(gimple_fold_call): Fix a typo in message.  Handle
>	__cxa_pure_virtual similarly to __builtin_unreachable.  For
>	inplace punt if gimple_call_flags would change.  Verify
>	that lhs and argument types are compatible.
>
>	* g++.dg/opt/pr59622-2.C: New test.
>
>--- gcc/gimple-fold.c.jj	2013-12-31 12:56:59.000000000 +0100
>+++ gcc/gimple-fold.c	2014-01-02 18:33:51.207921734 +0100
>@@ -51,6 +51,7 @@ along with GCC; see the file COPYING3.
> #include "gimple-pretty-print.h"
> #include "tree-ssa-address.h"
> #include "langhooks.h"
>+#include "calls.h"
> 
> /* Return true when DECL can be referenced from current unit.
>FROM_DECL (if non-null) specify constructor of variable DECL was taken
>from.
>@@ -1167,7 +1168,7 @@ gimple_fold_call (gimple_stmt_iterator *
>                                        (OBJ_TYPE_REF_EXPR (callee)))))
> 	    {
> 	      fprintf (dump_file,
>-		       "Type inheritnace inconsistent devirtualization of ");
>+		       "Type inheritance inconsistent devirtualization of ");
> 	      print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
> 	      fprintf (dump_file, " to ");
> 	      print_generic_expr (dump_file, callee, TDF_SLIM);
>@@ -1184,18 +1185,74 @@ gimple_fold_call (gimple_stmt_iterator *
> 	    = possible_polymorphic_call_targets (callee, &final);
> 	  if (final && targets.length () <= 1)
> 	    {
>+	      tree fndecl;
>+	      int call_flags = gimple_call_flags (stmt), new_flags;
> 	      if (targets.length () == 1)
>+		fndecl = targets[0]->decl;
>+	      else
>+		fndecl = builtin_decl_implicit (BUILT_IN_UNREACHABLE);
>+	      new_flags = flags_from_decl_or_type (fndecl);
>+	      if (inplace)
> 		{
>-		  gimple_call_set_fndecl (stmt, targets[0]->decl);
>-		  changed = true;
>+		  /* For inplace, avoid changes of call flags that
>+		     might require cfg cleanups, EH cleanups, removal
>+		     of vdef/vuses etc.  */
>+		  if ((call_flags ^ new_flags)
>+		      & (ECF_NORETURN | ECF_NOTHROW | ECF_NOVOPS))
>+		    final = false;
>+		  else if ((call_flags & ECF_NOVOPS) == 0)
>+		    {
>+		      if ((call_flags ^ new_flags) & ECF_CONST)
>+			final = false;
>+		      else if (((call_flags & (ECF_PURE | ECF_CONST
>+					       | ECF_NORETURN)) == 0)
>+			       ^ ((new_flags & (ECF_PURE | ECF_CONST
>+						| ECF_NORETURN)) == 0))
>+			final = false;
>+		    }
> 		}
>-	      else if (!inplace)
>+	      if (final)
> 		{
>-		  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;
>+		  tree fntype = gimple_call_fntype (stmt);
>+		  tree dtype = TREE_TYPE (fndecl);
>+		  if (gimple_call_lhs (stmt)
>+		      && !useless_type_conversion_p (TREE_TYPE (fntype),
>+						     TREE_TYPE (dtype)))
>+		    final = false;
>+		  else
>+		    {
>+		      tree t1, t2;
>+		      for (t1 = TYPE_ARG_TYPES (fntype),
>+			   t2 = TYPE_ARG_TYPES (dtype);
>+			   t1 && t2;
>+			   t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2))
>+			if (!useless_type_conversion_p (TREE_VALUE (t2),
>+							TREE_VALUE (t1)))
>+			  break;
>+		      if (t1 || t2)
>+			final = false;
>+		    }
>+
>+		  /* If fndecl (like __builtin_unreachable or
>+		     __cxa_pure_virtual) takes no arguments, doesn't have
>+		     return value and is noreturn, just add the call before
>+		     stmt and DCE will do it's job later on.  */
>+		  if (!final
>+		      && !inplace
>+		      && (new_flags & ECF_NORETURN)
>+		      && VOID_TYPE_P (TREE_TYPE (dtype))
>+		      && TYPE_ARG_TYPES (dtype) == void_list_node)
>+		    {
>+		      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;
>+		    }
>+		}
>+	      if (final)
>+		{
>+		  gimple_call_set_fndecl (stmt, fndecl);
>+		  changed = true;
> 		}
> 	    }
> 	}
>--- gcc/testsuite/g++.dg/opt/pr59622-2.C.jj	2014-01-02
>18:24:30.422832814 +0100
>+++ gcc/testsuite/g++.dg/opt/pr59622-2.C	2014-01-02 18:33:29.052037954
>+0100
>@@ -0,0 +1,21 @@
>+// PR tree-optimization/59622
>+// { dg-do compile }
>+// { dg-options "-O2" }
>+
>+namespace
>+{
>+  struct A
>+  {
>+    A () {}
>+    virtual A *bar (int) = 0;
>+    A *baz (int x) { return bar (x); }
>+  };
>+}
>+
>+A *a;
>+
>+void
>+foo ()
>+{
>+  a->baz (0);
>+}
>
>
>	Jakub




More information about the Gcc-patches mailing list