Patchwork Fix debug info for expr and jump stmt

login
register
mail settings
Submitter Dehao Chen
Date Oct. 25, 2012, 5:09 p.m.
Message ID <CAO2gOZXJBsEZQQyaJxoxvqs+=NxfJOi1GGxX8DcEG8k2SVn8Ng@mail.gmail.com>
Download mbox | patch
Permalink /patch/194257/
State New
Headers show

Comments

Dehao Chen - Oct. 25, 2012, 5:09 p.m.
Hi,

This patch fixes debug info for expr and jump stmt.

Bootstrapped and passed gcc regression tests.

Is it okay for trunk?

Thanks,
Dehao

gcc/ChangeLog:
2012-10-25  Dehao Chen  <dehao@google.com>

* tree-eh.c (do_return_redirection): Set location for jump statement.
(do_goto_redirection): Likewise.
(frob_into_branch_around): Likewise.
(lower_try_finally_nofallthru): Likewise.
(lower_try_finally_copy): Likewise.
(lower_try_finally_switch): Likewise.
* cfgexpand.c (set_expr_location_r): New callback function.
(gimple_assign_rhs_to_tree): Walk the expr recursively.
(expand_call_stmt): Likewise.
(expand_gimple_stmt_1): Likewise.
Eric Botcazou - Oct. 25, 2012, 6:06 p.m.
> This patch fixes debug info for expr and jump stmt.

It would be nice to have testcases...

> * cfgexpand.c (set_expr_location_r): New callback function.
> (gimple_assign_rhs_to_tree): Walk the expr recursively.
> (expand_call_stmt): Likewise.
> (expand_gimple_stmt_1): Likewise.

This cannot be right.  You're going to propagate a toplevel location above 
locations at deeper level that can be different and should prevail.  It would 
be far better to set the right location at node creation instead of patching 
it up during RTL expansion.
Tom Tromey - Oct. 25, 2012, 6:11 p.m.
>>>>> "Dehao" == Dehao Chen <dehao@google.com> writes:

Dehao> This patch fixes debug info for expr and jump stmt.
Dehao> Bootstrapped and passed gcc regression tests.
Dehao> Is it okay for trunk?

I wonder whether this affects the gdb test suite results.

I'm not trying to pick on you specifically, but there's been a few
debuginfo regressions lately that would have been caught by running the
gdb test suite against the compiler patch.

We do catch these in gdb, but it is a pain for us to track down each one
and file it in gcc bugzilla.  It would be more efficient for gcc patch
authors to do this, at least in the "obvious" case where someone is
working on a patch intended to affect debuginfo.

thanks,
Tom
Dehao Chen - Oct. 25, 2012, 6:50 p.m.
On Thu, Oct 25, 2012 at 11:06 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> This patch fixes debug info for expr and jump stmt.
>
> It would be nice to have testcases...

Sure, I'll try to forge some testcases for this.

>
>> * cfgexpand.c (set_expr_location_r): New callback function.
>> (gimple_assign_rhs_to_tree): Walk the expr recursively.
>> (expand_call_stmt): Likewise.
>> (expand_gimple_stmt_1): Likewise.
>
> This cannot be right.  You're going to propagate a toplevel location above
> locations at deeper level that can be different and should prevail.  It would
> be far better to set the right location at node creation instead of patching
> it up during RTL expansion.

I noticed that in r151350, Micheal changed from set_expr_location_r to
SET_EXPR_LOCATION. Any insights why the recursive call is replaced?
Looks to me if we reset the location for first level expr here, there
is no reason that we don't reset expr for deeper levels. Or shall we
simply remove SET_EXPR_LOCATION?

Thanks,
Dehao
>
> --
> Eric Botcazou
Dehao Chen - Oct. 25, 2012, 6:53 p.m.
On Thu, Oct 25, 2012 at 11:11 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Dehao" == Dehao Chen <dehao@google.com> writes:
>
> Dehao> This patch fixes debug info for expr and jump stmt.
> Dehao> Bootstrapped and passed gcc regression tests.
> Dehao> Is it okay for trunk?
>
> I wonder whether this affects the gdb test suite results.

My guess is this patch will fix several regressions there.

>
> I'm not trying to pick on you specifically, but there's been a few
> debuginfo regressions lately that would have been caught by running the
> gdb test suite against the compiler patch.
>
> We do catch these in gdb, but it is a pain for us to track down each one
> and file it in gcc bugzilla.  It would be more efficient for gcc patch
> authors to do this, at least in the "obvious" case where someone is
> working on a patch intended to affect debuginfo.

Yes, I should have run gdb tests before. I'll run gdb tests and try to
fix the regressions that are triggered by the block_location change.

