[PATCH] Fix lost locus issues at -O0 -g (PRs debug/29609, debug/36690, debug/37616)

[PATCH] Fix lost locus issues at -O0 -g (PRs debug/29609, debug/36690, debug/37616)

[Jakub Jelinek]

Hi!

Recent GCCs apparently frequently make debugging even -O0 -g code hard or
impossible in many cases.  This patch attempts to address bugs where
it is not possible to put a breakpoint on break, continue or goto, as even
at -O0 they have been optimized out, and also preserve scope for those
statements.  For the latter see e.g. the pr36690-2.c testcase where varz
is supposed to be visible only in the scope where it is defined, but
current GCCs don't keep track of that BLOCK anywhere and so the varz
variable is emitted in .debug_info as child of the DW_TAG_subprogram, not
part of DW_TAG_lexical_block which covers just the goto.
Most of the loses happen when even at -O0 we go into cfglayout mode,
which then try_optimize_cfg and optimizes out many edges with some
goto_locus that isn't used in either the last src or first dest bb insn.
The patch tries to maintain goto_locus throughout the passes and at
outof_cfglayout time splits edges where goto_locus is different from
last src and first dest bb insn.  Also when merging blocks in into_cfglayout
when not optimizing, a nop with the desired INSN_LOCATOR is inserted between
the two former bb insns.

So far regtested just with make check RUNTESTFLAGS={dwarf2,debug}.exp
and in gdb on all the new testcases.  I'll bootstrap/regtest it later on
on x86_64-linux and try to bootstrap it with -O0 too.

Ok for trunk if it passes?

2008-10-01  Jakub Jelinek  <jakub@redhat.com>

	PR debug/29609
	PR debug/36690
	PR debug/37616
	* basic-block.h (struct edge_def): Add goto_block field.
	* cfglayout.c (fixup_reorder_chain): Ensure that there is at least
	one insn with locus corresponding to edge's goto_locus if !optimize.
	* profile.c (branch_prob): Copy edge's goto_block.
	* cfgrtl.c (force_nonfallthru_and_redirect): Use goto_locus for
	emitted jumps.
	(cfg_layout_merge_blocks): Emit a nop with edge's goto_locus
	locator in between the merged basic blocks if !optimize and needed.
	* cfgexpand.c (expand_gimple_cond): Convert goto_block and
	goto_locus into RTL locator.  For unconditional jump use that
	locator for the jump insn.
	(expand_gimple_cond): Convert goto_block and goto_locus into
	RTL locator for all remaining edges.  For unconditional jump
	use that locator for the jump insn.
	* cfgcleanup.c (try_forward_edges): Avoid the optimization if
	there is more than one edge or insn locator along the forwarding
	edges and !optimize.  If there is just one, set e->goto_locus.
	* tree-cfg.c (make_cond_expr_edges, make_goto_expr_edges): Set also
	edge's goto_block.
	(move_block_to_fn): Adjust edge's goto_block.

	* gcc.dg/debug/pr29609-1.c: New test.
	* gcc.dg/debug/pr29609-2.c: New test.
	* gcc.dg/debug/pr36690-1.c: New test.
	* gcc.dg/debug/pr36690-2.c: New test.
	* gcc.dg/debug/pr36690-3.c: New test.
	* gcc.dg/debug/pr37616.c: New test.
	* gcc.dg/debug/dwarf2/pr29609-1.c: New test.
	* gcc.dg/debug/dwarf2/pr29609-2.c: New test.
	* gcc.dg/debug/dwarf2/pr36690-1.c: New test.
	* gcc.dg/debug/dwarf2/pr36690-2.c: New test.
	* gcc.dg/debug/dwarf2/pr36690-3.c: New test.
	* gcc.dg/debug/dwarf2/pr37616.c: New test.

--- gcc/basic-block.h.jj	2008-09-05 12:56:32.000000000 +0200
+++ gcc/basic-block.h	2008-09-30 17:45:20.000000000 +0200
@@ -129,7 +129,8 @@ struct edge_def GTY(())
   /* Auxiliary info specific to a pass.  */
   PTR GTY ((skip (""))) aux;
 
-  /* Location of any goto implicit in the edge, during tree-ssa.  */
+  /* Location of any goto implicit in the edge and associated BLOCK.  */
+  tree goto_block;
   location_t goto_locus;
 
   /* The index number corresponding to this edge in the edge vector
--- gcc/cfglayout.c.jj	2008-09-11 16:04:26.000000000 +0200
+++ gcc/cfglayout.c	2008-10-01 15:20:49.000000000 +0200
@@ -887,6 +887,46 @@ fixup_reorder_chain (void)
       if (e && !can_fallthru (e->src, e->dest))
 	force_nonfallthru (e);
     }
+
+  /* Ensure goto_locus from edges has some instructions with that locus
+     in RTL.  */
+  if (!optimize)
+    FOR_EACH_BB (bb)
+      {
+        edge e;
+        edge_iterator ei;
+
+        FOR_EACH_EDGE (e, ei, bb->succs)
+	  if (e->goto_locus && !(e->flags & EDGE_ABNORMAL))
+	    {
+	      basic_block nb;
+
+	      if (simplejump_p (BB_END (e->src)))
+		{
+		  if (INSN_LOCATOR (BB_END (e->src)) == (int) e->goto_locus)
+		    continue;
+		  if (INSN_LOCATOR (BB_END (e->src)) == 0)
+		    {
+		      INSN_LOCATOR (BB_END (e->src)) = e->goto_locus;
+		      continue;
+		    }
+		}
+	      if (e->dest != EXIT_BLOCK_PTR)
+		{
+		  insn = BB_HEAD (e->dest);
+		  if (!INSN_P (insn))
+		    insn = next_insn (insn);
+		  if (insn && INSN_P (insn)
+		      && INSN_LOCATOR (insn) == (int) e->goto_locus)
+		    continue;
+		}
+	      nb = split_edge (e);
+	      if (!INSN_P (BB_END (nb)))
+		BB_END (nb) = emit_insn_after_noloc (gen_nop (), BB_END (nb),
+						     nb);
+	      INSN_LOCATOR (BB_END (nb)) = e->goto_locus;
+	    }
+      }
 }
 \f
 /* Perform sanity checks on the insn chain.
--- gcc/profile.c.jj	2008-09-09 21:11:04.000000000 +0200
+++ gcc/profile.c	2008-09-30 17:45:20.000000000 +0200
@@ -960,10 +960,12 @@ branch_prob (void)
 	      && (LOCATION_FILE (e->goto_locus)
 	          != LOCATION_FILE (gimple_location (last))
 		  || (LOCATION_LINE (e->goto_locus)
-		      != LOCATION_LINE (gimple_location  (last)))))
+		      != LOCATION_LINE (gimple_location (last)))))
 	    {
 	      basic_block new_bb = split_edge (e);
-	      single_succ_edge (new_bb)->goto_locus = e->goto_locus;
+	      edge ne = single_succ_edge (new_bb);
+	      ne->goto_locus = e->goto_locus;
+	      ne->goto_block = e->goto_block;
 	    }
 	  if ((e->flags & (EDGE_ABNORMAL | EDGE_ABNORMAL_CALL))
 	       && e->dest != EXIT_BLOCK_PTR)
--- gcc/cfgrtl.c.jj	2008-09-05 12:56:32.000000000 +0200
+++ gcc/cfgrtl.c	2008-10-01 15:27:05.000000000 +0200
@@ -1009,6 +1009,7 @@ force_nonfallthru_and_redirect (edge e, 
   rtx note;
   edge new_edge;
   int abnormal_edge_flags = 0;
+  int loc;
 
   /* In the case the last instruction is conditional jump to the next
      instruction, first redirect the jump itself and then continue
@@ -1127,11 +1128,15 @@ force_nonfallthru_and_redirect (edge e, 
   else
     jump_block = e->src;
 
+  if (e->goto_locus && e->goto_block == NULL)
+    loc = e->goto_locus;
+  else
+    loc = 0;
   e->flags &= ~EDGE_FALLTHRU;
   if (target == EXIT_BLOCK_PTR)
     {
 #ifdef HAVE_return
-	emit_jump_insn_after_noloc (gen_return (), BB_END (jump_block));
+	emit_jump_insn_after_setloc (gen_return (), BB_END (jump_block), loc);
 #else
 	gcc_unreachable ();
 #endif
@@ -1139,7 +1144,7 @@ force_nonfallthru_and_redirect (edge e, 
   else
     {
       rtx label = block_label (target);
-      emit_jump_insn_after_noloc (gen_jump (label), BB_END (jump_block));
+      emit_jump_insn_after_setloc (gen_jump (label), BB_END (jump_block), loc);
       JUMP_LABEL (BB_END (jump_block)) = label;
       LABEL_NUSES (label)++;
     }
@@ -2606,6 +2611,32 @@ cfg_layout_merge_blocks (basic_block a, 
     try_redirect_by_replacing_jump (EDGE_SUCC (a, 0), b, true);
   gcc_assert (!JUMP_P (BB_END (a)));
 
+  /* When not optimizing and the edge is the only place in RTL which holds
+     some unique locus, emit a nop with that locus in between.  */
+  if (!optimize && EDGE_SUCC (a, 0)->goto_locus)
+    {
+      rtx insn = BB_END (a);
+      int goto_locus = EDGE_SUCC (a, 0)->goto_locus;
+
+      if (NOTE_P (insn))
+	insn = prev_nonnote_insn (insn);
+      if (insn && INSN_P (insn) && INSN_LOCATOR (insn) == goto_locus)
+	goto_locus = 0;
+      else
+	{
+	  insn = BB_HEAD (b);
+	  if (!INSN_P (insn))
+	    insn = next_insn (insn);
+	  if (insn && INSN_P (insn) && INSN_LOCATOR (insn) == goto_locus)
+	    goto_locus = 0;
+	}
+      if (goto_locus)
+	{
+	  BB_END (a) = emit_insn_after_noloc (gen_nop (), BB_END (a), a);
+	  INSN_LOCATOR (BB_END (a)) = goto_locus;
+	}
+    }
+
   /* Possible line number notes should appear in between.  */
   if (b->il.rtl->header)
     {
--- gcc/cfgexpand.c.jj	2008-09-18 17:12:45.000000000 +0200
+++ gcc/cfgexpand.c	2008-10-01 11:18:04.000000000 +0200
@@ -1666,7 +1666,12 @@ expand_gimple_cond (basic_block bb, gimp
       add_reg_br_prob_note (last, true_edge->probability);
       maybe_dump_rtl_for_gimple_stmt (stmt, last);
       if (true_edge->goto_locus)
-  	set_curr_insn_source_location (true_edge->goto_locus);
+	{
+	  set_curr_insn_source_location (true_edge->goto_locus);
+	  set_curr_insn_block (true_edge->goto_block);
+	  true_edge->goto_locus = curr_insn_locator ();
+	}
+      true_edge->goto_block = NULL;
       false_edge->flags |= EDGE_FALLTHRU;
       ggc_free (pred);
       return NULL;
@@ -1677,7 +1682,12 @@ expand_gimple_cond (basic_block bb, gimp
       add_reg_br_prob_note (last, false_edge->probability);
       maybe_dump_rtl_for_gimple_stmt (stmt, last);
       if (false_edge->goto_locus)
-  	set_curr_insn_source_location (false_edge->goto_locus);
+	{
+	  set_curr_insn_source_location (false_edge->goto_locus);
+	  set_curr_insn_block (false_edge->goto_block);
+	  false_edge->goto_locus = curr_insn_locator ();
+	}
+      false_edge->goto_block = NULL;
       true_edge->flags |= EDGE_FALLTHRU;
       ggc_free (pred);
       return NULL;
@@ -1686,6 +1696,13 @@ expand_gimple_cond (basic_block bb, gimp
   jumpif (pred, label_rtx_for_bb (true_edge->dest));
   add_reg_br_prob_note (last, true_edge->probability);
   last = get_last_insn ();
+  if (false_edge->goto_locus)
+    {
+      set_curr_insn_source_location (false_edge->goto_locus);
+      set_curr_insn_block (false_edge->goto_block);
+      false_edge->goto_locus = curr_insn_locator ();
+    }
+  false_edge->goto_block = NULL;
   emit_jump (label_rtx_for_bb (false_edge->dest));
 
   BB_END (bb) = last;
@@ -1708,9 +1725,6 @@ expand_gimple_cond (basic_block bb, gimp
 
   maybe_dump_rtl_for_gimple_stmt (stmt, last2);
 
-  if (false_edge->goto_locus)
-    set_curr_insn_source_location (false_edge->goto_locus);
-
   ggc_free (pred);
   return new_bb;
 }
@@ -1962,19 +1976,21 @@ expand_gimple_basic_block (basic_block b
 	}
     }
 
-  /* Expand implicit goto.  */
+  /* Expand implicit goto and convert goto_locus.  */
   FOR_EACH_EDGE (e, ei, bb->succs)
     {
-      if (e->flags & EDGE_FALLTHRU)
-	break;
-    }
-
-  if (e && e->dest != bb->next_bb)
-    {
-      emit_jump (label_rtx_for_bb (e->dest));
-      if (e->goto_locus)
-        set_curr_insn_source_location (e->goto_locus);
-      e->flags &= ~EDGE_FALLTHRU;
+      if (e->goto_locus && e->goto_block)
+	{
+	  set_curr_insn_source_location (e->goto_locus);
+	  set_curr_insn_block (e->goto_block);
+	  e->goto_locus = curr_insn_locator ();
+	}
+      e->goto_block = NULL;
+      if ((e->flags & EDGE_FALLTHRU) && e->dest != bb->next_bb)
+	{
+	  emit_jump (label_rtx_for_bb (e->dest));
+	  e->flags &= ~EDGE_FALLTHRU;
+	}
     }
 
   do_pending_stack_adjust ();
--- gcc/cfgcleanup.c.jj	2008-09-05 12:56:32.000000000 +0200
+++ gcc/cfgcleanup.c	2008-10-01 12:02:44.000000000 +0200
@@ -429,7 +429,7 @@ try_forward_edges (int mode, basic_block
   for (ei = ei_start (b->succs); (e = ei_safe_edge (ei)); )
     {
       basic_block target, first;
-      int counter;
+      int counter, goto_locus;
       bool threaded = false;
       int nthreaded_edges = 0;
       bool may_thread = first_pass | df_get_bb_dirty (b);
@@ -447,6 +447,7 @@ try_forward_edges (int mode, basic_block
 
       target = first = e->dest;
       counter = NUM_FIXED_BLOCKS;
+      goto_locus = e->goto_locus;
 
       /* If we are partitioning hot/cold basic_blocks, we don't want to mess
 	 up jumps that cross between hot/cold sections.
@@ -476,6 +477,27 @@ try_forward_edges (int mode, basic_block
 	      new_target = single_succ (target);
 	      if (target == new_target)
 		counter = n_basic_blocks;
+	      else if (!optimize)
+		{
+		  /* When not optimizing, ensure that edges or forwarder
+		     blocks with different locus are not optimized out.  */
+		  int locus = single_succ_edge (target)->goto_locus;
+
+		  if (locus && goto_locus && locus != goto_locus)
+		    counter = n_basic_blocks;
+		  else if (locus)
+		    goto_locus = locus;
+
+		  if (INSN_P (BB_END (target)))
+		    {
+		      locus = INSN_LOCATOR (BB_END (target));
+
+		      if (locus && goto_locus && locus != goto_locus)
+			counter = n_basic_blocks;
+		      else if (locus)
+			goto_locus = locus;
+		    }
+		}
 	    }
 
 	  /* Allow to thread only over one edge at time to simplify updating
@@ -539,6 +561,8 @@ try_forward_edges (int mode, basic_block
 	  int edge_frequency;
 	  int n = 0;
 
+	  e->goto_locus = goto_locus;
+
 	  /* Don't force if target is exit block.  */
 	  if (threaded && target != EXIT_BLOCK_PTR)
 	    {
--- gcc/tree-cfg.c.jj	2008-09-18 17:08:43.000000000 +0200
+++ gcc/tree-cfg.c	2008-09-30 17:45:20.000000000 +0200
@@ -658,9 +658,13 @@ make_cond_expr_edges (basic_block bb)
 
   e = make_edge (bb, then_bb, EDGE_TRUE_VALUE);
   e->goto_locus = gimple_location (then_stmt);
+  e->goto_block = gimple_block (then_stmt);
   e = make_edge (bb, else_bb, EDGE_FALSE_VALUE);
   if (e)
-    e->goto_locus = gimple_location (else_stmt);
+    {
+      e->goto_locus = gimple_location (else_stmt);
+      e->goto_block = gimple_block (else_stmt);
+    }
 
   /* We do not need the labels anymore.  */
   gimple_cond_set_true_label (entry, NULL_TREE);
@@ -849,6 +853,7 @@ make_goto_expr_edges (basic_block bb)
       tree dest = gimple_goto_dest (goto_t);
       edge e = make_edge (bb, label_to_block (dest), EDGE_FALLTHRU);
       e->goto_locus = gimple_location (goto_t);
+      e->goto_block = gimple_block (goto_t);
       gsi_remove (&last, true);
       return;
     }
@@ -5743,6 +5748,23 @@ move_block_to_fn (struct function *dest_
       update_stmt (stmt);
       pop_cfun ();
     }
+
+  FOR_EACH_EDGE (e, ei, bb->succs)
+    if (e->goto_locus)
+      {
+	tree block = e->goto_block;
+	if (d->orig_block == NULL_TREE
+	    || block == d->orig_block)
+	  e->goto_block = d->new_block;
+#ifdef ENABLE_CHECKING
+	else if (block != d->new_block)
+	  {
+	    while (block && block != d->orig_block)
+	      block = BLOCK_SUPERCONTEXT (block);
+	    gcc_assert (block);
+	  }
+#endif
+      }
 }
 
 /* Examine the statements in BB (which is in SRC_CFUN); find and return
--- gcc/testsuite/gcc.dg/debug/pr37616.c.jj	2008-10-01 14:35:14.000000000 +0200
+++ gcc/testsuite/gcc.dg/debug/pr37616.c	2008-10-01 15:50:15.000000000 +0200
@@ -0,0 +1,37 @@
+/* PR debug/37616 */
+/* Test that one can put breakpoints onto continue, exitlab and break
+   and actually see program reaching those breakpoints.  */
+/* { dg-do run } */
+/* { dg-options "-O0 -g -dA" } */
+
+extern void abort (void);
+
+int
+foo (int parm)
+{
+  int varj, varm;
+
+  for (varj = 0; varj < 10; varj++)
+    {
+      if (varj == 5)
+	continue;
+      if (varj == 7 && !parm)
+	goto exitlab;
+      if (varj == 9)
+	break;
+      varm = varj;
+    }
+
+exitlab:
+  return varm;
+}
+
+int
+main (void)
+{
+  if (foo (0) != 6)
+    abort ();
+  if (foo (1) != 8)
+    abort ();
+  return 0;
+}
--- gcc/testsuite/gcc.dg/debug/pr36690-3.c.jj	2008-10-01 14:49:28.000000000 +0200
+++ gcc/testsuite/gcc.dg/debug/pr36690-3.c	2008-10-01 15:50:09.000000000 +0200
@@ -0,0 +1,47 @@
+/* PR debug/36690 */
+/* { dg-do run } */
+/* { dg-options "-O0 -g -dA" } */
+
+int cnt;
+
+void
+bar (int i)
+{
+  cnt += i;
+}
+
+void
+foo (int i, int j)
+{
+  if (j)
+    {
+      bar (i + 1);
+      goto f1;
+    }
+  bar (i + 2);
+  goto f2;
+f1:
+  if (i > 10)
+    goto f3;
+f2:
+  if (i > 40)
+    goto f4;
+  else
+    goto f5;
+f3:
+  bar (i);
+f4:
+  bar (i);
+f5:
+  bar (i);
+}
+
+int
+main (void)
+{
+  foo (0, 1);
+  foo (11, 1);
+  foo (21, 0);
+  foo (41, 0);
+  return 0;
+}
--- gcc/testsuite/gcc.dg/debug/pr36690-2.c.jj	2008-10-01 14:49:26.000000000 +0200
+++ gcc/testsuite/gcc.dg/debug/pr36690-2.c	2008-10-01 16:06:12.000000000 +0200
@@ -0,0 +1,37 @@
+/* PR debug/36690 */
+/* Verify that breakpoint can be put on goto f1, it is hit and
+   varz at that spot is defined and contains 5.  Nowhere else
+   in the function should be varz in the scope.  */
+/* { dg-do run } */
+/* { dg-options "-O0 -g -dA" } */
+
+int cnt;
+
+void
+bar (int i)
+{
+  cnt += i;
+}
+
+void
+foo (int i)
+{
+  if (!i)
+    bar (0);
+  else
+    {
+      static int varz = 5;
+      goto f1;
+    }
+  bar (1);
+f1:
+  bar (2);
+}
+
+int
+main (void)
+{
+  foo (0);
+  foo (1);
+  return 0;
+}
--- gcc/testsuite/gcc.dg/debug/pr36690-1.c.jj	2008-10-01 14:39:33.000000000 +0200
+++ gcc/testsuite/gcc.dg/debug/pr36690-1.c	2008-10-01 15:49:57.000000000 +0200
@@ -0,0 +1,20 @@
+/* PR debug/36690 */
+/* Verify that break func is hit.  */
+/* { dg-do run } */
+/* { dg-options "-O0 -g -dA" } */
+
+int i;
+
+void
+func (void)
+{
+  while (i == 1)
+    i = 0;
+}
+
+int
+main (void)
+{
+  func ();
+  return 0;
+}
--- gcc/testsuite/gcc.dg/debug/pr29609-2.c.jj	2008-10-01 15:01:07.000000000 +0200
+++ gcc/testsuite/gcc.dg/debug/pr29609-2.c	2008-10-01 15:49:51.000000000 +0200
@@ -0,0 +1,50 @@
+/* PR debug/29609 */
+/* Verify that breakpoint on both goto failure; stmts is hit.  */
+/* { dg-do run } */
+/* { dg-options "-O0 -g -dA" } */
+
+extern void abort (void);
+int x;
+
+int
+foo (void)
+{
+  return 0 ^ x;
+}
+
+int
+bar (void)
+{
+  return 1 ^ x;
+}
+
+int
+baz (void)
+{
+  int c;
+
+  if (!foo ())
+    goto failure;
+
+  if (!bar ())
+    goto failure;
+
+  return 0;
+
+failure:
+  return 1;
+}
+
+int
+main (void)
+{
+  if (baz () != 1)
+    abort ();
+  x = 1;
+  if (baz () != 1)
+    abort ();
+  x = 2;
+  if (baz () != 0)
+    abort ();
+  return 0;
+}
--- gcc/testsuite/gcc.dg/debug/pr29609-1.c.jj	2008-10-01 14:56:41.000000000 +0200
+++ gcc/testsuite/gcc.dg/debug/pr29609-1.c	2008-10-01 15:57:37.000000000 +0200
@@ -0,0 +1,30 @@
+/* PR debug/29609 */
+/* Verify that breakpoint on the break is hit.  */
+/* { dg-do run } */
+/* { dg-options "-O0 -g -dA" } */
+
+extern void abort (void);
+
+int
+foo (void)
+{
+  int a, i;
+
+  for (i = 1; i <= 10; i++)
+    {
+      if (i < 3)
+	a = 1;
+      else
+	break;
+      a = 5;
+    }
+  return a;
+}
+
+int
+main (void)
+{
+  if (foo () != 5)
+    abort ();
+  return 0;
+}
--- gcc/testsuite/gcc.dg/debug/dwarf2/pr37616.c.jj	2008-10-01 14:35:14.000000000 +0200
+++ gcc/testsuite/gcc.dg/debug/dwarf2/pr37616.c	2008-10-01 15:53:00.000000000 +0200
@@ -0,0 +1,41 @@
+/* PR debug/37616 */
+/* Test that one can put breakpoints onto continue, exitlab and break
+   and actually see program reaching those breakpoints.  */
+/* { dg-do compile } */
+/* { dg-options "-O0 -gdwarf-2 -dA" } */
+
+extern void abort (void);
+
+int
+foo (int parm)
+{
+  int varj, varm;
+
+  for (varj = 0; varj < 10; varj++)
+    {
+      if (varj == 5)
+	continue;
+      if (varj == 7 && !parm)
+	goto exitlab;
+      if (varj == 9)
+	break;
+      varm = varj;
+    }
+
+exitlab:
+  return varm;
+}
+
+int
+main (void)
+{
+  if (foo (0) != 6)
+    abort ();
+  if (foo (1) != 8)
+    abort ();
+  return 0;
+}
+
+/* { dg-final { scan-assembler "pr37616.c:17" } } */
+/* { dg-final { scan-assembler "pr37616.c:19" } } */
+/* { dg-final { scan-assembler "pr37616.c:21" } } */
--- gcc/testsuite/gcc.dg/debug/dwarf2/pr36690-3.c.jj	2008-10-01 14:49:28.000000000 +0200
+++ gcc/testsuite/gcc.dg/debug/dwarf2/pr36690-3.c	2008-10-01 15:52:35.000000000 +0200
@@ -0,0 +1,53 @@
+/* PR debug/36690 */
+/* { dg-do compile } */
+/* { dg-options "-O0 -gdwarf-2 -dA" } */
+
+int cnt;
+
+void
+bar (int i)
+{
+  cnt += i;
+}
+
+void
+foo (int i, int j)
+{
+  if (j)
+    {
+      bar (i + 1);
+      goto f1;
+    }
+  bar (i + 2);
+  goto f2;
+f1:
+  if (i > 10)
+    goto f3;
+f2:
+  if (i > 40)
+    goto f4;
+  else
+    goto f5;
+f3:
+  bar (i);
+f4:
+  bar (i);
+f5:
+  bar (i);
+}
+
+int
+main (void)
+{
+  foo (0, 1);
+  foo (11, 1);
+  foo (21, 0);
+  foo (41, 0);
+  return 0;
+}
+
+/* { dg-final { scan-assembler "pr36690-3.c:19" } } */
+/* { dg-final { scan-assembler "pr36690-3.c:22" } } */
+/* { dg-final { scan-assembler "pr36690-3.c:25" } } */
+/* { dg-final { scan-assembler "pr36690-3.c:28" } } */
+/* { dg-final { scan-assembler "pr36690-3.c:30" } } */
--- gcc/testsuite/gcc.dg/debug/dwarf2/pr36690-2.c.jj	2008-10-01 14:49:26.000000000 +0200
+++ gcc/testsuite/gcc.dg/debug/dwarf2/pr36690-2.c	2008-10-01 16:06:32.000000000 +0200
@@ -0,0 +1,39 @@
+/* PR debug/36690 */
+/* Verify that breakpoint can be put on goto f1, it is hit and
+   varz at that spot is defined and contains 5.  Nowhere else
+   in the function should be varz in the scope.  */
+/* { dg-do compile } */
+/* { dg-options "-O0 -gdwarf-2 -dA" } */
+
+int cnt;
+
+void
+bar (int i)
+{
+  cnt += i;
+}
+
+void
+foo (int i)
+{
+  if (!i)
+    bar (0);
+  else
+    {
+      static int varz = 5;
+      goto f1;
+    }
+  bar (1);
+f1:
+  bar (2);
+}
+
+int
+main (void)
+{
+  foo (0);
+  foo (1);
+  return 0;
+}
+
+/* { dg-final { scan-assembler "pr36690-2.c:24" } } */
--- gcc/testsuite/gcc.dg/debug/dwarf2/pr36690-1.c.jj	2008-10-01 14:39:33.000000000 +0200
+++ gcc/testsuite/gcc.dg/debug/dwarf2/pr36690-1.c	2008-10-01 15:51:39.000000000 +0200
@@ -0,0 +1,22 @@
+/* PR debug/36690 */
+/* Verify that break func is hit.  */
+/* { dg-do compile } */
+/* { dg-options "-O0 -gdwarf-2 -dA" } */
+
+int i;
+
+void
+func (void)
+{
+  while (i == 1)
+    i = 0;
+}
+
+int
+main (void)
+{
+  func ();
+  return 0;
+}
+
+/* { dg-final { scan-assembler "pr36690-1.c:11" } } */
--- gcc/testsuite/gcc.dg/debug/dwarf2/pr29609-2.c.jj	2008-10-01 15:01:07.000000000 +0200
+++ gcc/testsuite/gcc.dg/debug/dwarf2/pr29609-2.c	2008-10-01 15:51:23.000000000 +0200
@@ -0,0 +1,53 @@
+/* PR debug/29609 */
+/* Verify that breakpoint on both goto failure; stmts is hit.  */
+/* { dg-do compile } */
+/* { dg-options "-O0 -gdwarf-2 -dA" } */
+
+extern void abort (void);
+int x;
+
+int
+foo (void)
+{
+  return 0 ^ x;
+}
+
+int
+bar (void)
+{
+  return 1 ^ x;
+}
+
+int
+baz (void)
+{
+  int c;
+
+  if (!foo ())
+    goto failure;
+
+  if (!bar ())
+    goto failure;
+
+  return 0;
+
+failure:
+  return 1;
+}
+
+int
+main (void)
+{
+  if (baz () != 1)
+    abort ();
+  x = 1;
+  if (baz () != 1)
+    abort ();
+  x = 2;
+  if (baz () != 0)
+    abort ();
+  return 0;
+}
+
+/* { dg-final { scan-assembler "pr29609-2.c:27" } } */
+/* { dg-final { scan-assembler "pr29609-2.c:30" } } */
--- gcc/testsuite/gcc.dg/debug/dwarf2/pr29609-1.c.jj	2008-10-01 14:56:41.000000000 +0200
+++ gcc/testsuite/gcc.dg/debug/dwarf2/pr29609-1.c	2008-10-01 15:57:51.000000000 +0200
@@ -0,0 +1,32 @@
+/* PR debug/29609 */
+/* Verify that breakpoint on the break is hit.  */
+/* { dg-do compile } */
+/* { dg-options "-O0 -gdwarf-2 -dA" } */
+
+void abort (void);
+
+int
+foo (void)
+{
+  int a, i;
+
+  for (i = 1; i <= 10; i++)
+    {
+      if (i < 3)
+	a = 1;
+      else
+	break;
+      a = 5;
+    }
+  return a;
+}
+
+int
+main (void)
+{
+  if (foo () != 5)
+    abort ();
+  return 0;
+}
+
+/* { dg-final { scan-assembler "pr29609-1.c:18" } } */

	Jakub

Re: [PATCH] Fix lost locus issues at -O0 -g (PRs debug/29609, debug/36690, debug/37616)

[Ian Lance Taylor]

Jakub Jelinek <jakub@redhat.com> writes:

> 2008-10-01  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR debug/29609
> 	PR debug/36690
> 	PR debug/37616
> 	* basic-block.h (struct edge_def): Add goto_block field.
> 	* cfglayout.c (fixup_reorder_chain): Ensure that there is at least
> 	one insn with locus corresponding to edge's goto_locus if !optimize.
> 	* profile.c (branch_prob): Copy edge's goto_block.
> 	* cfgrtl.c (force_nonfallthru_and_redirect): Use goto_locus for
> 	emitted jumps.
> 	(cfg_layout_merge_blocks): Emit a nop with edge's goto_locus
> 	locator in between the merged basic blocks if !optimize and needed.
> 	* cfgexpand.c (expand_gimple_cond): Convert goto_block and
> 	goto_locus into RTL locator.  For unconditional jump use that
> 	locator for the jump insn.
> 	(expand_gimple_cond): Convert goto_block and goto_locus into
> 	RTL locator for all remaining edges.  For unconditional jump
> 	use that locator for the jump insn.
> 	* cfgcleanup.c (try_forward_edges): Avoid the optimization if
> 	there is more than one edge or insn locator along the forwarding
> 	edges and !optimize.  If there is just one, set e->goto_locus.
> 	* tree-cfg.c (make_cond_expr_edges, make_goto_expr_edges): Set also
> 	edge's goto_block.
> 	(move_block_to_fn): Adjust edge's goto_block.
>
> 	* gcc.dg/debug/pr29609-1.c: New test.
> 	* gcc.dg/debug/pr29609-2.c: New test.
> 	* gcc.dg/debug/pr36690-1.c: New test.
> 	* gcc.dg/debug/pr36690-2.c: New test.
> 	* gcc.dg/debug/pr36690-3.c: New test.
> 	* gcc.dg/debug/pr37616.c: New test.
> 	* gcc.dg/debug/dwarf2/pr29609-1.c: New test.
> 	* gcc.dg/debug/dwarf2/pr29609-2.c: New test.
> 	* gcc.dg/debug/dwarf2/pr36690-1.c: New test.
> 	* gcc.dg/debug/dwarf2/pr36690-2.c: New test.
> 	* gcc.dg/debug/dwarf2/pr36690-3.c: New test.
> 	* gcc.dg/debug/dwarf2/pr37616.c: New test.

> +	      if (e->dest != EXIT_BLOCK_PTR)
> +		{
> +		  insn = BB_HEAD (e->dest);
> +		  if (!INSN_P (insn))
> +		    insn = next_insn (insn);
> +		  if (insn && INSN_P (insn)
> +		      && INSN_LOCATOR (insn) == (int) e->goto_locus)
> +		    continue;
> +		}

Why not use next_nonnote_insn instead of next_insn?


> --- gcc/testsuite/gcc.dg/debug/pr37616.c.jj	2008-10-01 14:35:14.000000000 +0200
> +++ gcc/testsuite/gcc.dg/debug/pr37616.c	2008-10-01 15:50:15.000000000 +0200
> @@ -0,0 +1,37 @@
> +/* PR debug/37616 */
> +/* Test that one can put breakpoints onto continue, exitlab and break
> +   and actually see program reaching those breakpoints.  */
> +/* { dg-do run } */
> +/* { dg-options "-O0 -g -dA" } */

I don't understand how this test tests what the comment describes.


> --- gcc/testsuite/gcc.dg/debug/pr36690-2.c.jj	2008-10-01 14:49:26.000000000 +0200
> +++ gcc/testsuite/gcc.dg/debug/pr36690-2.c	2008-10-01 16:06:12.000000000 +0200
> @@ -0,0 +1,37 @@
> +/* PR debug/36690 */
> +/* Verify that breakpoint can be put on goto f1, it is hit and
> +   varz at that spot is defined and contains 5.  Nowhere else
> +   in the function should be varz in the scope.  */

Same here.

> --- gcc/testsuite/gcc.dg/debug/pr36690-1.c.jj	2008-10-01 14:39:33.000000000 +0200
> +++ gcc/testsuite/gcc.dg/debug/pr36690-1.c	2008-10-01 15:49:57.000000000 +0200
> @@ -0,0 +1,20 @@
> +/* PR debug/36690 */
> +/* Verify that break func is hit.  */

Same here, and others.  What am I missing?


This patch is OK if those tests make sense, with or without the change
to next_nonnote_insn.

Thanks.

Ian

Re: [PATCH] Fix lost locus issues at -O0 -g (PRs debug/29609, debug/36690, debug/37616)

[Jakub Jelinek]

On Tue, Oct 07, 2008 at 09:51:07AM -0700, Ian Lance Taylor wrote:
> > +	      if (e->dest != EXIT_BLOCK_PTR)
> > +		{
> > +		  insn = BB_HEAD (e->dest);
> > +		  if (!INSN_P (insn))
> > +		    insn = next_insn (insn);
> > +		  if (insn && INSN_P (insn)
> > +		      && INSN_LOCATOR (insn) == (int) e->goto_locus)
> > +		    continue;
> > +		}
> 
> Why not use next_nonnote_insn instead of next_insn?

I was afraid next_nonnote_insn could fall through over a BB note into
a different BB.  Guess I could skip over other notes by hand, or
just iterate through insns, until either INSN_P is found or BB_END (e->dest)
is passed.

> > --- gcc/testsuite/gcc.dg/debug/pr37616.c.jj	2008-10-01 14:35:14.000000000 +0200
> > +++ gcc/testsuite/gcc.dg/debug/pr37616.c	2008-10-01 15:50:15.000000000 +0200
> > @@ -0,0 +1,37 @@
> > +/* PR debug/37616 */
> > +/* Test that one can put breakpoints onto continue, exitlab and break
> > +   and actually see program reaching those breakpoints.  */
> > +/* { dg-do run } */
> > +/* { dg-options "-O0 -g -dA" } */
> 
> I don't understand how this test tests what the comment describes.

The debug/pr*.c tests obviously only test whether you can compile/link/run the
program, the debug/dwarf2/pr*.c test are almost identical and test whether
the desired locus is emitted and the intent is that the same tests will be
used in gdb testsuite where it can be all tested more thoroughly.

	Jakub

Re: [PATCH] Fix lost locus issues at -O0 -g (PRs debug/29609, debug/36690, debug/37616)

[Ian Lance Taylor]

Jakub Jelinek <jakub@redhat.com> writes:

> I was afraid next_nonnote_insn could fall through over a BB note into
> a different BB.  Guess I could skip over other notes by hand, or
> just iterate through insns, until either INSN_P is found or BB_END (e->dest)
> is passed.

OK, fair enough.


>> > --- gcc/testsuite/gcc.dg/debug/pr37616.c.jj	2008-10-01 14:35:14.000000000 +0200
>> > +++ gcc/testsuite/gcc.dg/debug/pr37616.c	2008-10-01 15:50:15.000000000 +0200
>> > @@ -0,0 +1,37 @@
>> > +/* PR debug/37616 */
>> > +/* Test that one can put breakpoints onto continue, exitlab and break
>> > +   and actually see program reaching those breakpoints.  */
>> > +/* { dg-do run } */
>> > +/* { dg-options "-O0 -g -dA" } */
>> 
>> I don't understand how this test tests what the comment describes.
>
> The debug/pr*.c tests obviously only test whether you can compile/link/run the
> program, the debug/dwarf2/pr*.c test are almost identical and test whether
> the desired locus is emitted and the intent is that the same tests will be
> used in gdb testsuite where it can be all tested more thoroughly.

Please add appropriate comments to the tests.

OK in any case.

Thanks.

Ian

Re: [PATCH] Fix lost locus issues at -O0 -g (PRs debug/29609, debug/36690, debug/37616)

[Jakub Jelinek]

Hi!

While backporting the patch to 4.3-RH, I've discovered one case where I
haven't handled the goto_locus(location_t)+goto_block -> goto_locus(int locator)
translation, which on the trunk didn't trigger on any of the testcases, but
could, and on 4.3 branch it did.

If a GIMPLE_COND branches with both edges to bb's other than next, and the
true edge has goto_locus set, that one isn't translated, as the caller's
code to translate goto_locus isn't reached when expand_gimple_cond returns
non-NULL.

Fixed thusly, ok for trunk?

Slightly off topic, I wonder whether in the out of cfglayout !optimize code
I shouldn't compare locator's block and location_t separately instead of
just comparing the locators, as it is possible to have different RTL
locators for the same < location_t, block > pair.
There isn't a function to compare two locators, nor there is a function
to get a block from locator, only from insn, so I guess I'd have to add
that.
But this leads to questions about the internal representation of the
RTL locators.  Currently we have 2 pairs of vectors, one used separately for
location_t and one for block, always one vector in the pair has int values
and the other has location_t (also 32-bit) or tree (32-bit or 64-bit
depending on host) and the lookup looks quite expensive (binary search).
For 32-bit hosts, wouldn't just using 2 vectors, one with location_t and one
with tree, both indexed by the RTL locator int, be always more compact (and
obviously faster)?  For 64-bit hosts perhaps this could be slightly bigger
than current representation if there are far more location_t changes than
block changes (likely), but at least couldn't we avoid the binary
search for the location_t side and just use directly indexed vector?

2008-10-08  Jakub Jelinek  <jakub@redhat.com>

	* cfgexpand.c (expand_gimple_cond): Convert also goto_block and
	goto_locus of true_edge into RTL locator.

--- gcc/cfgexpand.c.jj	2008-10-08 11:50:40.000000000 +0200
+++ gcc/cfgexpand.c	2008-10-08 11:52:27.000000000 +0200
@@ -1725,6 +1725,14 @@ expand_gimple_cond (basic_block bb, gimp
 
   maybe_dump_rtl_for_gimple_stmt (stmt, last2);
 
+  if (true_edge->goto_locus)
+    {
+      set_curr_insn_source_location (true_edge->goto_locus);
+      set_curr_insn_block (true_edge->goto_block);
+      true_edge->goto_locus = curr_insn_locator ();
+    }
+  true_edge->goto_block = NULL;
+
   ggc_free (pred);
   return new_bb;
 }


	Jakub

Re: [PATCH] Fix lost locus issues at -O0 -g (PRs debug/29609, debug/36690, debug/37616)

[Eric Botcazou]

> While backporting the patch to 4.3-RH, I've discovered one case where I
> haven't handled the goto_locus(location_t)+goto_block -> goto_locus(int
> locator) translation, which on the trunk didn't trigger on any of the
> testcases, but could, and on 4.3 branch it did.

Slightly off topic, but what about backporting the patch to the official 4.3 
branch as well?  The PRs are marked as regressions for it and IMHO they are 
serious as far as debuggability is concerned.  We (AdaCore) are seeing them 
in our 4.3 compiler so we'll need to do something too.  Rather than having 
each distributor backport the patch internally, why not do it officially?

[AdaCore uses the official 4.3 branch as its codebase so it gets tested, at 
least for Ada, on a bunch of architectures, both compiler and debugger.]

-- 
Eric Botcazou

Re: [PATCH] Fix lost locus issues at -O0 -g (PRs debug/29609, debug/36690, debug/37616)

[Richard Guenther]

On Wed, Oct 8, 2008 at 12:42 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> While backporting the patch to 4.3-RH, I've discovered one case where I
>> haven't handled the goto_locus(location_t)+goto_block -> goto_locus(int
>> locator) translation, which on the trunk didn't trigger on any of the
>> testcases, but could, and on 4.3 branch it did.
>
> Slightly off topic, but what about backporting the patch to the official 4.3
> branch as well?  The PRs are marked as regressions for it and IMHO they are
> serious as far as debuggability is concerned.  We (AdaCore) are seeing them
> in our 4.3 compiler so we'll need to do something too.  Rather than having
> each distributor backport the patch internally, why not do it officially?

FYI I think this would be a good idea.

Thanks,
Richard.

> [AdaCore uses the official 4.3 branch as its codebase so it gets tested, at
> least for Ada, on a bunch of architectures, both compiler and debugger.]
>
> --
> Eric Botcazou
>

Re: [PATCH] Fix lost locus issues at -O0 -g (PRs debug/29609, debug/36690, debug/37616)

[Jakub Jelinek]

On Wed, Oct 08, 2008 at 12:49:09PM +0200, Richard Guenther wrote:
> On Wed, Oct 8, 2008 at 12:42 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> >> While backporting the patch to 4.3-RH, I've discovered one case where I
> >> haven't handled the goto_locus(location_t)+goto_block -> goto_locus(int
> >> locator) translation, which on the trunk didn't trigger on any of the
> >> testcases, but could, and on 4.3 branch it did.
> >
> > Slightly off topic, but what about backporting the patch to the official 4.3
> > branch as well?  The PRs are marked as regressions for it and IMHO they are
> > serious as far as debuggability is concerned.  We (AdaCore) are seeing them
> > in our 4.3 compiler so we'll need to do something too.  Rather than having
> > each distributor backport the patch internally, why not do it officially?
> 
> FYI I think this would be a good idea.

Sure, can we give it a week on the trunk (and in Fedora development repo) before
submitting the backport though?

	Jakub

Re: [PATCH] Fix lost locus issues at -O0 -g (PRs debug/29609, debug/36690, debug/37616)

[Richard Guenther]

On Wed, Oct 8, 2008 at 12:59 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Oct 08, 2008 at 12:49:09PM +0200, Richard Guenther wrote:
>> On Wed, Oct 8, 2008 at 12:42 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> >> While backporting the patch to 4.3-RH, I've discovered one case where I
>> >> haven't handled the goto_locus(location_t)+goto_block -> goto_locus(int
>> >> locator) translation, which on the trunk didn't trigger on any of the
>> >> testcases, but could, and on 4.3 branch it did.
>> >
>> > Slightly off topic, but what about backporting the patch to the official 4.3
>> > branch as well?  The PRs are marked as regressions for it and IMHO they are
>> > serious as far as debuggability is concerned.  We (AdaCore) are seeing them
>> > in our 4.3 compiler so we'll need to do something too.  Rather than having
>> > each distributor backport the patch internally, why not do it officially?
>>
>> FYI I think this would be a good idea.
>
> Sure, can we give it a week on the trunk (and in Fedora development repo) before
> submitting the backport though?

Sure.

Richard.

Re: [PATCH] Fix lost locus issues at -O0 -g (PRs debug/29609, debug/36690, debug/37616)

[Ian Lance Taylor]

Jakub Jelinek <jakub@redhat.com> writes:

> But this leads to questions about the internal representation of the
> RTL locators.  Currently we have 2 pairs of vectors, one used separately for
> location_t and one for block, always one vector in the pair has int values
> and the other has location_t (also 32-bit) or tree (32-bit or 64-bit
> depending on host) and the lookup looks quite expensive (binary search).
> For 32-bit hosts, wouldn't just using 2 vectors, one with location_t and one
> with tree, both indexed by the RTL locator int, be always more compact (and
> obviously faster)?  For 64-bit hosts perhaps this could be slightly bigger
> than current representation if there are far more location_t changes than
> block changes (likely), but at least couldn't we avoid the binary
> search for the location_t side and just use directly indexed vector?

Yes, the current approach has been clearly broken since we went to
mapped locations.  I think the ideal approach would be to encode the
binding-block number into the location_t, and have a vector mapping
those numbers to binding blocks.  That would give us relatively
efficient access to all the information we need for minimal storage.
We could then proceed to eliminate the block field in
gimple_statement_base and the block field in tree_exp.  The downside
would be that on very large files we could run out of bits in
location_t, so we might need to have an escape encoding.  The escape
encoding would cause us to look in a pointer_map from
trees/gimples/RTL to a binding block.  That would require some
attention any time we copied a location.  Fortunately the logic could
be encapsulated in a single shared function, and there aren't that
many plases where we copy a location.

Also, the approach of using reemit_insn_block_notes seems nuts in
today's compiler; block notes are basically only used by
final_scan_insn, which could certainly do the same thing as
reemit_insn_block_notes without using the extra memory.


> 2008-10-08  Jakub Jelinek  <jakub@redhat.com>
>
> 	* cfgexpand.c (expand_gimple_cond): Convert also goto_block and
> 	goto_locus of true_edge into RTL locator.

This patch is OK.

Thanks.

Ian