Small improvements to coverage info (2/n)

Message ID 2413740.8n2UPbJL8F@polaris
State New
Headers show
Series
  • Small improvements to coverage info (2/n)
Related show

Commit Message

Eric Botcazou July 3, 2019, 1:35 p.m.
Hi,

this is a series of fixes for the exception handling code, with the same goal 
of preventing instructions from inheriting random source location information 
in the debug info generated by the compiler.

Tested on x86_64-suse-linux, both GCC and GDB, OK for mainline?


2019-07-03  Eric Botcazou  <ebotcazou@adacore.com>

	* except.c (emit_to_new_bb_before): Make sure to put a location on SEQ.
	* tree-eh.c (replace_goto_queue_1) <GIMPLE_GOTO>: Propagate location.
	(emit_eh_dispatch): Delete.
	(lower_catch): Emit the eh_dispatch manually and set the location of
	the first catch statement onto it.
	(lower_eh_filter): Emit the eh_dispatch manually and set location.
	(lower_eh_dispatch): Propagate location.
	* tree-outof-ssa.c (set_location_for_edge): Handle EH edges specially.
	(eliminate_build): Likewise.

-- 
Eric Botcazou

Comments

Jeff Law July 3, 2019, 10:04 p.m. | #1
On 7/3/19 7:35 AM, Eric Botcazou wrote:
> Hi,

> 

> this is a series of fixes for the exception handling code, with the same goal 

> of preventing instructions from inheriting random source location information 

> in the debug info generated by the compiler.

> 

> Tested on x86_64-suse-linux, both GCC and GDB, OK for mainline?

> 

> 

> 2019-07-03  Eric Botcazou  <ebotcazou@adacore.com>

> 

> 	* except.c (emit_to_new_bb_before): Make sure to put a location on SEQ.

> 	* tree-eh.c (replace_goto_queue_1) <GIMPLE_GOTO>: Propagate location.

> 	(emit_eh_dispatch): Delete.

> 	(lower_catch): Emit the eh_dispatch manually and set the location of

> 	the first catch statement onto it.

> 	(lower_eh_filter): Emit the eh_dispatch manually and set location.

> 	(lower_eh_dispatch): Propagate location.

> 	* tree-outof-ssa.c (set_location_for_edge): Handle EH edges specially.

> 	(eliminate_build): Likewise.

> 

OK
jeff

Patch

