[patch] Do not leak location information during inlining (2nd try)

[patch] Do not leak location information during inlining (2nd try)

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]

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 how 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 
forcing input_location on the inlined bodies in this case.

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


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

	* tree-inline.c (remap_gimple_stmt): Force input_location on the new
	statement if id->reset_location is true.
	(copy_edges_for_bb): Do not set goto_locus on the new edges if
	id->reset_location is true.
	(copy_phis_for_bb): Force input_location on the arguments if
	id->reset_location is true.
	(expand_call_inline): Set id->reset_location if DECL_IGNORED_P
	is set on the function to be inlined.
	* tree-inline.h (struct copy_body_data): Move remapping_type_depth and
	prevent_decl_creation_for_types fields up and add reset_location field.


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

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

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 4456 bytes --]

Index: tree-inline.c
===================================================================
--- tree-inline.c	(revision 262181)
+++ tree-inline.c	(working copy)
@@ -1630,6 +1630,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 (id->reset_location)
+	    gimple_set_location (copy, input_location);
 	  id->debug_stmts.safe_push (copy);
 	  gimple_seq_add_stmt (&stmts, copy);
 	  return stmts;
@@ -1640,6 +1642,8 @@ remap_gimple_stmt (gimple *stmt, copy_bo
 	                   (gimple_debug_source_bind_get_var (stmt),
 			    gimple_debug_source_bind_get_value (stmt),
 			    stmt);
+	  if (id->reset_location)
+	    gimple_set_location (copy, input_location);
 	  id->debug_stmts.safe_push (copy);
 	  gimple_seq_add_stmt (&stmts, copy);
 	  return stmts;
@@ -1653,6 +1657,8 @@ remap_gimple_stmt (gimple *stmt, copy_bo
 	    return stmts;
 
 	  gdebug *copy = as_a <gdebug *> (gimple_copy (stmt));
+	  if (id->reset_location)
+	    gimple_set_location (copy, input_location);
 	  id->debug_stmts.safe_push (copy);
 	  gimple_seq_add_stmt (&stmts, copy);
 	  return stmts;
@@ -1751,6 +1757,9 @@ remap_gimple_stmt (gimple *stmt, copy_bo
       gimple_set_block (copy, *n);
     }
 
+  if (id->reset_location)
+    gimple_set_location (copy, input_location);
+
   /* Debug statements ought to be rebuilt and not copied.  */
   gcc_checking_assert (!is_gimple_debug (copy));
 
@@ -2178,7 +2187,8 @@ copy_edges_for_bb (basic_block bb, profi
 	new_edge
 	  = make_edge (new_bb, (basic_block) old_edge->dest->aux, flags);
 	new_edge->probability = old_edge->probability;
-	new_edge->goto_locus = remap_location (locus, id);
+	if (!id->reset_location)
+	  new_edge->goto_locus = remap_location (locus, id);
       }
 
   if (bb->index == ENTRY_BLOCK || bb->index == EXIT_BLOCK)
@@ -2375,7 +2385,10 @@ copy_phis_for_bb (basic_block bb, copy_b
 		      inserted = true;
 		    }
 		  locus = gimple_phi_arg_location_from_edge (phi, old_edge);
-		  locus = remap_location (locus, id);
+		  if (id->reset_location)
+		    locus = input_location;
+		  else
+		    locus = remap_location (locus, id);
 		  add_phi_arg (new_phi, new_arg, new_edge, locus);
 		}
 	    }
@@ -4499,8 +4512,7 @@ expand_call_inline (basic_block bb, gimp
       prepend_lexical_block (gimple_block (stmt), id->block);
     }
 
-  /* Local declarations will be replaced by their equivalents in this
-     map.  */
+  /* Local declarations will be replaced by their equivalents in this map.  */
   st = id->decl_map;
   id->decl_map = new hash_map<tree, tree>;
   dst = id->debug_map;
