This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][debug] Handle references to skipped params in remap_ssa_name
On Thu, Jul 5, 2018 at 1:25 PM Tom de Vries <tdevries@suse.de> wrote:
>
> [ was: Re: [testsuite/guality, committed] Prevent optimization of local in
> vla-1.c ]
>
> On Wed, Jul 04, 2018 at 02:32:27PM +0200, Tom de Vries wrote:
> > On 07/03/2018 11:05 AM, Tom de Vries wrote:
> > > On 07/02/2018 10:16 AM, Jakub Jelinek wrote:
> > >> On Mon, Jul 02, 2018 at 09:44:04AM +0200, Richard Biener wrote:
> > >>> Given the array has size i + 1 it's upper bound should be 'i' and 'i'
> > >>> should be available via DW_OP_[GNU_]entry_value.
> > >>>
> > >>> I see it is
> > >>>
> > >>> <175> DW_AT_upper_bound : 10 byte block: 75 1 8 20 24 8 20 26 31
> > >>> 1c (DW_OP_breg5 (rdi): 1; DW_OP_const1u: 32; DW_OP_shl;
> > >>> DW_OP_const1u: 32; DW_OP_shra; DW_OP_lit1; DW_OP_minus)
> > >>>
> > >>> and %rdi is 1. Not sure why gdb fails to print it's length. Yes, the
> > >>> storage itself doesn't have a location but the
> > >>> type specifies the size.
> > >>>
> > >>> (gdb) ptype a
> > >>> type = char [variable length]
> > >>> (gdb) p sizeof(a)
> > >>> $3 = 0
> > >>>
> > >>> this looks like a gdb bug to me?
> > >>>
> > >
> > > With gdb patch:
> > > ...
> > > diff --git a/gdb/findvar.c b/gdb/findvar.c
> > > index 8ad5e25cb2..ebaff923a1 100644
> > > --- a/gdb/findvar.c
> > > +++ b/gdb/findvar.c
> > > @@ -789,6 +789,8 @@ default_read_var_value
> > > break;
> > >
> > > case LOC_OPTIMIZED_OUT:
> > > + if (is_dynamic_type (type))
> > > + type = resolve_dynamic_type (type, NULL,
> > > + /* Unused address. */ 0);
> > > return allocate_optimized_out_value (type);
> > >
> > > default:
> > > ...
> > >
> > > I get:
> > > ...
> > > $ ./gdb -batch -ex "b f1" -ex "r" -ex "p sizeof (a)" vla-1.exe
> > > Breakpoint 1 at 0x4004a8: file vla-1.c, line 17.
> > >
> > > Breakpoint 1, f1 (i=i@entry=5) at vla-1.c:17
> > > 17 return a[0];
> > > $1 = 6
> > > ...
> > >
> >
> > Well, for -O1 and -O2.
> >
> > For O3, I get instead:
> > ...
> > $ ./gdb vla-1.exe -q -batch -ex "b f1" -ex "run" -ex "p sizeof (a)"
> > Breakpoint 1 at 0x4004b0: f1. (2 locations)
> >
> > Breakpoint 1, f1 (i=5) at vla-1.c:17
> > 17 return a[0];
> > $1 = 0
> > ...
> >
>
> Hi,
>
> When compiling guality/vla-1.c with -O3 -g, vla 'a[i + 1]' in f1 is optimized
> away, but f1 still contains a debug expression describing the upper bound of the
> vla (D.1914):
> ...
> __attribute__((noinline))
> f1 (intD.6 iD.1900)
> {
> <bb 2>
> saved_stack.1_2 = __builtin_stack_save ();
> # DEBUG BEGIN_STMT
> # DEBUG D#3 => i_1(D) + 1
> # DEBUG D#2 => (long intD.8) D#3
> # DEBUG D#1 => D#2 + -1
> # DEBUG D.1914 => (sizetype) D#1
> ...
>
> Then f1 is cloned to a version f1.constprop with no parameters, eliminating
> parameter i, and 'DEBUG D#3 => i_1(D) + 1' turns into 'D#3 => NULL'.
> Consequently, 'print sizeof (a)' yields '0' in gdb.
So does gdb correctly recognize there isn't any size available or do we somehow
generate invalid debug info, not recognizing that D#3 => NULL means
"optimized out" and thus all dependent expressions are "optimized out" as well?
That is, shouldn't gdb do
(gdb) print sizeof (a)
<optimized out>
?
> This patch fixes that by recognizing eliminated parameters in remap_ssa_name,
> defining a debug expression linking back to the the eliminated parameter, and
> using that debug expression to replace references to the eliminated parameter:
> ...
> __attribute__((noinline))
> f1.constprop ()
> {
> intD.6 iD.1949;
>
> <bb 3>
> # DEBUG D#8 s=> iD.1900
> # DEBUG iD.1949 => D#8
>
> <bb 2>
> + # DEBUG D#6 s=> iD.1900
> saved_stack.1_1 = __builtin_stack_save ();
> # DEBUG BEGIN_STMT
> - # DEBUG D#3 => NULL
> + # DEBUG D#3 => D#6 + 1
> # DEBUG D#2 => (long intD.8) D#3
> # DEBUG D#1 => D#2 + -1
> # DEBUG D.1951 => (sizetype) D#1
> ...
>
> The inserted debug expression (D#6) is a duplicate of the debug expression
> that will be inserted after copy_body in tree_function_versioning (D#8), so the
> patch contains a todo to fix the duplication.
>
> Bootstrapped and reg-tested on x86_64.
>
> OK for trunk?
>
> Thanks,
> - Tom
>
> [debug] Handle references to skipped params in remap_ssa_name
>
> 2018-07-05 Tom de Vries <tdevries@suse.de>
>
> * tree-inline.c (remap_ssa_name): Handle references to skipped
> params.
>
> ---
> gcc/tree-inline.c | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
> index 427ef959740..0fa996cab49 100644
> --- a/gcc/tree-inline.c
> +++ b/gcc/tree-inline.c
> @@ -204,11 +204,22 @@ remap_ssa_name (tree name, copy_body_data *id)
> gimple *def_temp;
> gimple_stmt_iterator gsi;
> tree val = SSA_NAME_VAR (name);
> + bool skipped_parm_decl = false;
>
> n = id->decl_map->get (val);
> if (n != NULL)
> - val = *n;
> - if (TREE_CODE (val) != PARM_DECL)
> + {
> + if (TREE_CODE (*n) == DEBUG_EXPR_DECL)
> + return *n;
> +
> + if (TREE_CODE (*n) == VAR_DECL
> + && DECL_ABSTRACT_ORIGIN (*n)
> + && TREE_CODE (DECL_ABSTRACT_ORIGIN (*n)) == PARM_DECL)
> + skipped_parm_decl = true;
> + else
> + val = *n;
> + }
> + if (TREE_CODE (val) != PARM_DECL && !skipped_parm_decl)
I wonder if this cannot be more easily set up in copy_arguments_for_versioning
which already does
else if (!id->decl_map->get (arg))
{
/* Make an equivalent VAR_DECL. If the argument was used
as temporary variable later in function, the uses will be
replaced by local variable. */
tree var = copy_decl_to_var (arg, id);
insert_decl_map (id, arg, var);
/* Declare this new variable. */
DECL_CHAIN (var) = *vars;
*vars = var;
}
which just misses to re-map the default def of the PARM_DECL (in case it exists)
to the same(?) var?
All remaining uses should be in debug stmts (I hope).
Richard.
> {
> processing_debug_stmt = -1;
> return name;
> @@ -219,6 +230,8 @@ remap_ssa_name (tree name, copy_body_data *id)
> SET_DECL_MODE (vexpr, DECL_MODE (SSA_NAME_VAR (name)));
> gsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)));
> gsi_insert_before (&gsi, def_temp, GSI_SAME_STMT);
> + // Todo: Reuse vexpr in tree_function_versioning
> + insert_decl_map (id, val, vexpr);
> return vexpr;
> }
>