Index: except.c
===================================================================
--- except.c	(revision 272930)
+++ except.c	(working copy)
@@ -921,7 +921,7 @@  assign_filter_values (void)
 static basic_block
 emit_to_new_bb_before (rtx_insn *seq, rtx_insn *insn)
 {
-  rtx_insn *last;
+  rtx_insn *next, *last;
   basic_block bb;
   edge e;
   edge_iterator ei;
@@ -934,7 +934,16 @@  emit_to_new_bb_before (rtx_insn *seq, rt
       force_nonfallthru (e);
     else
       ei_next (&ei);
-  last = emit_insn_before (seq, insn);
+
+  /* Make sure to put the location of INSN or a subsequent instruction on SEQ
+     to avoid inheriting the location of the previous instruction.  */
+  next = insn;
+  while (next && !NONDEBUG_INSN_P (next))
+    next = NEXT_INSN (next);
+  if (next)
+    last = emit_insn_before_setloc (seq, insn, INSN_LOCATION (next));
+  else
+    last = emit_insn_before (seq, insn);
   if (BARRIER_P (last))
     last = PREV_INSN (last);
   bb = create_basic_block (seq, last, BLOCK_FOR_INSN (insn)->prev_bb);
Index: tree-eh.c
===================================================================
--- tree-eh.c	(revision 272930)
+++ tree-eh.c	(working copy)
@@ -503,7 +503,11 @@  replace_goto_queue_1 (gimple *stmt, stru
       seq = find_goto_replacement (tf, temp);
       if (seq)
 	{
-	  gsi_insert_seq_before (gsi, gimple_seq_copy (seq), GSI_SAME_STMT);
+	  gimple_stmt_iterator i;
+	  seq = gimple_seq_copy (seq);
+	  for (i = gsi_start (seq); !gsi_end_p (i); gsi_next (&i))
+	    gimple_set_location (gsi_stmt (i), gimple_location (stmt));
+	  gsi_insert_seq_before (gsi, seq, GSI_SAME_STMT);
 	  gsi_remove (gsi, false);
 	  return;
 	}
@@ -811,15 +815,6 @@  emit_resx (gimple_seq *seq, eh_region re
     record_stmt_eh_region (region->outer, x);
 }
 
-/* Emit an EH_DISPATCH statement into SEQ for REGION.  */
-
-static void
-emit_eh_dispatch (gimple_seq *seq, eh_region region)
-{
-  geh_dispatch *x = gimple_build_eh_dispatch (region->index);
-  gimple_seq_add_stmt (seq, x);
-}
-
 /* Note that the current EH region may contain a throw, or a
    call to a function which itself may contain a throw.  */
 
@@ -1762,7 +1757,9 @@  lower_catch (struct leh_state *state, gt
   tree out_label;
   gimple_seq new_seq, cleanup;
   gimple *x;
+  geh_dispatch *eh_dispatch;
   location_t try_catch_loc = gimple_location (tp);
+  location_t catch_loc = UNKNOWN_LOCATION;
 
   if (flag_exceptions)
     {
@@ -1776,7 +1773,8 @@  lower_catch (struct leh_state *state, gt
     return gimple_try_eval (tp);
 
   new_seq = NULL;
-  emit_eh_dispatch (&new_seq, try_region);
+  eh_dispatch = gimple_build_eh_dispatch (try_region->index);
+  gimple_seq_add_stmt (&new_seq, eh_dispatch);
   emit_resx (&new_seq, try_region);
 
   this_state.cur_region = state->cur_region;
@@ -1799,6 +1797,8 @@  lower_catch (struct leh_state *state, gt
       gimple_seq handler;
 
       catch_stmt = as_a <gcatch *> (gsi_stmt (gsi));
+      if (catch_loc == UNKNOWN_LOCATION)
+	catch_loc = gimple_location (catch_stmt);
       c = gen_eh_region_catch (try_region, gimple_catch_types (catch_stmt));
 
       handler = gimple_catch_handler (catch_stmt);
@@ -1822,6 +1822,10 @@  lower_catch (struct leh_state *state, gt
 	break;
     }
 
+  /* Try to set a location on the dispatching construct to avoid inheriting
+     the location of the previous statement.  */
+  gimple_set_location (eh_dispatch, catch_loc);
+
   gimple_try_set_cleanup (tp, new_seq);
 
   gimple_seq new_eh_seq = eh_seq;
@@ -1857,11 +1861,13 @@  lower_eh_filter (struct leh_state *state
   if (!eh_region_may_contain_throw (this_region))
     return gimple_try_eval (tp);
 
-  new_seq = NULL;
   this_state.cur_region = state->cur_region;
   this_state.ehp_region = this_region;
 
-  emit_eh_dispatch (&new_seq, this_region);
+  new_seq = NULL;
+  x = gimple_build_eh_dispatch (this_region->index);
+  gimple_set_location (x, gimple_location (tp));
+  gimple_seq_add_stmt (&new_seq, x);
   emit_resx (&new_seq, this_region);
 
   this_region->u.allowed.label = create_artificial_label (UNKNOWN_LOCATION);
@@ -3752,6 +3758,7 @@  lower_eh_dispatch (basic_block src, geh_
 	    filter = create_tmp_var (TREE_TYPE (TREE_TYPE (fn)));
 	    filter = make_ssa_name (filter, x);
 	    gimple_call_set_lhs (x, filter);
+	    gimple_set_location (x, gimple_location (stmt));
 	    gsi_insert_before (&gsi, x, GSI_SAME_STMT);
 
 	    /* Turn the default label into a default case.  */
@@ -3759,6 +3766,7 @@  lower_eh_dispatch (basic_block src, geh_
 	    sort_case_labels (labels);
 
 	    x = gimple_build_switch (filter, default_label, labels);
+	    gimple_set_location (x, gimple_location (stmt));
 	    gsi_insert_before (&gsi, x, GSI_SAME_STMT);
 	  }
       }
@@ -3775,6 +3783,7 @@  lower_eh_dispatch (basic_block src, geh_
 	filter = create_tmp_var (TREE_TYPE (TREE_TYPE (fn)));
 	filter = make_ssa_name (filter, x);
 	gimple_call_set_lhs (x, filter);
+	gimple_set_location (x, gimple_location (stmt));
 	gsi_insert_before (&gsi, x, GSI_SAME_STMT);
 
 	r->u.allowed.label = NULL;
Index: tree-outof-ssa.c
===================================================================
--- tree-outof-ssa.c	(revision 272930)
+++ tree-outof-ssa.c	(working copy)
@@ -171,14 +171,43 @@  struct elim_graph
    use its location.  Otherwise search instructions in predecessors
    of E for a location, and use that one.  That makes sense because
    we insert on edges for PHI nodes, and effects of PHIs happen on
-   the end of the predecessor conceptually.  */
+   the end of the predecessor conceptually.  An exception is made
+   for EH edges because we don't want to drag the source location
+   of unrelated statements at the beginning of handlers; they would
+   be further reused for various EH constructs, which would damage
+   the coverage information.  */
 
 static void
 set_location_for_edge (edge e)
 {
   if (e->goto_locus)
+    set_curr_insn_location (e->goto_locus);
+  else if (e->flags & EDGE_EH)
     {
-      set_curr_insn_location (e->goto_locus);
+      basic_block bb = e->dest;
+      gimple_stmt_iterator gsi;
+
+      do
+	{
+	  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+	    {
+	      gimple *stmt = gsi_stmt (gsi);
+	      if (is_gimple_debug (stmt))
+		continue;
+	      if (gimple_has_location (stmt) || gimple_block (stmt))
+		{
+		  set_curr_insn_location (gimple_location (stmt));
+		  return;
+		}
+	    }
+	  /* Nothing found in this basic block.  Make a half-assed attempt
+	     to continue with another block.  */
+	  if (single_succ_p (bb))
+	    bb = single_succ (bb);
+	  else
+	    bb = e->dest;
+	}
+      while (bb != e->dest);
     }
   else
     {
@@ -564,7 +593,11 @@  eliminate_build (elim_graph *g)
 	continue;
 
       Ti = PHI_ARG_DEF (phi, g->e->dest_idx);
-      locus = gimple_phi_arg_location_from_edge (phi, g->e);
+      /* See set_location_for_edge for the rationale.  */
+      if (g->e->flags & EDGE_EH)
+	locus = UNKNOWN_LOCATION;
+      else
+	locus = gimple_phi_arg_location_from_edge (phi, g->e);
 
       /* If this argument is a constant, or a SSA_NAME which is being
 	 left in SSA form, just queue a copy to be emitted on this