| Message ID | CAO2gOZXJBsEZQQyaJxoxvqs+=NxfJOi1GGxX8DcEG8k2SVn8Ng@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
> 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.
>>>>> "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
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
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
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
> 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.
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.
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. */