This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Improve debug info for partial inlining (PR debug/54519, take 2)


On Wed, 3 Oct 2012, Jakub Jelinek wrote:

> On Tue, Sep 11, 2012 at 03:59:56PM +0200, Jakub Jelinek wrote:
> > As discussed in the PR, right now we do a very bad job for debug info
> > of partially inlined functions (both when they are kept only partially
> > inlined, or when partial inlining is performed, but doesn't seem to be
> > useful and foo.part.N is inlined back, either into the original function, or
> > into a function into which foo has been inlined first).
> > 
> > This patch improves that by doing something similar to what ipa-prop.c does,
> > in particular for arguments that aren't actually passed to foo.part.N
> > we add debug args and corresponding debug bind and debug source bind stmts
> > to provide better debug info (if foo.part.N isn't inlined, then
> > DW_OP_GNU_parameter_ref is going to be used together with corresponding call
> > site arguments).
> > 
> > Bootstrapped/regtested on x86_64-linux and i686-linux, some of the tests
> > still fail with some option combinations, am going to file a DF VTA PR for
> > that momentarily.  Ok for trunk?
> 
> Now that Alex fixed the DF VTA issue, most of the non-LTO FAILs are gone,
> the remaining one on x86_64 looks like a GDB bug, and there is one -Os
> failure on pr54519-3.c which could be either alias oracle or var-tracking
> bug/missing feature - basically there is a non-addressable parameter
> in stack slot, and vt_canon_true_dep -> canon_true_dependence thinks an
> argument push insn might alias with it, because it doesn't have a MEM_EXPR
> and ao_ref_from_mem fails.
> 
> Posting an updated patch, because last night I found that even when the
> patch should have fixed
> 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;
> }
> it didn't, there was a confusion when DECL_ORIGIN should be used and when
> not.  This version of the patch fixes that, on this testcase (adjusted
> added as pr54519-6.c) p x, p y and up; p z now work well.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2012-10-03  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/54519
> 	* ipa-split.c (split_function): Add debug args and
> 	debug source and normal stmts for args_to_skip which are
> 	gimple regs.
> 	* tree-inline.c (copy_debug_stmt): When inlining, adjust
> 	source debug bind stmts to debug binds of corresponding
> 	DEBUG_EXPR_DECL.
> 
> 	* gcc.dg/guality/pr54519-1.c: New test.
> 	* gcc.dg/guality/pr54519-2.c: New test.
> 	* gcc.dg/guality/pr54519-3.c: New test.
> 	* gcc.dg/guality/pr54519-4.c: New test.
> 	* gcc.dg/guality/pr54519-5.c: New test.
> 	* gcc.dg/guality/pr54519-6.c: New test.
> 
> --- gcc/ipa-split.c.jj	2012-09-25 14:26:52.612821323 +0200
> +++ gcc/ipa-split.c	2012-10-02 17:41:31.030357922 +0200
> @@ -1059,6 +1059,7 @@ split_function (struct split_point *spli
>    gimple last_stmt = NULL;
>    unsigned int i;
>    tree arg, ddef;
> +  VEC(tree, gc) **debug_args = NULL;
>  
>    if (dump_file)
>      {
> @@ -1232,6 +1233,65 @@ split_function (struct split_point *spli
>    gimple_set_block (call, DECL_INITIAL (current_function_decl));
>    VEC_free (tree, heap, args_to_pass);
>  

The following could use a comment on what you are doing ...

> +  if (args_to_skip)
> +    for (parm = DECL_ARGUMENTS (current_function_decl), num = 0;
> +	 parm; parm = DECL_CHAIN (parm), num++)
> +      if (bitmap_bit_p (args_to_skip, num)
> +	  && is_gimple_reg (parm))
> +	{
> +	  tree ddecl;
> +	  gimple def_temp;
> +
> +	  arg = get_or_create_ssa_default_def (cfun, parm);
> +	  if (!MAY_HAVE_DEBUG_STMTS)
> +	    continue;
> +	  if (debug_args == NULL)
> +	    debug_args = decl_debug_args_insert (node->symbol.decl);
> +	  ddecl = make_node (DEBUG_EXPR_DECL);
> +	  DECL_ARTIFICIAL (ddecl) = 1;
> +	  TREE_TYPE (ddecl) = TREE_TYPE (parm);
> +	  DECL_MODE (ddecl) = DECL_MODE (parm);
> +	  VEC_safe_push (tree, gc, *debug_args, DECL_ORIGIN (parm));
> +	  VEC_safe_push (tree, gc, *debug_args, ddecl);
> +	  def_temp = gimple_build_debug_bind (ddecl, unshare_expr (arg),
> +					      call);
> +	  gsi_insert_after (&gsi, def_temp, GSI_NEW_STMT);
> +	}
> +  if (debug_args != NULL)
> +    {
> +      unsigned int i;
> +      tree var, vexpr;
> +      gimple_stmt_iterator cgsi;
> +      gimple def_temp;
> +
> +      push_cfun (DECL_STRUCT_FUNCTION (node->symbol.decl));

What do you need the push/pop_cfun for?  I see ENTRY_BLOCK_PTR
(easily fixable), but else?

> +      var = BLOCK_VARS (DECL_INITIAL (node->symbol.decl));
> +      i = VEC_length (tree, *debug_args);
> +      cgsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR));
> +      do
> +	{
> +	  i -= 2;
> +	  while (var != NULL_TREE
> +		 && DECL_ABSTRACT_ORIGIN (var)
> +		    != VEC_index (tree, *debug_args, i))
> +	    var = TREE_CHAIN (var);
> +	  if (var == NULL_TREE)
> +	    break;
> +	  vexpr = make_node (DEBUG_EXPR_DECL);
> +	  parm = VEC_index (tree, *debug_args, i);
> +	  DECL_ARTIFICIAL (vexpr) = 1;
> +	  TREE_TYPE (vexpr) = TREE_TYPE (parm);
> +	  DECL_MODE (vexpr) = DECL_MODE (parm);
> +	  def_temp = gimple_build_debug_source_bind (vexpr, parm,
> +						     NULL);
> +	  gsi_insert_before (&cgsi, def_temp, GSI_SAME_STMT);
> +	  def_temp = gimple_build_debug_bind (var, vexpr, NULL);
> +	  gsi_insert_before (&cgsi, def_temp, GSI_SAME_STMT);
> +	}
> +      while (i);
> +      pop_cfun ();
> +    }
> +
>    /* We avoid address being taken on any variable used by split part,
>       so return slot optimization is always possible.  Moreover this is
>       required to make DECL_BY_REFERENCE work.  */
> --- gcc/tree-inline.c.jj	2012-09-25 14:26:52.576821521 +0200
> +++ gcc/tree-inline.c	2012-10-02 17:43:13.395786581 +0200
> @@ -2374,6 +2374,31 @@ copy_debug_stmt (gimple stmt, copy_body_
>        gimple_debug_source_bind_set_var (stmt, t);
>        walk_tree (gimple_debug_source_bind_get_value_ptr (stmt),
>  		 remap_gimple_op_r, &wi, NULL);
> +      /* When inlining and source bind refers to one of the optimized
> +	 away parameters, change the source bind into normal debug bind
> +	 referring to the corresponding DEBUG_EXPR_DECL that should have
> +	 been bound before the call stmt.  */
> +      t = gimple_debug_source_bind_get_value (stmt);
> +      if (t != NULL_TREE
> +	  && TREE_CODE (t) == PARM_DECL
> +	  && id->gimple_call)
> +	{
> +	  VEC(tree, gc) **debug_args = decl_debug_args_lookup (id->src_fn);
> +	  unsigned int i;
> +	  if (debug_args != NULL)
> +	    {
> +	      for (i = 0; i < VEC_length (tree, *debug_args); i += 2)
> +		if (VEC_index (tree, *debug_args, i) == DECL_ORIGIN (t)
> +		    && TREE_CODE (VEC_index (tree, *debug_args, i + 1))
> +		       == DEBUG_EXPR_DECL)
> +		  {
> +		    t = VEC_index (tree, *debug_args, i + 1);
> +		    gimple_debug_source_bind_set_value (stmt, t);
> +		    stmt->gsbase.subcode = GIMPLE_DEBUG_BIND;

That looks fishy ... shouldn't it be at least the other way around?

                 stmt->gsbase.subcode = GIMPLE_DEBUG_BIND;
                 gimple_debug_bind_set_value (stmt, t);

?

Otherwise this looks ok.

Thanks,
Richard.

> +		    break;
> +		  }
> +	    }
> +	}      
>      }
>  
>    processing_debug_stmt = 0;
> --- gcc/testsuite/gcc.dg/guality/pr54519-2.c.jj	2012-10-02 16:27:24.862658030 +0200
> +++ gcc/testsuite/gcc.dg/guality/pr54519-2.c	2012-10-02 16:27:24.862658030 +0200
> @@ -0,0 +1,45 @@
> +/* PR debug/54519 */
> +/* { dg-do run } */
> +/* { dg-options "-g" } */
> +
> +__attribute__((noinline, noclone)) void
> +fn1 (int x)
> +{
> +  __asm volatile ("" : "+r" (x) : : "memory");
> +}
> +
> +static int
> +fn2 (int x, int y)
> +{
> +  if (y)
> +    {
> +      fn1 (x);		/* { dg-final { gdb-test 17 "x" "6" } } */
> +      fn1 (x);		/* { dg-final { gdb-test 17 "y" "25" } } */
> +      fn1 (x);
> +      fn1 (x);
> +      y = -2 + x;
> +      y = y * y * y + y;
> +      fn1 (x + y);	/* { dg-final { gdb-test 22 "y" "68" } } */
> +    }
> +  return x;
> +}
> +
> +__attribute__((noinline, noclone)) int
> +fn3 (int x, int y)
> +{
> +  return fn2 (x, y) + y;
> +}
> +
> +__attribute__((noinline, noclone)) int
> +fn4 (int x, int y)
> +{
> +  return fn2 (x, y) + y;
> +}
> +
> +int
> +main ()
> +{
> +  fn3 (6, 25);
> +  fn4 (4, 117);
> +  return 0;
> +}
> --- gcc/testsuite/gcc.dg/guality/pr54519-1.c.jj	2012-10-02 16:27:24.862658030 +0200
> +++ gcc/testsuite/gcc.dg/guality/pr54519-1.c	2012-10-02 16:27:24.862658030 +0200
> @@ -0,0 +1,48 @@
> +/* PR debug/54519 */
> +/* { dg-do run } */
> +/* { dg-options "-g" } */
> +
> +__attribute__((noinline, noclone)) void
> +fn1 (int x)
> +{
> +  __asm volatile ("" : "+r" (x) : : "memory");
> +}
> +
> +static int
> +fn2 (int x, int y, int z)
> +{
> +  int a = 8;
> +  if (x != z)
> +    {
> +      fn1 (x);
> +      fn1 (x);		/* { dg-final { gdb-test 20 "x" "36" } } */
> +      if (x == 36)	/* { dg-final { gdb-test 20 "y" "25" } } */
> +	fn1 (x);	/* { dg-final { gdb-test 20 "z" "6" } } */
> +      fn1 (x);		/* { dg-final { gdb-test 23 "x" "98" } } */
> +      if (x == 98)	/* { dg-final { gdb-test 23 "y" "117" } } */
> +	fn1 (x);	/* { dg-final { gdb-test 23 "z" "8" } } */
> +      fn1 (x);
> +      fn1 (x + a);
> +    }
> +  return y;
> +}
> +
> +__attribute__((noinline, noclone)) int
> +fn3 (int x, int y)
> +{
> +  return fn2 (x, y, 6);
> +}
> +
> +__attribute__((noinline, noclone)) int
> +fn4 (int x, int y)
> +{
> +  return fn2 (x, y, 8);
> +}
> +
> +int
> +main ()
> +{
> +  fn3 (36, 25);
> +  fn4 (98, 117);
> +  return 0;
> +}
> --- gcc/testsuite/gcc.dg/guality/pr54519-3.c.jj	2012-10-02 16:27:24.863658017 +0200
> +++ gcc/testsuite/gcc.dg/guality/pr54519-3.c	2012-10-02 16:27:24.863658017 +0200
> @@ -0,0 +1,42 @@
> +/* PR debug/54519 */
> +/* { dg-do run } */
> +/* { dg-options "-g" } */
> +
> +__attribute__((noinline, noclone)) void
> +fn1 (int x)
> +{
> +  __asm volatile ("" : "+r" (x) : : "memory");
> +}
> +
> +static int
> +fn2 (int x, int y, int z)
> +{
> +  int a = 8;
> +  if (x != z)
> +    {
> +      fn1 (x);
> +      fn1 (x);		/* { dg-final { gdb-test 20 "x" "36" } } */
> +      if (x == 36)	/* { dg-final { gdb-test 20 "y" "25" } } */
> +	fn1 (x);	/* { dg-final { gdb-test 20 "z" "6" } } */
> +      fn1 (x);		/* { dg-final { gdb-test 23 "x" "98" } } */
> +      if (x == 98)	/* { dg-final { gdb-test 23 "y" "117" } } */
> +	fn1 (x);	/* { dg-final { gdb-test 23 "z" "8" } } */
> +      fn1 (x);
> +      fn1 (x + a);
> +    }
> +  return y;
> +}
> +
> +int (*p) (int, int, int) = fn2;
> +
> +int
> +main ()
> +{
> +  __asm volatile ("" : : : "memory");
> +  int (*q) (int, int, int) = p;
> +  __asm volatile ("" : : : "memory");
> +  q (36, 25, 6);
> +  q (98, 117, 8);
> +  q (0, 0, 0);
> +  return 0;
> +}
> --- gcc/testsuite/gcc.dg/guality/pr54519-4.c.jj	2012-10-02 16:27:24.863658017 +0200
> +++ gcc/testsuite/gcc.dg/guality/pr54519-4.c	2012-10-02 16:27:24.863658017 +0200
> @@ -0,0 +1,39 @@
> +/* PR debug/54519 */
> +/* { dg-do run } */
> +/* { dg-options "-g" } */
> +
> +__attribute__((noinline, noclone)) void
> +fn1 (int x)
> +{
> +  __asm volatile ("" : "+r" (x) : : "memory");
> +}
> +
> +static int
> +fn2 (int x, int y)
> +{
> +  if (y)
> +    {
> +      fn1 (x);		/* { dg-final { gdb-test 17 "x" "6" } } */
> +      fn1 (x);		/* { dg-final { gdb-test 17 "y" "25" } } */
> +      fn1 (x);
> +      fn1 (x);
> +      y = -2 + x;
> +      y = y * y * y + y;
> +      fn1 (x + y);	/* { dg-final { gdb-test 22 "y" "68" } } */
> +    }
> +  return x;
> +}
> +
> +int (*p) (int, int) = fn2;
> +
> +int
> +main ()
> +{
> +  __asm volatile ("" : : : "memory");
> +  int (*q) (int, int) = p;
> +  __asm volatile ("" : : : "memory");
> +  q (6, 25);
> +  q (4, 117);
> +  q (0, 0);
> +  return 0;
> +}
> --- gcc/testsuite/gcc.dg/guality/pr54519-5.c.jj	2012-10-02 16:27:24.863658017 +0200
> +++ gcc/testsuite/gcc.dg/guality/pr54519-5.c	2012-10-02 16:27:24.863658017 +0200
> @@ -0,0 +1,45 @@
> +/* PR debug/54519 */
> +/* { dg-do run } */
> +/* { dg-options "-g" } */
> +
> +__attribute__((noinline, noclone)) void
> +fn1 (int x)
> +{
> +  __asm volatile ("" : "+r" (x) : : "memory");
> +}
> +
> +static int
> +fn2 (int x, int y)
> +{
> +  if (y)
> +    {
> +      fn1 (x);		/* { dg-final { gdb-test 17 "x" "6" } } */
> +      fn1 (x);		/* { dg-final { gdb-test 17 "y" "25" } } */
> +      fn1 (x);
> +      fn1 (x);
> +      y = -2 + x;
> +      y = y * y * y + y;
> +      fn1 (x + y);	/* { dg-final { gdb-test 22 "y" "68" } } */
> +    }
> +  return x;
> +}
> +
> +__attribute__((noinline, noclone)) int
> +fn3 (int x, int y)
> +{
> +  return fn2 (x, y);
> +}
> +
> +__attribute__((noinline, noclone)) int
> +fn4 (int x, int y)
> +{
> +  return fn2 (x, y);
> +}
> +
> +int
> +main ()
> +{
> +  fn3 (6, 25);
> +  fn4 (4, 117);
> +  return 0;
> +}
> --- gcc/testsuite/gcc.dg/guality/pr54519-6.c.jj	2012-10-02 18:03:45.137732997 +0200
> +++ gcc/testsuite/gcc.dg/guality/pr54519-6.c	2012-10-02 18:03:19.000000000 +0200
> @@ -0,0 +1,27 @@
> +/* PR debug/54519 */
> +/* { dg-do run } */
> +/* { dg-options "-g" } */
> +
> +#include "../nop.h"
> +
> +static inline void
> +f1 (int x, int y)
> +{
> +  asm volatile (NOP);	/* { dg-final { gdb-test 11 "x" "2" } } */
> +  asm volatile (NOP);	/* { dg-final { gdb-test 11 "y" "0" } } */
> +}
> +
> +static inline void
> +f2 (int z)
> +{
> +  f1 (z, 0);
> +  f1 (z, 1);
> +}
> +
> +int
> +main ()
> +{
> +  f2 (2);
> +  f2 (3);
> +  return 0;
> +}
> 
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]