| Message ID | 20180705112558.mphxe2sz7mozvcur@delia |
|---|---|
| State | New |
| Headers | show |
| Series | [debug] Handle references to skipped params in remap_ssa_name | expand |
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; > } >
On 07/05/2018 01:39 PM, Richard Biener wrote: > 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> > > ? The type for the vla gcc is emitting is an DW_TAG_array_type with DW_TAG_subrange_type without DW_AT_upper_bound or DW_AT_count, which makes the upper bound value 'unknown'. So I'd say the debug info is valid. Using this gdb patch: ... diff --git a/gdb/eval.c b/gdb/eval.c index 9db6e7c69d..ea6f782c5b 100644 --- a/gdb/eval.c +++ b/gdb/eval.c @@ -3145,6 +3145,8 @@ evaluate_subexp_for_sizeof (...) { val = evaluate_subexp (NULL_TYPE, exp, pos, EVAL_NORMAL); type = value_type (val); + if (TYPE_LENGTH (type) == 0) + return allocate_optimized_out_value (size_type); } else (*pos) += 4; ... I get: ... $ ./gdb vla-1.exe -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 = <optimized out> ... Thanks, - Tom
On Thu, Jul 5, 2018 at 4:12 PM Tom de Vries <tdevries@suse.de> wrote: > > On 07/05/2018 01:39 PM, Richard Biener wrote: > > 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> > > > > ? > > The type for the vla gcc is emitting is an DW_TAG_array_type with > DW_TAG_subrange_type without DW_AT_upper_bound or DW_AT_count, which > makes the upper bound value 'unknown'. So I'd say the debug info is valid. OK, that sounds reasonable. I wonder if languages like Ada have a way to declare an array type with unknown upper bound but known lower bound. For typedef int arr[]; arr *x; we generate just <1><2d>: Abbrev Number: 2 (DW_TAG_typedef) <2e> DW_AT_name : arr <32> DW_AT_decl_file : 1 <33> DW_AT_decl_line : 1 <34> DW_AT_decl_column : 13 <35> DW_AT_type : <0x39> <1><39>: Abbrev Number: 3 (DW_TAG_array_type) <3a> DW_AT_type : <0x44> <3e> DW_AT_sibling : <0x44> <2><42>: Abbrev Number: 4 (DW_TAG_subrange_type) <2><43>: Abbrev Number: 0 which does (gdb) ptype arr type = int [] (gdb) ptype x type = int (*)[] (gdb) p sizeof (arr) $1 = 0 so I wonder whether the patch makes it print <optimized out> instead? I think both 0 and <optimized out> are less than ideal and maybe <variable size> would be better. In the type case above it's certainly not "optimized out". > Using this gdb patch: > ... > diff --git a/gdb/eval.c b/gdb/eval.c > index 9db6e7c69d..ea6f782c5b 100644 > --- a/gdb/eval.c > +++ b/gdb/eval.c > @@ -3145,6 +3145,8 @@ evaluate_subexp_for_sizeof (...) > { > val = evaluate_subexp (NULL_TYPE, exp, pos, EVAL_NORMAL); > type = value_type (val); > + if (TYPE_LENGTH (type) == 0) > + return allocate_optimized_out_value (size_type); > } > else > (*pos) += 4; > ... > > I get: > ... > $ ./gdb vla-1.exe -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 = <optimized out> > ... > > Thanks, > - Tom
On 07/05/2018 01:39 PM, Richard Biener wrote: > 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: <SNIP> >> [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? I've updated the patch to add a debug expr here in copy_arguments_for_versioning for every parameter that has a default def, and to use that debug expr in remap_ssa_name. > All remaining uses should be in debug stmts (I hope). I ran into a test-case where that was not the case, so I had to handle that in remap_ssa_name, the comment in the patch describes that in more detail. Bootstrapped and reg-tested on x86_64. OK for trunk? Thanks, - Tom [debug] Handle references to skipped params in remap_ssa_name When compiling guality/vla-1.c with -O3 -g, vla a 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. This patch fixes that by defining debug expressions for default defs of eliminated parameters in copy_arguments_for_versioning: ... __attribute__((noinline)) f1.constprop () { intD.6 iD.1949; <bb 3> # DEBUG D#8 s=> iD.1900 # DEBUG iD.1949 => D#8 + # DEBUG D#6 s=> iD.1900 <bb 2> 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. 2018-07-05 Tom de Vries <tdevries@suse.de> * tree-inline.c (create_debug_expr): Factor out of ... (remap_ssa_name): ... here. Use debug expr only for debug uses. (copy_arguments_for_versioning): Add debug expr mapping for skipped param default defs. (tree_function_versioning): Insert statements defining debug exprs. --- gcc/tree-inline.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 58 insertions(+), 8 deletions(-) diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index 427ef959740..11440f49cf0 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -179,6 +179,24 @@ insert_debug_decl_map (copy_body_data *id, tree key, tree value) context. */ static int processing_debug_stmt = 0; +/* Create a DEBUG_EXPR_DECL based on NAME, and return it. Create a + debug_source_bind assigning VAL to the DEBUG_EXPR_DECL, and return it in + STMT. */ + +static tree +create_debug_expr (tree name, tree val, gimple **stmt) +{ + tree vexpr = make_node (DEBUG_EXPR_DECL); + + *stmt = gimple_build_debug_source_bind (vexpr, val, NULL); + + DECL_ARTIFICIAL (vexpr) = 1; + TREE_TYPE (vexpr) = TREE_TYPE (name); + SET_DECL_MODE (vexpr, DECL_MODE (SSA_NAME_VAR (name))); + + return vexpr; +} + /* Construct new SSA name for old NAME. ID is the inline context. */ static tree @@ -191,7 +209,19 @@ remap_ssa_name (tree name, copy_body_data *id) n = id->decl_map->get (name); if (n) - return unshare_expr (*n); + { + if (TREE_CODE (*n) == DEBUG_EXPR_DECL && !processing_debug_stmt) + /* This is triggered for f.i. pr48063.c, which contains undefined + behaviour: a mismatch between argument and parameter type. + Consequently, when cloning, the uses of the parameter are not + replaced by the mismatched argument, and we end up with non-debug + uses of the parameter. Don't use the debug expr in such a case, and + let the use be mapped using the local variable that was introduced + as substitution of the parameter. */ + ; + else + return unshare_expr (*n); + } if (processing_debug_stmt) { @@ -200,7 +230,6 @@ remap_ssa_name (tree name, copy_body_data *id) && id->entry_bb == NULL && single_succ_p (ENTRY_BLOCK_PTR_FOR_FN (cfun))) { - tree vexpr = make_node (DEBUG_EXPR_DECL); gimple *def_temp; gimple_stmt_iterator gsi; tree val = SSA_NAME_VAR (name); @@ -213,10 +242,8 @@ remap_ssa_name (tree name, copy_body_data *id) processing_debug_stmt = -1; return name; } - def_temp = gimple_build_debug_source_bind (vexpr, val, NULL); - DECL_ARTIFICIAL (vexpr) = 1; - TREE_TYPE (vexpr) = TREE_TYPE (name); - SET_DECL_MODE (vexpr, DECL_MODE (SSA_NAME_VAR (name))); + + tree vexpr = create_debug_expr (name, val, &def_temp); gsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun))); gsi_insert_before (&gsi, def_temp, GSI_SAME_STMT); return vexpr; @@ -5544,7 +5571,8 @@ copy_decl_maybe_to_var (tree decl, copy_body_data *id) /* Return a copy of the function's argument tree. */ static tree copy_arguments_for_versioning (tree orig_parm, copy_body_data * id, - bitmap args_to_skip, tree *vars) + bitmap args_to_skip, tree *vars, tree old_decl, + gimple_seq *stmts) { tree arg, *parg; tree new_parm = NULL; @@ -5572,6 +5600,20 @@ copy_arguments_for_versioning (tree orig_parm, copy_body_data * id, /* Declare this new variable. */ DECL_CHAIN (var) = *vars; *vars = var; + + if (!MAY_HAVE_DEBUG_BIND_STMTS) + continue; + + tree def = ssa_default_def (DECL_STRUCT_FUNCTION (old_decl), arg); + if (!def) + continue; + + gimple *def_stmt; + tree vexpr = create_debug_expr (def, arg, &def_stmt); + /* TODO: Reuse vexpr in debug_args_to_skip loop in + tree_function_versioning. */ + insert_decl_map (id, def, vexpr); + gimple_seq_add_stmt (stmts, def_stmt); } return new_parm; } @@ -5905,10 +5947,12 @@ tree_function_versioning (tree old_decl, tree new_decl, } } /* Copy the function's arguments. */ + gimple_seq arg_stmts = NULL; if (DECL_ARGUMENTS (old_decl) != NULL_TREE) DECL_ARGUMENTS (new_decl) = copy_arguments_for_versioning (DECL_ARGUMENTS (old_decl), &id, - args_to_skip, &vars); + args_to_skip, &vars, old_decl, + &arg_stmts); DECL_INITIAL (new_decl) = remap_blocks (DECL_INITIAL (id.src_fn), &id); BLOCK_SUPERCONTEXT (DECL_INITIAL (new_decl)) = new_decl; @@ -5969,6 +6013,12 @@ tree_function_versioning (tree old_decl, tree new_decl, insert_init_stmt (&id, bb, init_stmts.pop ()); update_clone_info (&id); + if (arg_stmts) + { + gimple_stmt_iterator gsi = gsi_last_bb (bb); + gsi_insert_seq_after (&gsi, arg_stmts, GSI_CONTINUE_LINKING); + } + /* Remap the nonlocal_goto_save_area, if any. */ if (cfun->nonlocal_goto_save_area) {
On Fri, Jul 6, 2018 at 12:47 PM Tom de Vries <tdevries@suse.de> wrote: > > On 07/05/2018 01:39 PM, Richard Biener wrote: > > 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: > > <SNIP> > > >> [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? > > I've updated the patch to add a debug expr here in > copy_arguments_for_versioning for every parameter that has a default > def, and to use that debug expr in remap_ssa_name. > > > All remaining uses should be in debug stmts (I hope). > > I ran into a test-case where that was not the case, so I had to handle > that in remap_ssa_name, the comment in the patch describes that in more > detail. I see. I now also spotted the code in remap_ssa_name that is supposed to handle this it seems and for the testcase we only give up because the PARM_DECL is remapped to a VAR_DECL. So I suppose it is to be handled via the debug-args stuff which probably lacks in the area of versioning. Your patch feels like it adds stuff ontop of existing mechanisms that should "just work" with the correct setup at the correct places... Jakub, can you shed any light on how this is supposed to work? Looking at rev. 175288 it's not entirely clear to me? Richard. > Bootstrapped and reg-tested on x86_64. > > OK for trunk? > > Thanks, > - Tom
On 07/06/2018 12:28 PM, Richard Biener wrote: > On Thu, Jul 5, 2018 at 4:12 PM Tom de Vries <tdevries@suse.de> wrote: >> >> On 07/05/2018 01:39 PM, Richard Biener wrote: >>> 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> >>> >>> ? >> >> The type for the vla gcc is emitting is an DW_TAG_array_type with >> DW_TAG_subrange_type without DW_AT_upper_bound or DW_AT_count, which >> makes the upper bound value 'unknown'. So I'd say the debug info is valid. > > OK, that sounds reasonable. I wonder if languages like Ada have a way > to declare an array type with unknown upper bound but known lower bound. > For > > typedef int arr[]; > arr *x; > > we generate just > > <1><2d>: Abbrev Number: 2 (DW_TAG_typedef) > <2e> DW_AT_name : arr > <32> DW_AT_decl_file : 1 > <33> DW_AT_decl_line : 1 > <34> DW_AT_decl_column : 13 > <35> DW_AT_type : <0x39> > <1><39>: Abbrev Number: 3 (DW_TAG_array_type) > <3a> DW_AT_type : <0x44> > <3e> DW_AT_sibling : <0x44> > <2><42>: Abbrev Number: 4 (DW_TAG_subrange_type) > <2><43>: Abbrev Number: 0 > > which does > > (gdb) ptype arr > type = int [] > (gdb) ptype x > type = int (*)[] > (gdb) p sizeof (arr) > $1 = 0 > > so I wonder whether the patch makes it print <optimized out> > instead? I think both 0 and <optimized out> are less than ideal > and maybe <variable size> would be better. In the type case > above it's certainly not "optimized out". > I ran into trouble with the earlier posted gdb patch, in gdb/testsuite/gdb.fortran/vla-sizeof.exp. I've submitted a second try to gdb-patches ("[PATCH][exp] Interpret size of vla with unknown size as <optimized out>" at https://sourceware.org/ml/gdb-patches/2018-07/msg00541.html ). It handles the zero-sized array in the C example above correctly. Thanks, - Tom
On Wed, Jul 18, 2018 at 3:42 PM Tom de Vries <tdevries@suse.de> wrote: > > On 07/06/2018 12:28 PM, Richard Biener wrote: > > On Thu, Jul 5, 2018 at 4:12 PM Tom de Vries <tdevries@suse.de> wrote: > >> > >> On 07/05/2018 01:39 PM, Richard Biener wrote: > >>> 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> > >>> > >>> ? > >> > >> The type for the vla gcc is emitting is an DW_TAG_array_type with > >> DW_TAG_subrange_type without DW_AT_upper_bound or DW_AT_count, which > >> makes the upper bound value 'unknown'. So I'd say the debug info is valid. > > > > OK, that sounds reasonable. I wonder if languages like Ada have a way > > to declare an array type with unknown upper bound but known lower bound. > > For > > > > typedef int arr[]; > > arr *x; > > > > we generate just > > > > <1><2d>: Abbrev Number: 2 (DW_TAG_typedef) > > <2e> DW_AT_name : arr > > <32> DW_AT_decl_file : 1 > > <33> DW_AT_decl_line : 1 > > <34> DW_AT_decl_column : 13 > > <35> DW_AT_type : <0x39> > > <1><39>: Abbrev Number: 3 (DW_TAG_array_type) > > <3a> DW_AT_type : <0x44> > > <3e> DW_AT_sibling : <0x44> > > <2><42>: Abbrev Number: 4 (DW_TAG_subrange_type) > > <2><43>: Abbrev Number: 0 > > > > which does > > > > (gdb) ptype arr > > type = int [] > > (gdb) ptype x > > type = int (*)[] > > (gdb) p sizeof (arr) > > $1 = 0 > > > > so I wonder whether the patch makes it print <optimized out> > > instead? I think both 0 and <optimized out> are less than ideal > > and maybe <variable size> would be better. In the type case > > above it's certainly not "optimized out". > > > > I ran into trouble with the earlier posted gdb patch, in > gdb/testsuite/gdb.fortran/vla-sizeof.exp. > > I've submitted a second try to gdb-patches ("[PATCH][exp] Interpret size > of vla with unknown size as <optimized out>" at > https://sourceware.org/ml/gdb-patches/2018-07/msg00541.html ). It > handles the zero-sized array in the C example above correctly. Thanks. Without -gstrict-dwarf we can use DW_OP_GNU_variable_value and if that doesn't have a location it should be <optimized out>. I wonder if it makes sense to disambiguate things from the gcc side by generating an empty location description for known optimized out values (the standard seems to explicitely call that out as a way to say "optimized out"). Because int a[] is different from int a[x] where x is optimized out. Richard. > Thanks, > - Tom
On 07/19/2018 10:30 AM, Richard Biener wrote: > On Wed, Jul 18, 2018 at 3:42 PM Tom de Vries <tdevries@suse.de> wrote: >> >> On 07/06/2018 12:28 PM, Richard Biener wrote: >>> On Thu, Jul 5, 2018 at 4:12 PM Tom de Vries <tdevries@suse.de> wrote: >>>> >>>> On 07/05/2018 01:39 PM, Richard Biener wrote: >>>>> 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> >>>>> >>>>> ? >>>> >>>> The type for the vla gcc is emitting is an DW_TAG_array_type with >>>> DW_TAG_subrange_type without DW_AT_upper_bound or DW_AT_count, which >>>> makes the upper bound value 'unknown'. So I'd say the debug info is valid. >>> >>> OK, that sounds reasonable. I wonder if languages like Ada have a way >>> to declare an array type with unknown upper bound but known lower bound. >>> For >>> >>> typedef int arr[]; >>> arr *x; >>> >>> we generate just >>> >>> <1><2d>: Abbrev Number: 2 (DW_TAG_typedef) >>> <2e> DW_AT_name : arr >>> <32> DW_AT_decl_file : 1 >>> <33> DW_AT_decl_line : 1 >>> <34> DW_AT_decl_column : 13 >>> <35> DW_AT_type : <0x39> >>> <1><39>: Abbrev Number: 3 (DW_TAG_array_type) >>> <3a> DW_AT_type : <0x44> >>> <3e> DW_AT_sibling : <0x44> >>> <2><42>: Abbrev Number: 4 (DW_TAG_subrange_type) >>> <2><43>: Abbrev Number: 0 >>> >>> which does >>> >>> (gdb) ptype arr >>> type = int [] >>> (gdb) ptype x >>> type = int (*)[] >>> (gdb) p sizeof (arr) >>> $1 = 0 >>> >>> so I wonder whether the patch makes it print <optimized out> >>> instead? I think both 0 and <optimized out> are less than ideal >>> and maybe <variable size> would be better. In the type case >>> above it's certainly not "optimized out". >>> >> >> I ran into trouble with the earlier posted gdb patch, in >> gdb/testsuite/gdb.fortran/vla-sizeof.exp. >> >> I've submitted a second try to gdb-patches ("[PATCH][exp] Interpret size >> of vla with unknown size as <optimized out>" at >> https://sourceware.org/ml/gdb-patches/2018-07/msg00541.html ). It >> handles the zero-sized array in the C example above correctly. > > Thanks. Without -gstrict-dwarf we can use DW_OP_GNU_variable_value > and if that doesn't have a location it should be <optimized out>. > > I wonder if it makes sense to disambiguate things from the gcc side > by generating an empty location description for known optimized out > values (the standard seems to explicitely call that out as a way to > say "optimized out"). Agreed, that makes sense, and thanks for pointing that out. [ Found it at dwarf5 doc, 2.6.1.1.1 Empty Location Descriptions. ] If I manage to find or construct a test-case where this is applicable on trunk, I'll take a look. Thanks, - Tom > Because int a[] is different from int a[x] where > x is optimized out. >
On Tue, Jul 24, 2018 at 12:29:37PM +0200, Tom de Vries wrote: > > I wonder if it makes sense to disambiguate things from the gcc side > > by generating an empty location description for known optimized out > > values (the standard seems to explicitely call that out as a way to > > say "optimized out"). You can't emit the empty location descriptions in the middle of other expressions though, and if you have e.g. an expression that uses DW_OP_GNU_variable_size only conditionally, even if you know that the corresponding DIE doesn't have location, the containing DWARF expression could still be well defined if the condition guarding the DW_OP_GNU_variable_size isn't true. And, also, for DW_OP_GNU_variable_size DIE lookup should be performed, say you have only a declaration in the TU, but definition somewhere else, in that case the debugger should be using that other TU's definition with hopefully location description. Or if there is a global variable interposed by some other TU, again, DW_OP_GNU_variable_size should follow to whatever variable wins. Jakub
On Tue, Jul 24, 2018 at 12:39 PM Jakub Jelinek <jakub@redhat.com> wrote: > > On Tue, Jul 24, 2018 at 12:29:37PM +0200, Tom de Vries wrote: > > > I wonder if it makes sense to disambiguate things from the gcc side > > > by generating an empty location description for known optimized out > > > values (the standard seems to explicitely call that out as a way to > > > say "optimized out"). > > You can't emit the empty location descriptions in the middle of other > expressions though, True. We'd have to "fold" any expression dependend on the optimized out value to optimized out itself. > and if you have e.g. an expression that uses > DW_OP_GNU_variable_size only conditionally, even if you know that > the corresponding DIE doesn't have location, the containing DWARF expression > could still be well defined if the condition guarding the > DW_OP_GNU_variable_size isn't true. DW_OP_GNU_variable_value you mean. That's true. But I was talking about omitting DW_AT_upper_bound which would correspond to int[] vs. a empty location list DW_AT_upper_bound which would correspond to int[<something>]. ISTR we quite aggressively prune DW_OP_GNU_variable_values that we cannot resolve. > And, also, for DW_OP_GNU_variable_size DIE lookup should be performed, > say you have only a declaration in the TU, but definition somewhere else, > in that case the debugger should be using that other TU's definition > with hopefully location description. Or if there is a global variable > interposed by some other TU, again, DW_OP_GNU_variable_size should follow > to whatever variable wins. Not sure if that is how it behaves (well, gdb still doesn't support DW_OP_GNU_variable_value). Related is probably that I wanted to have debug info of VLAs in the abstract instance so we'd have a DW_OP_GNU_variable_value of a DIE in the abstract instance (with no location) and the concrete instance DIE of the refered to DIE would only be related to it via its DW_AT_abstract_origin attribute. Ideally that would just work but given lacking gdb support I couldn't figure that out and thus we re-emit the whole type in the concrete instances... If we sort that out we can drop all of the VLA type remapping during inlining / cloning I think given we have debug info for the abstract instance around. Richard. > > Jakub
On Tue, Jul 24, 2018 at 04:33:30PM +0200, Richard Biener wrote: > DW_OP_GNU_variable_value you mean. That's true. But I was talking about Sure. > omitting DW_AT_upper_bound which would correspond to int[] vs. > a empty location list DW_AT_upper_bound which would correspond to > int[<something>]. I think that is fine to differentiate. > Not sure if that is how it behaves (well, gdb still doesn't support > DW_OP_GNU_variable_value). Related is probably that I wanted to have > debug info of VLAs in the abstract instance so we'd have a > DW_OP_GNU_variable_value > of a DIE in the abstract instance (with no location) and the concrete instance > DIE of the refered to DIE would only be related to it via its > DW_AT_abstract_origin > attribute. Ideally that would just work but given lacking gdb support > I couldn't > figure that out and thus we re-emit the whole type in the concrete instances... The intent was that the debugger would do similar thing as if the user at that point asked for the value of that (integral) variable, whatever it would print would be just pushed to the DWARF stack. Not sure how far are the GDB folks with the DW_OP_GNU_variable_value support. Jakub
On Tue, Jul 24, 2018 at 4:41 PM Jakub Jelinek <jakub@redhat.com> wrote: > > On Tue, Jul 24, 2018 at 04:33:30PM +0200, Richard Biener wrote: > > DW_OP_GNU_variable_value you mean. That's true. But I was talking about > > Sure. > > > omitting DW_AT_upper_bound which would correspond to int[] vs. > > a empty location list DW_AT_upper_bound which would correspond to > > int[<something>]. > > I think that is fine to differentiate. > > > Not sure if that is how it behaves (well, gdb still doesn't support > > DW_OP_GNU_variable_value). Related is probably that I wanted to have > > debug info of VLAs in the abstract instance so we'd have a > > DW_OP_GNU_variable_value > > of a DIE in the abstract instance (with no location) and the concrete instance > > DIE of the refered to DIE would only be related to it via its > > DW_AT_abstract_origin > > attribute. Ideally that would just work but given lacking gdb support > > I couldn't > > figure that out and thus we re-emit the whole type in the concrete instances... > > The intent was that the debugger would do similar thing as if the user at that > point asked for the value of that (integral) variable, whatever it would > print would be just pushed to the DWARF stack. OK, that is definitely exactly what I was hoping for. I guess I should contribute a example like those in the DWARF specs with source and abstract instance vs. inline instance for example with intended behavior so you can amend your proposal. > Not sure how far are the GDB folks with the DW_OP_GNU_variable_value > support. No idea - maybe Tom can help out here as well. Richard. > Jakub
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) { 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; }