Thanks,
Dehao

>
> thanks,
> Tom
Dehao Chen - Oct. 26, 2012, 1:40 a.m.
Binary search shows that the culprit for the recent gdb regression is
http://gcc.gnu.org/viewcvs?view=revision&revision=192719, it failed
the following tests:

gdb.base/store.exp
gdb.base/restore.exp

The block_location patch also breaks the following tests:

gdb.cp/method.exp

But the error is fixed by this patch I sent. Thus after this patch,
the block_location improvement will not cause regression to gdb tests.

Thanks,
Dehao

On Thu, Oct 25, 2012 at 11:53 AM, Dehao Chen <dehao@google.com> wrote:
> On Thu, Oct 25, 2012 at 11:11 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>> "Dehao" == Dehao Chen <dehao@google.com> writes:
>>
>> Dehao> This patch fixes debug info for expr and jump stmt.
>> Dehao> Bootstrapped and passed gcc regression tests.
>> Dehao> Is it okay for trunk?
>>
>> I wonder whether this affects the gdb test suite results.
>
> My guess is this patch will fix several regressions there.
>
>>
>> I'm not trying to pick on you specifically, but there's been a few
>> debuginfo regressions lately that would have been caught by running the
>> gdb test suite against the compiler patch.
>>
>> We do catch these in gdb, but it is a pain for us to track down each one
>> and file it in gcc bugzilla.  It would be more efficient for gcc patch
>> authors to do this, at least in the "obvious" case where someone is
>> working on a patch intended to affect debuginfo.
>
> Yes, I should have run gdb tests before. I'll run gdb tests and try to
> fix the regressions that are triggered by the block_location change.
>
> Thanks,
> Dehao
>
>>
>> thanks,
>> Tom
Eric Botcazou - Oct. 26, 2012, 1:34 p.m.
> I noticed that in r151350, Micheal changed from set_expr_location_r to
> SET_EXPR_LOCATION. Any insights why the recursive call is replaced?
> Looks to me if we reset the location for first level expr here, there
> is no reason that we don't reset expr for deeper levels. Or shall we
> simply remove SET_EXPR_LOCATION?

Certainly overwriting an existing location with another one looks very wrong.
And even propagating a toplevel location onto nested nodes without location 
can be problematic if you jump over a different location in the tree.

We don't need to put a location on every single node before RTL expansion, 
since RTL insns inherit the current location, so I'm not sure what you are 
trying to achieve here.  Fixing specific issues with changes like the one you 
posted for tree-eh.c is the way to go instead.
Michael Matz - Oct. 26, 2012, 2:11 p.m.
Hi,

On Thu, 25 Oct 2012, Dehao Chen wrote:

> >> * cfgexpand.c (set_expr_location_r): New callback function.
> >> (gimple_assign_rhs_to_tree): Walk the expr recursively.
> >> (expand_call_stmt): Likewise.
> >> (expand_gimple_stmt_1): Likewise.
> >
> > This cannot be right.  You're going to propagate a toplevel location 
> > above locations at deeper level that can be different and should 
> > prevail.  It would be far better to set the right location at node 
> > creation instead of patching it up during RTL expansion.

Indeed.  The cfgexpand.c hunk doesn't make much sense.  What situation are 
you trying to fix up?  In the places were you introduced a walk_tree to 
recursively walk all subtrees there aren't recursive tree structures at 
all, the SET_EXPR_LOCATION use is correct there.

> I noticed that in r151350, Micheal changed from set_expr_location_r to
> SET_EXPR_LOCATION. Any insights why the recursive call is replaced?

Because I removed the only caller of that function.  I replaced it with 
explicit SET_EXPR_LOCATION for the expressions where it made sense, for 
single RHS sides, and call expressions.  Those things can't be recursive 
themself.  As Eric points out the old code was suspicious anyway as it 
simply overwrote any locations of subexpressions, even if they already had 
one set.

> Looks to me if we reset the location for first level expr here, there
> is no reason that we don't reset expr for deeper levels. Or shall we
> simply remove SET_EXPR_LOCATION?

Depends.  What situations were you trying to fix up?


Ciao,
Michael.

Patch

Index: gcc/tree-eh.c
===================================================================
--- gcc/tree-eh.c	(revision 192809)
+++ gcc/tree-eh.c	(working copy)
@@ -739,6 +739,7 @@  do_return_redirection (struct goto_queue_node *q,
     gimple_seq_add_seq (&q->repl_stmt, mod);
 
   x = gimple_build_goto (finlab);
+  gimple_set_location (x, q->location);
   gimple_seq_add_stmt (&q->repl_stmt, x);
 }
 
