diff mbox series

Do not leak location information during inlining

Message ID 1830303.IuxID5XYVR@polaris
State New
Headers show
Series Do not leak location information during inlining | expand

Commit Message

Eric Botcazou June 19, 2018, 10:21 p.m. UTC 
Hi,

the Ada compiler uses small functions defined in its runtime to implement 
various intrinsic operations and it always inlines them, even at -O0.  But it 
doesn't want location information from the runtime files to appear in the 
debug info so it puts DECL_IGNORED_P on these functions.  final.c already 
knows not to generate location information for DECL_IGNORED_P functions when 
they are standalone but that's not the case for the inliner, i.e. it leaks 
location information from these functions where they are inlined.

The attached patch is aimed at preventing this from happening by explicitly 
putting input_location on the inlined bodies, adjusting the locations on debug 
statements, as well as a few other adjustements.

Tested (GCC and GDB) on x86-64/Linux, OK for the mainline?


2018-06-19  Eric Botcazou  <ebotcazou@adacore.com>

	* tree-inline.c (remap_location): New function extracted from...
	(remap_gimple_stmt): Force input_location on the copy if DECL_IGNORED_P
	is set on the source function.
	(copy_edges_for_bb): Add ID parameter.  Copy locus and remap block of
	edges if DECL_IGNORED_P isn't set on the source function.
	(copy_phis_for_bb): ...here.  Do not copy locus of arguments if
	DECL_IGNORED_P is set on the source function.
	(maybe_move_debug_stmts_to_successors): Also reset the locus of the
	debug statement when resetting its value.
	(copy_cfg_body): Adjust call to copy_edges_for_bb.
	(expand_call_inline): Copy the locus of the call onto the assignment of
	the return value, if any.  Use local variable in more cases.


2018-06-19  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/debug15.adb: New test.

Comments

Richard Biener June 20, 2018, 8:10 a.m. UTC | #1 
On Wed, Jun 20, 2018 at 12:25 AM Eric Botcazou <ebotcazou@adacore.com> wrote:
>
> Hi,
>
> the Ada compiler uses small functions defined in its runtime to implement
> various intrinsic operations and it always inlines them, even at -O0.  But it
> doesn't want location information from the runtime files to appear in the
> debug info so it puts DECL_IGNORED_P on these functions.  final.c already
> knows not to generate location information for DECL_IGNORED_P functions when
> they are standalone but that's not the case for the inliner, i.e. it leaks
> location information from these functions where they are inlined.
>
> The attached patch is aimed at preventing this from happening by explicitly
> putting input_location on the inlined bodies, adjusting the locations on debug
> statements, as well as a few other adjustements.
>
> Tested (GCC and GDB) on x86-64/Linux, OK for the mainline?

There are fixes in this patch together with the new functionality -
can you split
those out?  I'm thinking of the copy_edges_for_bb hunks as well as
the expand_call_inline ones.

As for the real change I'd prefer to not repeat the DECL_INGORED_P checks
but instead have a id->remap_location flag and a id->call_location that
locations are remapped to (to not sprinkle more input_location uses
in the code).

Also in case the call location is UNKNOWN_LOCATION do we at least want
to use a location that has the encompaning BLOCK recorded?

Thanks,
Richard.

>
> 2018-06-19  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * tree-inline.c (remap_location): New function extracted from...
>         (remap_gimple_stmt): Force input_location on the copy if DECL_IGNORED_P
>         is set on the source function.
>         (copy_edges_for_bb): Add ID parameter.  Copy locus and remap block of
>         edges if DECL_IGNORED_P isn't set on the source function.
>         (copy_phis_for_bb): ...here.  Do not copy locus of arguments if
>         DECL_IGNORED_P is set on the source function.
>         (maybe_move_debug_stmts_to_successors): Also reset the locus of the
>         debug statement when resetting its value.
>         (copy_cfg_body): Adjust call to copy_edges_for_bb.
>         (expand_call_inline): Copy the locus of the call onto the assignment of
>         the return value, if any.  Use local variable in more cases.
>
>
> 2018-06-19  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gnat.dg/debug15.adb: New test.
>
> --
> Eric Botcazou
Eric Botcazou June 20, 2018, 2:36 p.m. UTC | #2 
> There are fixes in this patch together with the new functionality -
> can you split
> those out?  I'm thinking of the copy_edges_for_bb hunks as well as
> the expand_call_inline ones.

Like this?
 

	* tree-inline.c (copy_edges_for_bb): Minor tweak.
	(maybe_move_debug_stmts_to_successors): Also reset the locus of the
	debug statement when resetting its value.
	(expand_call_inline): Copy the locus of the call onto the assignment of
	the return value, if any.  Use local variable in more cases.
Richard Biener June 21, 2018, 8:02 a.m. UTC | #3 
On Wed, Jun 20, 2018 at 4:37 PM Eric Botcazou <ebotcazou@adacore.com> wrote:
>
> > There are fixes in this patch together with the new functionality -
> > can you split
> > those out?  I'm thinking of the copy_edges_for_bb hunks as well as
> > the expand_call_inline ones.
>
> Like this?