@@ -4509,6 +4521,7 @@ expand_call_inline (basic_block bb, gimp
   /* Record the function we are about to inline.  */
   id->src_fn = fn;
   id->src_cfun = DECL_STRUCT_FUNCTION (fn);
+  id->reset_location = DECL_IGNORED_P (fn);
   id->call_stmt = call_stmt;
 
   /* When inlining into an OpenMP SIMD-on-SIMT loop, arrange for new automatic
Index: tree-inline.h
===================================================================
--- tree-inline.h	(revision 262181)
+++ tree-inline.h	(working copy)
@@ -80,6 +80,9 @@ struct copy_body_data
      is not.  */
   gcall *call_stmt;
 
+  /* > 0 if we are remapping a type currently.  */
+  int remapping_type_depth;
+
   /* Exception landing pad the inlined call lies in.  */
   int eh_lp_nr;
 
@@ -110,11 +113,14 @@ struct copy_body_data
   /* True if this statement will need to be regimplified.  */
   bool regimplify;
 
-  /* True if trees should not be unshared.  */
+  /* True if trees may not be unshared.  */
   bool do_not_unshare;
 
-  /* > 0 if we are remapping a type currently.  */
-  int remapping_type_depth;
+  /* True if new declarations may not be created during type remapping.  */
+  bool prevent_decl_creation_for_types;
+
+  /* True if the location information will need to be reset.  */
+  bool reset_location;
 
   /* A function to be called when duplicating BLOCK nodes.  */
   void (*transform_lang_insert_block) (tree);
@@ -145,9 +151,6 @@ struct copy_body_data
   /* A list of addressable local variables remapped into the caller
      when inlining a call within an OpenMP SIMD-on-SIMT loop.  */
   vec<tree> *dst_simt_vars;
-
-  /* Do not create new declarations when within type remapping.  */
-  bool prevent_decl_creation_for_types;
 };
 
 /* Weights of constructions for estimate_num_insns.  */

[-- Attachment #3: debug15.adb --]
[-- Type: text/x-adasrc, Size: 361 bytes --]

-- { dg-do compile }
-- { dg-options "-g1" }

procedure Debug15 is

   type Shape is abstract tagged record
      S : Integer;
   end record;

   type Rectangle is new Shape with record
      R : Integer;
   end record;

   X : Integer;

   R: Rectangle := (1, 2);
   S: Shape'Class := R;

begin
   X := 12;
end;

-- { dg-final { scan-assembler-not "loc 2" } }

Re: [patch] Do not leak location information during inlining (2nd try)

[Jeff Law]

On 06/27/2018 07:24 AM, Eric Botcazou 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 how 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 
> forcing input_location on the inlined bodies in this case.
> 
> Tested (GCC and GDB) on x86-64/Linux, OK for the mainline?
> 
> 
> 2018-06-27  Eric Botcazou  <ebotcazou@adacore.com>
> 
> 	* tree-inline.c (remap_gimple_stmt): Force input_location on the new
> 	statement if id->reset_location is true.
> 	(copy_edges_for_bb): Do not set goto_locus on the new edges if
> 	id->reset_location is true.
> 	(copy_phis_for_bb): Force input_location on the arguments if
> 	id->reset_location is true.
> 	(expand_call_inline): Set id->reset_location if DECL_IGNORED_P
> 	is set on the function to be inlined.
> 	* tree-inline.h (struct copy_body_data): Move remapping_type_depth and
> 	prevent_decl_creation_for_types fields up and add reset_location field.
> 
> 
> 2018-06-27  Eric Botcazou  <ebotcazou@adacore.com>
> 
>         * gnat.dg/debug15.adb: New test.
More references to input_location aren't ideal.  But I don't think
that's a strong enough reason to reject.  OK for the trunk.

jeff

Re: [patch] Do not leak location information during inlining (2nd try)

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 450 bytes --]

> More references to input_location aren't ideal.  But I don't think
> that's a strong enough reason to reject.

This can be avoided relatively easily though, patch to that effect attached.

Tested on x86-64/Linux, OK for the mainline?


	* tree-inline.c (remap_gimple_stmt): Replace input_location with
	gimple_location (id->call_stmt) throughout.
	(copy_phis_for_bb): Likewise.
	(expand_call_inline): Likewise.  Tweak formatting.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 3098 bytes --]

Index: tree-inline.c
===================================================================
--- tree-inline.c	(revision 262207)
+++ tree-inline.c	(working copy)
@@ -1631,7 +1631,7 @@ remap_gimple_stmt (gimple *stmt, copy_bo
 				       gimple_debug_bind_get_value (stmt),
 				       stmt);
 	  if (id->reset_location)
-	    gimple_set_location (copy, input_location);
+	    gimple_set_location (copy, gimple_location (id->call_stmt));
 	  id->debug_stmts.safe_push (copy);
 	  gimple_seq_add_stmt (&stmts, copy);
 	  return stmts;
