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 <[email protected]>
>
> 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 <[email protected]>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend
