Re: [PATCH] Fix PR optimization/12085

[Eric Botcazou <ebotcazou at libertysurf dot fr>]

> As you've pointed out, its exceptionally rare that function call type
> checking is abused in this way.  You've argued that your testing has
> revealed not a single example in a complete GCC bootstrap and regression
> test run.  And even in these rare cases, not inlining in a function call
> although a minor pessimization is certainly not the end of the world.
> Inlining at the best of times is a hint, and if its a choice between
> between inlining or not, vs. correctly inlining, correctness obviously
> wins.

Ok, this is convincing.

> If you're worried about being too strict, you might also compare your
> implementation against my similar patch to defend GCC's builtins from
> similar abuse: http://gcc.gnu.org/ml/gcc-patches/2003-09/msg00771.html
> [Note this even mentions PR optimization/12085].

Thanks for the pointer.  It taught me VOID_TYPE_P and now I know where the 
trailing 'void' parameter comes from in Objective-C: it seems that some 
va_arg functions are un-va_arg-ized before being called.  This prompted me 
to write the submitted version of the patch, because the first version 
caused aborts in the objc.dg testsuite.

> I still believe "abort" should be reserved for situations where the
> compiler itself has screwed up.  When the user screws up, ... well thats
> just to be expected.  We could write a diagnostic to a log file explaining
> why we chose not to inline (if we don't already).
>
> Getting an ICE with --enable-checking but not without, should really
> reflect a major problem with the compiler itself, not just a missing
> -pedantic warning message (or some such).

Ok, I'll remove the aborts.

> Again this is your call.  I don't want to force you to support a
> patch you don't want to.  However, I will state that I'm happy to
> stand behind your patch myself should we subsequently encounter any
> problems with it.  I'd already put investigating the patch's possible
> interactions with "always inline" on my TODO list.

Thanks!  I've attached two patches:
- pr12085-2.diff is an updated version (with VOID_TYPE_P and a corrected 
comment) of the first patch,
- pr12085-final.diff is the version with the checks at the right place and 
without aborts.

I'll bootstrap/regtest pr12085-2.diff on mainline.  If this is successfull, 
I'll commit pr12085-final.diff and an updated testcase on mainline.  Does 
this look reasonable to you?

> Would you feel happier with the much simpler variant for 3.3?
>
> +  if (TREE_CODE (TREE_OPERAND (t, 0)) == NOP_EXPR)
> +    return NULL_TREE;

I'd rather not touch the 3.3 branch at all.  This is IMO really not worth it.


Many thanks for your advices and your patience.

--
Eric Botcazou

Index: tree-inline.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree-inline.c,v
retrieving revision 1.88
diff -u -p -r1.88 tree-inline.c
--- tree-inline.c	13 Nov 2003 20:50:40 -0000	1.88
+++ tree-inline.c	7 Dec 2003 21:19:24 -0000
@@ -1325,6 +1325,52 @@ expand_call_inline (tree *tp, int *walk_
       && DECL_SAVED_TREE (DECL_ABSTRACT_ORIGIN (fn)))
     fn = DECL_ABSTRACT_ORIGIN (fn);
 
+  /* We can't inline functions at a calling point where they are viewed
+     with too different a prototype than the actual one, because the
+     calling convention may not be the same on both sides.  */
+  if (TREE_CODE (TREE_OPERAND (t, 0)) == NOP_EXPR)
+    {
+      tree from_ftype = TREE_TYPE (TREE_TYPE (TREE_OPERAND (t, 0)));
+      tree to_ftype = TREE_TYPE (fn);
+
+      if (from_ftype != to_ftype)
+	{
+	  tree from_arg, to_arg;
+
+	  /* If the calling point expects a return value and it is too
+	     different from the one actually returned, don't inline.  */
+	  if (! VOID_TYPE_P (TREE_TYPE (from_ftype))
+	      && TYPE_MODE (TREE_TYPE (from_ftype))
+		 != TYPE_MODE (TREE_TYPE (to_ftype)))
+#if ENABLE_CHECKING
+	    abort();
+#else
+	    return NULL_TREE;
+#endif
+
+	  /* If the calling point doesn't pass at least the correct
+	     number of arguments with the correct modes, don't inline.
+	     Objective-C appears to add a trailing void parameter at
+	     the calling point under certain circumstances.  */
+	  from_arg = TYPE_ARG_TYPES (from_ftype);
+	  to_arg = TYPE_ARG_TYPES (to_ftype);
+
+	  while (to_arg)
+	    {
+	      if (! from_arg
+		  || TYPE_MODE (TREE_VALUE (from_arg))
+		     != TYPE_MODE (TREE_VALUE (to_arg)))
+#if ENABLE_CHECKING
+		abort();
+#else
+		return NULL_TREE;
+#endif
+	      from_arg = TREE_CHAIN (from_arg);
+	      to_arg = TREE_CHAIN (to_arg);
+	    }
+	}
+    }
+
   /* Don't try to inline functions that are not well-suited to
      inlining.  */
   if (!DECL_SAVED_TREE (fn) || !cgraph_inline_p (id->current_decl, fn))

Index: tree-inline.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree-inline.c,v
retrieving revision 1.88
diff -u -p -r1.88 tree-inline.c
--- tree-inline.c	13 Nov 2003 20:50:40 -0000	1.88
+++ tree-inline.c	7 Dec 2003 21:20:48 -0000
@@ -1338,6 +1338,45 @@ expand_call_inline (tree *tp, int *walk_
       return NULL_TREE;
     }
 
+  /* We can't inline functions at a calling point where they are viewed
+     with too different a prototype than the actual one, because the
+     calling convention may not be the same on both sides.  */
+  if (TREE_CODE (TREE_OPERAND (t, 0)) == NOP_EXPR)
+    {
+      tree from_ftype = TREE_TYPE (TREE_TYPE (TREE_OPERAND (t, 0)));
+      tree to_ftype = TREE_TYPE (fn);
+
+      if (from_ftype != to_ftype)
+	{
+	  tree from_arg, to_arg;
+
+	  /* If the calling point expects a return value and it is too
+	     different from the one actually returned, don't inline.  */
+	  if (! VOID_TYPE_P (TREE_TYPE (from_ftype))
+	      && TYPE_MODE (TREE_TYPE (from_ftype))
+		 != TYPE_MODE (TREE_TYPE (to_ftype)))
+	    return NULL_TREE;
+
+	  /* If the calling point doesn't pass at least the correct
+	     number of arguments with the correct modes, don't inline.
+	     Objective-C appears to add a trailing void parameter at
+	     the calling point under certain circumstances.  */
+	  from_arg = TYPE_ARG_TYPES (from_ftype);
+	  to_arg = TYPE_ARG_TYPES (to_ftype);
+
+	  while (to_arg)
+	    {
+	      if (! from_arg
+		  || TYPE_MODE (TREE_VALUE (from_arg))
+		     != TYPE_MODE (TREE_VALUE (to_arg)))
+		return NULL_TREE;
+
+	      from_arg = TREE_CHAIN (from_arg);
+	      to_arg = TREE_CHAIN (to_arg);
+	    }
+	}
+    }
+
   if (! (*lang_hooks.tree_inlining.start_inlining) (fn))
     return NULL_TREE;