OK.

Similar factoring of remap_location and copying/remapping edges goto_locus
is a fix worth splitting out (and backporting eventually if a need arises).  It
interferes somewhat with the DECL_INGORED parts but IMHO is separate from
those.

Thanks,
Richard.

>
>         * tree-inline.c (copy_edges_for_bb): Minor tweak.
>         (maybe_move_debug_stmts_to_successors): Also reset the locus of the
>         debug statement when resetting its value.
>         (expand_call_inline): Copy the locus of the call onto the assignment of
>         the return value, if any.  Use local variable in more cases.
>
> --
> Eric Botcazou
Eric Botcazou June 22, 2018, 9:12 a.m. UTC | #4 
> Similar factoring of remap_location and copying/remapping edges goto_locus
> is a fix worth splitting out (and backporting eventually if a need arises). 

Tentative patch attached, it passed basic testing.  I'll do a full testing 
cycle if this is the way to go.


	* tree-inline.c (remap_location): New function extracted from...
  	(copy_edges_for_bb): Add ID parameter.  Copy and remap goto_locus.
  	(copy_phis_for_bb): ...here.  Call remap_location.
	(copy_cfg_body): Adjust call to copy_edges_for_bb.
diff mbox series

Patch

Index: tree-inline.c
===================================================================
--- tree-inline.c	(revision 261687)
+++ tree-inline.c	(working copy)
@@ -747,6 +747,25 @@  remap_blocks_to_null (tree block, copy_b
     remap_blocks_to_null (t, id);
 }
 