@@ -1643,7 +1643,7 @@ remap_gimple_stmt (gimple *stmt, copy_bo
 			    gimple_debug_source_bind_get_value (stmt),
 			    stmt);
 	  if (id->reset_location)
-	    gimple_set_location (copy, input_location);
+	    gimple_set_location (copy, gimple_location (id->call_stmt));
 	  id->debug_stmts.safe_push (copy);
 	  gimple_seq_add_stmt (&stmts, copy);
 	  return stmts;
@@ -1658,7 +1658,7 @@ remap_gimple_stmt (gimple *stmt, copy_bo
 
 	  gdebug *copy = as_a <gdebug *> (gimple_copy (stmt));
 	  if (id->reset_location)
-	    gimple_set_location (copy, input_location);
+	    gimple_set_location (copy, gimple_location (id->call_stmt));
 	  id->debug_stmts.safe_push (copy);
 	  gimple_seq_add_stmt (&stmts, copy);
 	  return stmts;
@@ -1758,7 +1758,7 @@ remap_gimple_stmt (gimple *stmt, copy_bo
     }
 
   if (id->reset_location)
-    gimple_set_location (copy, input_location);
+    gimple_set_location (copy, gimple_location (id->call_stmt));
 
   /* Debug statements ought to be rebuilt and not copied.  */
   gcc_checking_assert (!is_gimple_debug (copy));
@@ -2386,7 +2386,7 @@ copy_phis_for_bb (basic_block bb, copy_b
 		    }
 		  locus = gimple_phi_arg_location_from_edge (phi, old_edge);
 		  if (id->reset_location)
-		    locus = input_location;
+		    locus = gimple_location (id->call_stmt);
 		  else
 		    locus = remap_location (locus, id);
 		  add_phi_arg (new_phi, new_arg, new_edge, locus);
@@ -4336,8 +4336,8 @@ expand_call_inline (basic_block bb, gimp
   gimple *simtenter_stmt = NULL;
   vec<tree> *simtvars_save;
 
-  /* The gimplifier uses input_location in too many places, such as
-     internal_get_tmp_var ().  */
+  /* FIXME: the gimplifier uses input_location in too many places,
+     such as internal_get_tmp_var.  Cope with it for now.  */
   location_t saved_location = input_location;
   input_location = gimple_location (stmt);
 
@@ -4559,12 +4559,18 @@ expand_call_inline (basic_block bb, gimp
 			GSI_NEW_STMT);
     }
   initialize_inlined_parameters (id, stmt, fn, bb);
-  if (debug_nonbind_markers_p && debug_inline_points && id->block
+
+  if (debug_nonbind_markers_p
+      && debug_inline_points
+      && id->block
       && inlined_function_outer_scope_p (id->block))
     {
       gimple_stmt_iterator si = gsi_last_bb (bb);
-      gsi_insert_after (&si, gimple_build_debug_inline_entry
-			(id->block, input_location), GSI_NEW_STMT);
+      gsi_insert_after (&si,
+			gimple_build_debug_inline_entry (id->block,
+							 gimple_location
+							 (id->call_stmt)),
+			GSI_NEW_STMT);
     }
 
   if (DECL_INITIAL (fn))

Re: [patch] Do not leak location information during inlining (2nd try)

[Richard Biener]

On June 28, 2018 12:28:15 PM GMT+02:00, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> More references to input_location aren't ideal.  But I don't think
>> that's a strong enough reason to reject.
>
>This can be avoided relatively easily though, patch to that effect
>attached.
>
>Tested on x86-64/Linux, OK for the mainline?

But then why not expose this as extra field in ID instead of repeatedly calling gimple_location? 

I don't have an issue with using input_location here until we fix the gimplifier... 

Richard. 

>
>	* tree-inline.c (remap_gimple_stmt): Replace input_location with
>	gimple_location (id->call_stmt) throughout.
>	(copy_phis_for_bb): Likewise.
>	(expand_call_inline): Likewise.  Tweak formatting.

Re: [patch] Do not leak location information during inlining (2nd try)

[Eric Botcazou]

> But then why not expose this as extra field in ID instead of repeatedly
> calling gimple_location?

That's a matter of taste I guess.

> I don't have an issue with using input_location here until we fix the
> gimplifier...

OK, patch dropped.

-- 
Eric Botcazou