@@ -758,6 +759,7 @@  do_goto_redirection (struct goto_queue_node *q, tr
     gimple_seq_add_seq (&q->repl_stmt, mod);
 
   x = gimple_build_goto (finlab);
+  gimple_set_location (x, q->location);
   gimple_seq_add_stmt (&q->repl_stmt, x);
 }
 
@@ -857,6 +859,7 @@  frob_into_branch_around (gimple tp, eh_region regi
       if (!over)
 	over = create_artificial_label (loc);
       x = gimple_build_goto (over);
+      gimple_set_location (x, loc);
       gimple_seq_add_stmt (&cleanup, x);
     }
   gimple_seq_add_seq (&eh_seq, cleanup);
@@ -1085,6 +1088,7 @@  lower_try_finally_nofallthru (struct leh_state *st
 	  emit_post_landing_pad (&eh_seq, tf->region);
 
 	  x = gimple_build_goto (lab);
+	  gimple_set_location (x, gimple_location (tf->try_finally_expr));
 	  gimple_seq_add_stmt (&eh_seq, x);
 	}
     }
@@ -1223,6 +1227,7 @@  lower_try_finally_copy (struct leh_state *state, s
 
       tmp = lower_try_finally_fallthru_label (tf);
       x = gimple_build_goto (tmp);
+      gimple_set_location (x, tf_loc);
       gimple_seq_add_stmt (&new_stmt, x);
     }
 
@@ -1395,6 +1400,7 @@  lower_try_finally_switch (struct leh_state *state,
 
       tmp = lower_try_finally_fallthru_label (tf);
       x = gimple_build_goto (tmp);
+      gimple_set_location (x, tf_loc);
       gimple_seq_add_stmt (&switch_body, x);
     }
 
@@ -1423,6 +1429,7 @@  lower_try_finally_switch (struct leh_state *state,
       gimple_seq_add_stmt (&eh_seq, x);
 
       x = gimple_build_goto (finally_label);
+      gimple_set_location (x, tf_loc);
       gimple_seq_add_stmt (&eh_seq, x);
 
       tmp = build_int_cst (integer_type_node, eh_index);
Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c	(revision 192809)
+++ gcc/cfgexpand.c	(working copy)
@@ -58,6 +58,17 @@  gimple currently_expanding_gimple_stmt;
 
 static rtx expand_debug_expr (tree);
 
+/* Call back function to set the expr location as gimple location.  */
+
+static tree
+set_expr_location_r (tree *tp, int *walk_subtrees ATTRIBUTE_UNUSED, void *data)
+{
+  gimple stmt = (gimple) data;
+  if (gimple_has_location (stmt) && CAN_HAVE_LOCATION_P (*tp))
+    SET_EXPR_LOCATION (*tp, gimple_location (stmt));
+  return NULL_TREE;
+}
+
 /* Return an expression tree corresponding to the RHS of GIMPLE
    statement STMT.  */
 
@@ -98,8 +109,7 @@  gimple_assign_rhs_to_tree (gimple stmt)
   else
     gcc_unreachable ();
 
-  if (gimple_has_location (stmt) && CAN_HAVE_LOCATION_P (t))
-    SET_EXPR_LOCATION (t, gimple_location (stmt));
+  walk_tree (&t, set_expr_location_r, stmt, NULL);
 
   return t;
 }
@@ -1990,7 +2000,7 @@  expand_call_stmt (gimple stmt)
   else
     CALL_FROM_THUNK_P (exp) = gimple_call_from_thunk_p (stmt);
   CALL_EXPR_VA_ARG_PACK (exp) = gimple_call_va_arg_pack_p (stmt);
-  SET_EXPR_LOCATION (exp, gimple_location (stmt));
+  walk_tree (&exp, set_expr_location_r, stmt, NULL);
 
   /* Ensure RTL is created for debug args.  */
   if (decl && DECL_HAS_DEBUG_ARGS_P (decl))
@@ -2097,8 +2107,8 @@  expand_gimple_stmt_1 (gimple stmt)
 	    tree rhs = gimple_assign_rhs1 (stmt);
 	    gcc_assert (get_gimple_rhs_class (gimple_expr_code (stmt))
 			== GIMPLE_SINGLE_RHS);
-	    if (gimple_has_location (stmt) && CAN_HAVE_LOCATION_P (rhs))
-	      SET_EXPR_LOCATION (rhs, gimple_location (stmt));
+	    walk_tree (&rhs, set_expr_location_r, stmt, NULL);
+	    walk_tree (&lhs, set_expr_location_r, stmt, NULL);
 	    if (TREE_CLOBBER_P (rhs))
 	      /* This is a clobber to mark the going out of scope for
 		 this LHS.  */