Patchwork Fix segfault with inlining

login
register
mail settings
Submitter Eric Botcazou
Date Sept. 13, 2013, 2:29 p.m.
Message ID <7802386.q05VISrV2B@polaris>
Download mbox | patch
Permalink /patch/274786/
State New
Headers show

Comments

Eric Botcazou - Sept. 13, 2013, 2:29 p.m.
Hi,

in Ada parameters can be passed by reference: in this case, the address of the 
argument is directly passed to the callee, which dereferences it to access the 
argument; now Ada also enables -fexceptions -fnon-call-exceptions, which means 
that any pointer dereference is seen as the potential source of an exception, 
which can quickly block the optimizer.  That's why we put TREE_THIS_NOTRAP on 
the dereferences associated with a parameter.

This works fine as long as the function isn't inlined, because it may happen 
that the argument is itself the dereference of a pointer, properly guarded by 
a null check on the pointer.  When the function is inlined, the dereference in 
the caller is replaced with that of the callee, which is TREE_THIS_NOTRAP and 
thus can be moved ahead of the null pointer check, for example by LIM.

The patch ensures that this cannot happen by clearing TREE_THIS_NOTRAP in the 
inliner.  I think that this affects only the Ada compiler in practice.

Tested on x86_64-suse-linux, OK for the mainline?


2013-09-13  Eric Botcazou  <ebotcazou@adacore.com>

	* tree-inline.h (struct copy_body_data): Add transform_parameter.
	* tree-inline.c (is_parameter_of): New predicate.
	(remap_gimple_op_r): Do not propagate TREE_THIS_NOTRAP on MEM_REF if
	a parameter has been remapped.
	(copy_tree_body_r): Likewise on INDIRECT_REF and MEM_REF.
	(optimize_inline_calls): Initialize transform_parameter.
	(unsave_expr_now): Likewise.
	(copy_gimple_seq_and_replace_locals): Likewise.
	(tree_function_versioning): Likewise.
	(maybe_inline_call_in_expr): Likewise.


2013-09-13  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/opt27.adb: New test.
	* gnat.dg/opt27_pkg.ad[sb]: New helper.
Richard Guenther - Sept. 16, 2013, 9:28 a.m.
On Fri, Sep 13, 2013 at 4:29 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> in Ada parameters can be passed by reference: in this case, the address of the
> argument is directly passed to the callee, which dereferences it to access the
> argument; now Ada also enables -fexceptions -fnon-call-exceptions, which means
> that any pointer dereference is seen as the potential source of an exception,
> which can quickly block the optimizer.  That's why we put TREE_THIS_NOTRAP on
> the dereferences associated with a parameter.
>
> This works fine as long as the function isn't inlined, because it may happen
> that the argument is itself the dereference of a pointer, properly guarded by
> a null check on the pointer.  When the function is inlined, the dereference in
> the caller is replaced with that of the callee, which is TREE_THIS_NOTRAP and
> thus can be moved ahead of the null pointer check, for example by LIM.
>
> The patch ensures that this cannot happen by clearing TREE_THIS_NOTRAP in the
> inliner.  I think that this affects only the Ada compiler in practice.
>
> Tested on x86_64-suse-linux, OK for the mainline?

I've looked at the C++ testcase

int foo (int &x)
{
  try {
    return x;
  }
  catch (...)
  {
    return 0;
  }
}

which exhibits exactly the behavior you quote - return x is considered throwing
an exception.  The C++ FE doesn't arrange for TREE_THIS_NOTRAP to be
set here (maybe due to this issue you quote?).

Other than that the patch looks reasonable (I suppose you need
is_parameter_of only because as we recursively handle the trees
PARM_DECLs from the destination could already have leaked into
the tree we recurse into?)

Thanks,
Richard.