+/* Remap the location info pointed to by LOCUS.  */
+
+static location_t
+remap_location (location_t locus, copy_body_data *id)
+{
+  if (LOCATION_BLOCK (locus))
+    {
+      tree *n = id->decl_map->get (LOCATION_BLOCK (locus));
+      gcc_assert (n);
+      if (*n)
+	return set_block (locus, *n);
+    }
+
+  if (id->block)
+    return set_block (locus, id->block);
+
+  return LOCATION_LOCUS (locus);
+}
+
 static void
 copy_statement_list (tree *tp)
 {
@@ -1618,6 +1637,8 @@  remap_gimple_stmt (gimple *stmt, copy_bo
 	    = gimple_build_debug_bind (gimple_debug_bind_get_var (stmt),
 				       gimple_debug_bind_get_value (stmt),
 				       stmt);
+	  if (DECL_IGNORED_P (id->src_fn) && !DECL_IGNORED_P (id->dst_fn))
+	    gimple_set_location (copy, input_location);
 	  id->debug_stmts.safe_push (copy);
 	  gimple_seq_add_stmt (&stmts, copy);
 	  return stmts;
@@ -1628,6 +1649,8 @@  remap_gimple_stmt (gimple *stmt, copy_bo
 	                   (gimple_debug_source_bind_get_var (stmt),
 			    gimple_debug_source_bind_get_value (stmt),
 			    stmt);
+	  if (DECL_IGNORED_P (id->src_fn) && !DECL_IGNORED_P (id->dst_fn))
+	    gimple_set_location (copy, input_location);
 	  id->debug_stmts.safe_push (copy);
 	  gimple_seq_add_stmt (&stmts, copy);
 	  return stmts;
@@ -1740,6 +1763,8 @@  remap_gimple_stmt (gimple *stmt, copy_bo
       gcc_assert (n);
       gimple_set_block (copy, *n);
     }
+  if (DECL_IGNORED_P (id->src_fn) && !DECL_IGNORED_P (id->dst_fn))
+    gimple_set_location (copy, input_location);
 
   if (gimple_debug_bind_p (copy) || gimple_debug_source_bind_p (copy)
       || gimple_debug_nonbind_marker_p (copy))
@@ -2144,14 +2169,14 @@  update_ssa_across_abnormal_edges (basic_
    debug stmts are left after a statement that must end the basic block.  */
 
 static bool
-copy_edges_for_bb (basic_block bb, profile_count num, profile_count den,
-		   basic_block ret_bb, basic_block abnormal_goto_dest)
+copy_edges_for_bb (copy_body_data *id, basic_block bb, profile_count num,
+		   profile_count den, basic_block ret_bb,
+		   basic_block abnormal_goto_dest)
 {
   basic_block new_bb = (basic_block) bb->aux;
   edge_iterator ei;
   edge old_edge;
   gimple_stmt_iterator si;
-  int flags;
   bool need_debug_cleanup = false;
 
   /* Use the indices from the original blocks to create edges for the
@@ -2160,16 +2185,20 @@  copy_edges_for_bb (basic_block bb, profi
     if (!(old_edge->flags & EDGE_EH))
       {
 	edge new_edge;
+	int flags = old_edge->flags;
+	location_t locus = old_edge->goto_locus;
 
-	flags = old_edge->flags;
-
-	/* Return edges do get a FALLTHRU flag when the get inlined.  */
+	/* Return edges do get a FALLTHRU flag when they get inlined.  */
 	if (old_edge->dest->index == EXIT_BLOCK
-	    && !(old_edge->flags & (EDGE_TRUE_VALUE|EDGE_FALSE_VALUE|EDGE_FAKE))
+	    && !(flags & (EDGE_TRUE_VALUE|EDGE_FALSE_VALUE|EDGE_FAKE))
 	    && old_edge->dest->aux != EXIT_BLOCK_PTR_FOR_FN (cfun))
 	  flags |= EDGE_FALLTHRU;
+
 	new_edge = make_edge (new_bb, (basic_block) old_edge->dest->aux, flags);
 	new_edge->probability = old_edge->probability;
+	if (LOCATION_LOCUS (locus) != UNKNOWN_LOCATION
+	    && !DECL_IGNORED_P (id->src_fn))
+	  new_edge->goto_locus = remap_location (locus, id);
       }
 
   if (bb->index == ENTRY_BLOCK || bb->index == EXIT_BLOCK)
@@ -2366,15 +2395,11 @@  copy_phis_for_bb (basic_block bb, copy_b
 		      inserted = true;
 		    }
 		  locus = gimple_phi_arg_location_from_edge (phi, old_edge);
-		  if (LOCATION_BLOCK (locus))
-		    {
-		      tree *n;
-		      n = id->decl_map->get (LOCATION_BLOCK (locus));
-		      gcc_assert (n);
-		      locus = set_block (locus, *n);
-		    }
+		  if (DECL_IGNORED_P (id->src_fn)
+		      && !DECL_IGNORED_P (id->dst_fn))
+		    locus = input_location;
 		  else
-		    locus = LOCATION_LOCUS (locus);
+		    locus = remap_location (locus, id);
 
 		  add_phi_arg (new_phi, new_arg, new_edge, locus);
 		}
@@ -2502,7 +2527,10 @@  maybe_move_debug_stmts_to_successors (co
 	      si = ssi;
 	      gsi_prev (&ssi);
 	      if (!single_pred_p (e->dest) && gimple_debug_bind_p (stmt))
-		gimple_debug_bind_reset_value (stmt);
+		{
+		  gimple_debug_bind_reset_value (stmt);
+		  gimple_set_location (stmt, UNKNOWN_LOCATION);
+		}
 	      gsi_remove (&si, false);
 	      gsi_insert_before (&dsi, stmt, GSI_SAME_STMT);
 	      continue;
@@ -2515,10 +2543,10 @@  maybe_move_debug_stmts_to_successors (co
 		{
 		  value = gimple_debug_bind_get_value (stmt);
 		  value = unshare_expr (value);
+		  new_stmt = gimple_build_debug_bind (var, value, stmt);
 		}
 	      else
-		value = NULL_TREE;
-	      new_stmt = gimple_build_debug_bind (var, value, stmt);
+		new_stmt = gimple_build_debug_bind (var, NULL_TREE, NULL);
 	    }
 	  else if (gimple_debug_source_bind_p (stmt))
 	    {
@@ -2702,8 +2730,9 @@  copy_cfg_body (copy_body_data * id,
   FOR_ALL_BB_FN (bb, cfun_to_copy)
     if (!id->blocks_to_copy
 	|| (bb->index > 0 && bitmap_bit_p (id->blocks_to_copy, bb->index)))
-      need_debug_cleanup |= copy_edges_for_bb (bb, num, den, exit_block_map,
-					       abnormal_goto_dest);
+      need_debug_cleanup
+	|= copy_edges_for_bb (id, bb, num, den, exit_block_map,
+			      abnormal_goto_dest);
 
   if (new_entry)
     {
@@ -4456,9 +4485,9 @@  expand_call_inline (basic_block bb, gimp
   id->assign_stmts.create (0);
 
   /* Update the callers EH personality.  */
-  if (DECL_FUNCTION_PERSONALITY (cg_edge->callee->decl))
+  if (DECL_FUNCTION_PERSONALITY (fn))
     DECL_FUNCTION_PERSONALITY (cg_edge->caller->decl)
-      = DECL_FUNCTION_PERSONALITY (cg_edge->callee->decl);
+      = DECL_FUNCTION_PERSONALITY (fn);
 
   /* Split the block before the GIMPLE_CALL.  */
   stmt_gsi = gsi_for_stmt (stmt);
@@ -4711,6 +4740,7 @@  expand_call_inline (basic_block bb, gimp
     {
       gimple *old_stmt = stmt;
       stmt = gimple_build_assign (gimple_call_lhs (stmt), use_retvar);
+      gimple_set_location (stmt, gimple_location (old_stmt));
       gsi_replace (&stmt_gsi, stmt, false);
       maybe_clean_or_replace_eh_stmt (old_stmt, stmt);
       /* Append a clobber for id->retvar if easily possible.  */
@@ -4806,7 +4836,7 @@  expand_call_inline (basic_block bb, gimp
      variables in the function when the blocks get blown away as soon as we
      remove the cgraph node.  */
   if (gimple_block (stmt))
-    (*debug_hooks->outlining_inline_function) (cg_edge->callee->decl);
+    (*debug_hooks->outlining_inline_function) (fn);
 
   /* Update callgraph if needed.  */
   cg_edge->callee->remove ();