Fix up DW_TAG_formal_parameter placement

Hi!

With the PR54519 patch I've just posted, I've noticed, I've noticed on the
same testcase from yesterday's IRC:
static inline void foo (int x, int y) { asm volatile ("nop"); }
static inline void bar (int z) { foo (z, 0); foo (z, 1); }
int main ()
{
  bar (0);
  bar (1);
  return 0;
}
that while I can print x and y just fine, if I do bt, x, y and z printed
in the backtrace are all optimized out.
The problem is that first tree versioning for foo.isra.* or bar.isra.*
deposits the optimized away parameters as VAR_DECLs into the DECL_INITIAL
block (which is fine), but then during inlining they end up in the remapped
block of DECL_INITIAL, not the new block added above it into which inliner
puts parameters.  So in the debug info we have
DW_TAG_inlined_subroutine
  DW_TAG_formal_parameter for non-optimized away parameters
  DW_TAG_lexical_block
    DW_TAG_formal_parameter for optimized away parameters
and the debugger (expectably) looks only at DW_TAG_inlined_subroutine
DIE's immediate children for the formal parameters to print during
backtrace.
Fixed up by moving the VAR_DECLs for parameters optimized away by versioning
to BLOCK_SUPERCONTEXT during inlining, at that point we know both of the
blocks have the same scope, and if the original DECL_INITIAL doesn't contain
any other vars, we can actually end up with shorter/more correct debug info
as well as memory savings due to being able to GC the remapped DECL_INITIAL
block.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2012-10-03  Jakub Jelinek  <jakub@redhat.com>

	* tree-inline.c (expand_call_inline): Move VAR_DECLs with
	PARM_DECL origins from remapped DECL_INITIAL's BLOCK_VARS
	into id->block's BLOCK_VARS.



	Jakub

On Wed, 3 Oct 2012, Jakub Jelinek wrote:

> Hi!
> 
> With the PR54519 patch I've just posted, I've noticed, I've noticed on the
> same testcase from yesterday's IRC:
> static inline void foo (int x, int y) { asm volatile ("nop"); }
> static inline void bar (int z) { foo (z, 0); foo (z, 1); }
> int main ()
> {
>   bar (0);
>   bar (1);
>   return 0;
> }
> that while I can print x and y just fine, if I do bt, x, y and z printed
> in the backtrace are all optimized out.
> The problem is that first tree versioning for foo.isra.* or bar.isra.*
> deposits the optimized away parameters as VAR_DECLs into the DECL_INITIAL
> block (which is fine), but then during inlining they end up in the remapped
> block of DECL_INITIAL, not the new block added above it into which inliner
> puts parameters.  So in the debug info we have
> DW_TAG_inlined_subroutine
>   DW_TAG_formal_parameter for non-optimized away parameters
>   DW_TAG_lexical_block
>     DW_TAG_formal_parameter for optimized away parameters
> and the debugger (expectably) looks only at DW_TAG_inlined_subroutine
> DIE's immediate children for the formal parameters to print during
> backtrace.
> Fixed up by moving the VAR_DECLs for parameters optimized away by versioning
> to BLOCK_SUPERCONTEXT during inlining, at that point we know both of the
> blocks have the same scope, and if the original DECL_INITIAL doesn't contain
> any other vars, we can actually end up with shorter/more correct debug info
> as well as memory savings due to being able to GC the remapped DECL_INITIAL
> block.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

This looks like the wrong place to fix things to me ... either we can
fix this at the point we create the VAR_DECLs for the optimized away
PARM_DECLs (or we should delay that until here?) or we fix it up
in dwarf2out.c (how does this fix interact with stabs and the other
debuginfo formats?  mentioning DWARF in tree-inline looks odd,
unless we get rid of the other formats - something I'd of course
welcome ;))

Thanks,
Richard.