>
> 2013-09-13  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * tree-inline.h (struct copy_body_data): Add transform_parameter.
>         * tree-inline.c (is_parameter_of): New predicate.
>         (remap_gimple_op_r): Do not propagate TREE_THIS_NOTRAP on MEM_REF if
>         a parameter has been remapped.
>         (copy_tree_body_r): Likewise on INDIRECT_REF and MEM_REF.
>         (optimize_inline_calls): Initialize transform_parameter.
>         (unsave_expr_now): Likewise.
>         (copy_gimple_seq_and_replace_locals): Likewise.
>         (tree_function_versioning): Likewise.
>         (maybe_inline_call_in_expr): Likewise.
>
>
> 2013-09-13  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gnat.dg/opt27.adb: New test.
>         * gnat.dg/opt27_pkg.ad[sb]: New helper.
>
>
> --
> Eric Botcazou
Jason Merrill - Sept. 16, 2013, 7:33 p.m.
On 09/16/2013 05:28 AM, Richard Biener wrote:
> which exhibits exactly the behavior you quote - return x is considered throwing
> an exception.  The C++ FE doesn't arrange for TREE_THIS_NOTRAP to be
> set here (maybe due to this issue you quote?).

I haven't been aware of TREE_THIS_NOTRAP, but we could certainly start 
setting it.

Jason
Eric Botcazou - Sept. 17, 2013, 7:03 a.m.
> I've looked at the C++ testcase
> 
> int foo (int &x)
> {
>   try {
>     return x;
>   }
>   catch (...)
>   {
>     return 0;
>   }
> }
> 
> which exhibits exactly the behavior you quote - return x is considered
> throwing an exception.  The C++ FE doesn't arrange for TREE_THIS_NOTRAP to
> be set here (maybe due to this issue you quote?).

I presume that you compiled with -fnon-call-exceptions?  Otherwise, I don't 
see how something that isn't a call can throw an exception in C++, it should 
be seen at most as possibly trapping, which is less blocking.

> Other than that the patch looks reasonable (I suppose you need
> is_parameter_of only because as we recursively handle the trees
> PARM_DECLs from the destination could already have leaked into
> the tree we recurse into?)

Do you mean that the test on DECL_CONTEXT is superfluous?  Possibly indeed, 
but with nested functions you can have PARM_DECLs of different origins in a 
given function body, although this may be irrelevant for tree-inline.c.
Richard Guenther - Sept. 17, 2013, 8:25 a.m.
On Tue, Sep 17, 2013 at 9:03 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> I've looked at the C++ testcase
>>
>> int foo (int &x)
>> {
>>   try {
>>     return x;
>>   }
>>   catch (...)
>>   {
>>     return 0;
>>   }
>> }
>>
>> which exhibits exactly the behavior you quote - return x is considered
>> throwing an exception.  The C++ FE doesn't arrange for TREE_THIS_NOTRAP to
>> be set here (maybe due to this issue you quote?).
>
> I presume that you compiled with -fnon-call-exceptions?  Otherwise, I don't
> see how something that isn't a call can throw an exception in C++, it should
> be seen at most as possibly trapping, which is less blocking.

Yes, with -fnon-call-exceptions.

>> Other than that the patch looks reasonable (I suppose you need
>> is_parameter_of only because as we recursively handle the trees
>> PARM_DECLs from the destination could already have leaked into
>> the tree we recurse into?)
>
> Do you mean that the test on DECL_CONTEXT is superfluous?  Possibly indeed,
> but with nested functions you can have PARM_DECLs of different origins in a
> given function body, although this may be irrelevant for tree-inline.c.

Yeah, I thought testing for a PARM_DECL should be sufficient?  For
nested functions
references to outer parms should have been lowered via the static
chain at the point
tree-inline.c sees them.

So, if you agree that the DECL_CONTEXT test is superfluous the patch is ok
with the is_parameter_of function removed.

Thanks,
Richard.

> --
> Eric Botcazou
Eric Botcazou - Sept. 17, 2013, 8:42 a.m.
> Yeah, I thought testing for a PARM_DECL should be sufficient?  For
> nested functions references to outer parms should have been lowered via the
> static chain at the point tree-inline.c sees them.

