Re: [PATCH] Fix debug info for expr and jump stmt

[Michael Matz <matz at suse dot de>]

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.