> 2012-10-03  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* tree-inline.c (expand_call_inline): Move VAR_DECLs with
> 	PARM_DECL origins from remapped DECL_INITIAL's BLOCK_VARS
> 	into id->block's BLOCK_VARS.
> 
> --- gcc/tree-inline.c.jj	2012-10-02 17:43:13.000000000 +0200
> +++ gcc/tree-inline.c	2012-10-02 19:43:52.576382413 +0200
> @@ -3946,7 +3946,29 @@ expand_call_inline (basic_block bb, gimp
>    initialize_inlined_parameters (id, stmt, fn, bb);
>  
>    if (DECL_INITIAL (fn))
> -    prepend_lexical_block (id->block, remap_blocks (DECL_INITIAL (fn), id));
> +    {
> +      tree *var;
> +
> +      prepend_lexical_block (id->block, remap_blocks (DECL_INITIAL (fn), id));
> +      gcc_checking_assert (BLOCK_SUBBLOCKS (id->block)
> +			   && (BLOCK_CHAIN (BLOCK_SUBBLOCKS (id->block))
> +			       == NULL_TREE));
> +      /* Move vars for PARM_DECLs from DECL_INITIAL block to id->block,
> +	 otherwise DW_TAG_formal_parameter will not be children of
> +	 DW_TAG_inlined_subroutine, but of a DW_TAG_lexical_block
> +	 under it.  The parameters can be then evaluated in the debugger,
> +	 but don't show in backtraces.  */
> +      for (var = &BLOCK_VARS (BLOCK_SUBBLOCKS (id->block)); *var; )
> +	if (TREE_CODE (DECL_ORIGIN (*var)) == PARM_DECL)
> +	  {
> +	    tree v = *var;
> +	    *var = TREE_CHAIN (v);
> +	    TREE_CHAIN (v) = BLOCK_VARS (id->block);
> +	    BLOCK_VARS (id->block) = v;
> +	  }
> +	else
> +	  var = &TREE_CHAIN (*var);
> +    }
>  
>    /* Return statements in the function body will be replaced by jumps
>       to the RET_LABEL.  */
> 
> 
> 	Jakub
> 
>

On Thu, Oct 04, 2012 at 09:42:59AM +0200, Richard Guenther wrote:
> This looks like the wrong place to fix things to me ... either we can
> fix this at the point we create the VAR_DECLs for the optimized away
> PARM_DECLs (or we should delay that until here?)

No, that is not possible.  There is no other block they could be added
to (they are added to DECL_INITIAL block), and they definitely need to
be added there, they are needed for the more common case where it is not inlined.
And in that case it is the right location, for non-inlined function
DECL_INITIAL block's BLOCK_VARS is added directly as children of the
DW_TAG_subprogram.

> or we fix it up
> in dwarf2out.c (how does this fix interact with stabs and the other
> debuginfo formats?

We can't do that either, dwarf2out doesn't have information whether blocks
are really used (as in, any insns/stmts mention that block in
INSN_BLOCK/gimple_block) or not, it is only correct to move the VAR_DECLs
with PARM_DECL DECL_ORIGIN (i.e. DW_TAG_formal_parameter) up if the outer
BLOCK is not referenced by any insn/stmt (i.e. if the ranges of the
inner block with the VAR_DECL and outer block are exactly the same).
If the outer block has range that is superset of the inner block's range,
then the move would invalidly say that the DW_TAG_formal_parameter
is available somewhere where it is not supposed to be available.

Initially I thought I'd do the moves in tree-ssa-live.c, in
remove_unused_scope_block_p it has information about what blocks are used
by any stmts and what are not.  But it would be terribly expensive,
for each VAR_DECL in a block where its BLOCK_SUPERCONTEXT wasn't originally
TREE_USED before remove_unused_scope_block_p (and such blocks up to a
!TREE_USED inlined_function_outer_scope_p), it would need to search
all the BLOCK_SUPERCONTEXT BLOCK_VARS to see if the VAR_DECL isn't present
there as well, and only if not, move to the inlined_function_outer_scope_p
BLOCK.

Doing it in tree-inline.c is IMHO the right spot, it is the place that
creates the extra artificial BLOCK around the remapped DECL_INITIAL block
and puts function arguments there.  At that point we know for sure that
the DECL_INITIAL block has the same ranges as the whole inline function,
and it is desirable to move all arguments to the outer block, not just
those that were still present in DECL_ARGUMENTS during inlining.

If you want to be more specific on what is to be moved, we could either
add some VAR_DECL flag bit (but that is expensive, we don't have many),
or perhaps just check that DECL_CONTEXT (DECL_ORIGIN (v)) == DECL_ORIGIN (fn)
(but is that ever false?).

>  mentioning DWARF in tree-inline looks odd,
> unless we get rid of the other formats - something I'd of course
> welcome ;))

That can be fixed, I can replace the DWARF terminology with something more
fuzzy.

	Jakub

On Thu, 4 Oct 2012, Jakub Jelinek wrote:

> On Thu, Oct 04, 2012 at 09:42:59AM +0200, Richard Guenther wrote:
> > This looks like the wrong place to fix things to me ... either we can
> > fix this at the point we create the VAR_DECLs for the optimized away
> > PARM_DECLs (or we should delay that until here?)
> 
> No, that is not possible.  There is no other block they could be added
> to (they are added to DECL_INITIAL block), and they definitely need to
> be added there, they are needed for the more common case where it is not inlined.
> And in that case it is the right location, for non-inlined function
> DECL_INITIAL block's BLOCK_VARS is added directly as children of the
> DW_TAG_subprogram.
> 
> > or we fix it up
> > in dwarf2out.c (how does this fix interact with stabs and the other
> > debuginfo formats?
> 
> We can't do that either, dwarf2out doesn't have information whether blocks
> are really used (as in, any insns/stmts mention that block in
> INSN_BLOCK/gimple_block) or not, it is only correct to move the VAR_DECLs
> with PARM_DECL DECL_ORIGIN (i.e. DW_TAG_formal_parameter) up if the outer
> BLOCK is not referenced by any insn/stmt (i.e. if the ranges of the
> inner block with the VAR_DECL and outer block are exactly the same).
> If the outer block has range that is superset of the inner block's range,
> then the move would invalidly say that the DW_TAG_formal_parameter
> is available somewhere where it is not supposed to be available.
> 
> Initially I thought I'd do the moves in tree-ssa-live.c, in
> remove_unused_scope_block_p it has information about what blocks are used
> by any stmts and what are not.  But it would be terribly expensive,
> for each VAR_DECL in a block where its BLOCK_SUPERCONTEXT wasn't originally
> TREE_USED before remove_unused_scope_block_p (and such blocks up to a
> !TREE_USED inlined_function_outer_scope_p), it would need to search
> all the BLOCK_SUPERCONTEXT BLOCK_VARS to see if the VAR_DECL isn't present
> there as well, and only if not, move to the inlined_function_outer_scope_p
> BLOCK.
> 
> Doing it in tree-inline.c is IMHO the right spot, it is the place that
> creates the extra artificial BLOCK around the remapped DECL_INITIAL block
> and puts function arguments there.  At that point we know for sure that
> the DECL_INITIAL block has the same ranges as the whole inline function,
> and it is desirable to move all arguments to the outer block, not just
> those that were still present in DECL_ARGUMENTS during inlining.
> 
> If you want to be more specific on what is to be moved, we could either
> add some VAR_DECL flag bit (but that is expensive, we don't have many),
> or perhaps just check that DECL_CONTEXT (DECL_ORIGIN (v)) == DECL_ORIGIN (fn)
> (but is that ever false?).
> 
> >  mentioning DWARF in tree-inline looks odd,
> > unless we get rid of the other formats - something I'd of course
> > welcome ;))
> 
> That can be fixed, I can replace the DWARF terminology with something more
> fuzzy.

Ok, I think I know better what I was after now thinking about this
more.

   initialize_inlined_parameters (id, stmt, fn, bb);

   if (DECL_INITIAL (fn))
-    prepend_lexical_block (id->block, remap_blocks (DECL_INITIAL (fn), 
id));

the context shows it, initialize_inlined_parameters would be
the natural place to "inline" optimized out parameters
represented as VAR_DECLs with PARM_DECL DECL_ORIGIN.

Of course the way remap_blocks works makes this difficult,
if not impossible (or more ugly than your solution).

So in the end your patch is ok as-is.

Thanks,
Richard.