OK for the latter point, but are you sure for the former?  My understanding is 
that we're already in SSA form, so parameters can be represented by SSA_NAMEs 
without defining statements.
Jakub Jelinek - Sept. 17, 2013, 8:47 a.m.
On Fri, Sep 13, 2013 at 04:29:48PM +0200, Eric Botcazou wrote:
> @@ -4748,6 +4774,8 @@ copy_gimple_seq_and_replace_locals (gimp
>    id.transform_call_graph_edges = CB_CGE_DUPLICATE;
>    id.transform_new_cfg = false;
>    id.transform_return_to_modify = false;
> +  id.transform_parameter = false;
> +  id.transform_parameter = false;
>    id.transform_lang_insert_block = NULL;
>  
>    /* Walk the tree once to find local labels.  */

Why are you storing the same thing twice?

	Jakub
Richard Guenther - Sept. 17, 2013, 8:54 a.m.
On Tue, Sep 17, 2013 at 10:42 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Yeah, I thought testing for a PARM_DECL should be sufficient?  For
>> nested functions references to outer parms should have been lowered via the
>> static chain at the point tree-inline.c sees them.
>
> OK for the latter point, but are you sure for the former?  My understanding is
> that we're already in SSA form, so parameters can be represented by SSA_NAMEs
> without defining statements.

That's true...  so you can only simplify is_parameter_of by dropping
the context check.

Richard.

> --
> Eric Botcazou
Eric Botcazou - Sept. 17, 2013, 11:07 a.m.
> That's true...  so you can only simplify is_parameter_of by dropping
> the context check.

OK, thanks, installed with this modification and the fix for the oversight 
spotted by Jakub, after retesting on x86-64/Linux.

Patch

Index: tree-inline.c
===================================================================
--- tree-inline.c	(revision 202431)
+++ tree-inline.c	(working copy)
@@ -751,6 +751,21 @@  copy_gimple_bind (gimple stmt, copy_body
   return new_bind;
 }
 
+/* Return true if DECL is a parameter or a SSA_NAME for a parameter of
+   function FNDECL.  */
+
+static bool
+is_parameter_of (tree decl, tree fndecl)
+{
+  if (TREE_CODE (decl) == SSA_NAME)
+    {
+      decl = SSA_NAME_VAR (decl);
+      if (!decl)
+	return false;
+    }
+
+  return (TREE_CODE (decl) == PARM_DECL && DECL_CONTEXT (decl) == fndecl);
+}
 
 /* Remap the GIMPLE operand pointed to by *TP.  DATA is really a
    'struct walk_stmt_info *'.  DATA->INFO is a 'copy_body_data *'.
@@ -840,20 +855,25 @@  remap_gimple_op_r (tree *tp, int *walk_s
 
       if (TREE_CODE (*tp) == MEM_REF)
 	{
-	  tree ptr = TREE_OPERAND (*tp, 0);
-	  tree type = remap_type (TREE_TYPE (*tp), id);
-	  tree old = *tp;
-
 	  /* We need to re-canonicalize MEM_REFs from inline substitutions
 	     that can happen when a pointer argument is an ADDR_EXPR.
 	     Recurse here manually to allow that.  */
+	  tree ptr = TREE_OPERAND (*tp, 0);
+	  tree type = remap_type (TREE_TYPE (*tp), id);
+	  tree old = *tp;
 	  walk_tree (&ptr, remap_gimple_op_r, data, NULL);
-	  *tp = fold_build2 (MEM_REF, type,
-			     ptr, TREE_OPERAND (*tp, 1));
-	  TREE_THIS_NOTRAP (*tp) = TREE_THIS_NOTRAP (old);
+	  *tp = fold_build2 (MEM_REF, type, ptr, TREE_OPERAND (*tp, 1));
 	  TREE_THIS_VOLATILE (*tp) = TREE_THIS_VOLATILE (old);
 	  TREE_SIDE_EFFECTS (*tp) = TREE_SIDE_EFFECTS (old);
 	  TREE_NO_WARNING (*tp) = TREE_NO_WARNING (old);
+	  /* We cannot propagate the TREE_THIS_NOTRAP flag if we have
+	     remapped a parameter as the property might be valid only
+	     for the parameter itself.  */
+	  if (TREE_THIS_NOTRAP (old)
+	      && (!is_parameter_of (TREE_OPERAND (old, 0), id->src_fn)
+		  || (!id->transform_parameter
+		      && is_parameter_of (ptr, id->dst_fn))))
+	    TREE_THIS_NOTRAP (*tp) = 1;
 	  *walk_subtrees = 0;
 	  return NULL;
 	}
@@ -1043,45 +1063,45 @@  copy_tree_body_r (tree *tp, int *walk_su
 	  /* Get rid of *& from inline substitutions that can happen when a
 	     pointer argument is an ADDR_EXPR.  */
 	  tree decl = TREE_OPERAND (*tp, 0);
-	  tree *n;
-
-	  n = (tree *) pointer_map_contains (id->decl_map, decl);
+	  tree *n = (tree *) pointer_map_contains (id->decl_map, decl);
 	  if (n)
 	    {
-	      tree new_tree;
-	      tree old;
 	      /* If we happen to get an ADDR_EXPR in n->value, strip
 	         it manually here as we'll eventually get ADDR_EXPRs
 		 which lie about their types pointed to.  In this case
 		 build_fold_indirect_ref wouldn't strip the INDIRECT_REF,
 		 but we absolutely rely on that.  As fold_indirect_ref
 	         does other useful transformations, try that first, though.  */
-	      tree type = TREE_TYPE (TREE_TYPE (*n));
-	      if (id->do_not_unshare)
-		new_tree = *n;
-	      else
-		new_tree = unshare_expr (*n);
-	      old = *tp;
-	      *tp = gimple_fold_indirect_ref (new_tree);
+	      tree type = TREE_TYPE (*tp);
+	      tree ptr = id->do_not_unshare ? *n : unshare_expr (*n);
+	      tree old = *tp;
+	      *tp = gimple_fold_indirect_ref (ptr);
 	      if (! *tp)
 	        {
-		  if (TREE_CODE (new_tree) == ADDR_EXPR)
+		  if (TREE_CODE (ptr) == ADDR_EXPR)
 		    {
-		      *tp = fold_indirect_ref_1 (EXPR_LOCATION (new_tree),
-						 type, new_tree);
+		      *tp
+		        = fold_indirect_ref_1 (EXPR_LOCATION (ptr), type, ptr);
 		      /* ???  We should either assert here or build
 			 a VIEW_CONVERT_EXPR instead of blindly leaking
 			 incompatible types to our IL.  */
 		      if (! *tp)
-			*tp = TREE_OPERAND (new_tree, 0);
+			*tp = TREE_OPERAND (ptr, 0);
 		    }
 	          else
 		    {
-	              *tp = build1 (INDIRECT_REF, type, new_tree);
+	              *tp = build1 (INDIRECT_REF, type, ptr);
 		      TREE_THIS_VOLATILE (*tp) = TREE_THIS_VOLATILE (old);
 		      TREE_SIDE_EFFECTS (*tp) = TREE_SIDE_EFFECTS (old);
 		      TREE_READONLY (*tp) = TREE_READONLY (old);
-		      TREE_THIS_NOTRAP (*tp) = TREE_THIS_NOTRAP (old);
+		      /* We cannot propagate the TREE_THIS_NOTRAP flag if we
+			 have remapped a parameter as the property might be
+			 valid only for the parameter itself.  */
+		      if (TREE_THIS_NOTRAP (old)
+			  && (!is_parameter_of (decl, id->src_fn)
+			      || (!id->transform_parameter
+				  && is_parameter_of (ptr, id->dst_fn))))
+		        TREE_THIS_NOTRAP (*tp) = 1;
 		    }
 		}
 	      *walk_subtrees = 0;
@@ -1090,20 +1110,25 @@  copy_tree_body_r (tree *tp, int *walk_su
 	}
       else if (TREE_CODE (*tp) == MEM_REF)
 	{
-	  tree ptr = TREE_OPERAND (*tp, 0);
-	  tree type = remap_type (TREE_TYPE (*tp), id);
-	  tree old = *tp;
-
 	  /* We need to re-canonicalize MEM_REFs from inline substitutions
 	     that can happen when a pointer argument is an ADDR_EXPR.
 	     Recurse here manually to allow that.  */
+	  tree ptr = TREE_OPERAND (*tp, 0);
+	  tree type = remap_type (TREE_TYPE (*tp), id);
+	  tree old = *tp;
 	  walk_tree (&ptr, copy_tree_body_r, data, NULL);
-	  *tp = fold_build2 (MEM_REF, type,
-			     ptr, TREE_OPERAND (*tp, 1));
-	  TREE_THIS_NOTRAP (*tp) = TREE_THIS_NOTRAP (old);
+	  *tp = fold_build2 (MEM_REF, type, ptr, TREE_OPERAND (*tp, 1));
 	  TREE_THIS_VOLATILE (*tp) = TREE_THIS_VOLATILE (old);
 	  TREE_SIDE_EFFECTS (*tp) = TREE_SIDE_EFFECTS (old);
 	  TREE_NO_WARNING (*tp) = TREE_NO_WARNING (old);
+	  /* We cannot propagate the TREE_THIS_NOTRAP flag if we have
+	     remapped a parameter as the property might be valid only
+	     for the parameter itself.  */
+	  if (TREE_THIS_NOTRAP (old)
+	      && (!is_parameter_of (TREE_OPERAND (old, 0), id->src_fn)
+		  || (!id->transform_parameter
+		      && is_parameter_of (ptr, id->dst_fn))))
+	    TREE_THIS_NOTRAP (*tp) = 1;
 	  *walk_subtrees = 0;
 	  return NULL;
 	}
@@ -4443,6 +4468,7 @@  optimize_inline_calls (tree fn)
   id.transform_call_graph_edges = CB_CGE_DUPLICATE;
   id.transform_new_cfg = false;
   id.transform_return_to_modify = true;
+  id.transform_parameter = true;
   id.transform_lang_insert_block = NULL;
   id.statements_to_fold = pointer_set_create ();
 
@@ -4748,6 +4774,8 @@  copy_gimple_seq_and_replace_locals (gimp
   id.transform_call_graph_edges = CB_CGE_DUPLICATE;
   id.transform_new_cfg = false;
   id.transform_return_to_modify = false;
+  id.transform_parameter = false;
+  id.transform_parameter = false;
   id.transform_lang_insert_block = NULL;
 
   /* Walk the tree once to find local labels.  */
@@ -5207,6 +5235,7 @@  tree_function_versioning (tree old_decl,
     = update_clones ? CB_CGE_MOVE_CLONES : CB_CGE_MOVE;
   id.transform_new_cfg = true;
   id.transform_return_to_modify = false;
+  id.transform_parameter = false;
   id.transform_lang_insert_block = NULL;
 
   old_entry_block = ENTRY_BLOCK_PTR_FOR_FUNCTION
@@ -5431,6 +5460,7 @@  maybe_inline_call_in_expr (tree exp)
       id.transform_call_graph_edges = CB_CGE_DUPLICATE;
       id.transform_new_cfg = false;
       id.transform_return_to_modify = true;
+      id.transform_parameter = true;
       id.transform_lang_insert_block = NULL;
 
       /* Make sure not to unshare trees behind the front-end's back
Index: tree-inline.h
===================================================================
--- tree-inline.h	(revision 202431)
+++ tree-inline.h	(working copy)
@@ -97,6 +97,10 @@  typedef struct copy_body_data
      by manipulating the CFG rather than a statement.  */
   bool transform_return_to_modify;
 
+  /* True if the parameters of the source function are transformed.
+     Only true for inlining.  */
+  bool transform_parameter;
+
   /* True if this statement will need to be regimplified.  */
   bool regimplify;