On Tue, 21 Aug 2018, Tom de Vries wrote:
> On 08/20/2018 03:09 PM, Richard Biener wrote:
> > On Fri, 17 Aug 2018, Tom de Vries wrote:
> >
> >> I've rewritten the patch to work generically, not just for
> >> DW_AT_upper_bound,
> >> and to reuse the code already there in add_scalar_info.
> >>
> >> OK for trunk?
> >>
> >> Thanks,
> >> - Tom
> >>
> >> [debug] Fix handling of vlas in lto
> >>
> >> Atm, when running vla-1.c with -O0 -flto, we have:
> >> ...
> >> FAIL: gcc.dg/guality/vla-1.c -O0 -flto -fuse-linker-plugin \
> >> -fno-fat-lto-objects line 17 sizeof (a) == 6
> >> ...
> >>
> >> The vla a[i + 1] in f1 is gimplified into:
> >> ...
> >> f1 (int i)
> >> {
> >> char a[0:D.1922] [value-expr: *a.0];
> >> char[0:D.1922] * a.0;
> >>
> >> D.1921 = i + 1;
> >> D.1926 = (sizetype) D.1921;
> >> a.0 = __builtin_alloca_with_align (D.1926, 8);
> >> ...
> >>
> >> The early debug info for the upper bound of the type of vla a that we
> >> stream
> >> out is:
> >> ...
> >> DIE 0: DW_TAG_subrange_type (0x7f85029a90f0)
> >> DW_AT_upper_bound: location descriptor:
> >> (0x7f85029a9230) DW_OP_GNU_variable_value die -> 0 (0x7f85029a94b0), > >> 0
> >> DIE 0: DW_TAG_variable (0x7f85029a94b0)
> >> DW_AT_name: "D.1922"
> >> DW_AT_type: die -> 0 (0x7f85029a3d70)
> >> DW_AT_artificial: 1
> >> ...
> >>
> >> and in ltrans we have for that same upper bound:
> >> ...
> >> DIE 0: DW_TAG_subrange_type (0x7f5183b57d70)
> >> DW_AT_upper_bound: die -> 0 (0x7f5183b576e0)
> >> DIE 0: DW_TAG_variable (0x7f5183b576e0)
> >> DW_AT_name: "D.4278"
> >> DW_AT_abstract_origin: die -> label: vla_1.c.6719312a + 193
> >> (0x7f5183b57730)
> >> ...
> >> where D.4278 has abstract origin D.1922.
> >>
> >> The D.4278 die has no DW_AT_location, so when evaluting "sizeof (a)" in the
> >> debugger, we can't find the information to get the value of D.4278, and the
> >> debugger prints "<optimized out>".
> >>
> >> This patch fixes that by either:
> >> - adding DW_AT_location to the referenced variable die, or
> >> - instead of using a ref for the upper bound, using an exprloc.
> >>
> >> When changing gcc.dg/guality/guality.exp to run the usual flto flavours
> >> "-fno-use-linker-plugin -flto-partition=none" and "-fuse-linker-plugin
> >> -fno-fat-lto-objects" in combination with O0, Og, O1, O2, O3 and Os, this
> >> patch
> >> fixes all (20) failures in vla-1.c, leaving only:
> >> ...
> >> No symbol "i" in current context.
> >> UNSUPPORTED: gcc.dg/guality/vla-1.c -O3 -flto -fno-use-linker-plugin \
> >> -flto-partition=none line 17 i == 5
> >> 'a' has unknown type; cast it to its declared type
> >> UNSUPPORTED: gcc.dg/guality/vla-1.c -O3 -flto -fno-use-linker-plugin \
> >> -flto-partition=none line 17 sizeof (a) == 6
> >> ...
> >>
> >> Bootstrapped and reg-tested on x86_64.
> >
> > This looks OK to me.
>
> Committed.
>
> > Note that with a gdb with DW_OP_variable_value
> > support we should be able to elide the VLA type in the concrete
> > instance...
> >
>
> Using this patch we elide the VLA type in the concrete instance for -O2
> -flto:
> ...
> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> index dd8f438dfd3..5776185d9c6 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -23712,12 +23712,14 @@ gen_variable_die (tree decl, tree origin,
> dw_die_ref context_die)
> && variably_modified_type_p
> (type, decl_function_context (decl_or_origin)))
> {
> +#if 0
> if (decl_by_reference_p (decl_or_origin))
> add_type_attribute (var_die, TREE_TYPE (type),
> TYPE_UNQUALIFIED, false,
> context_die);
> else
> add_type_attribute (var_die, type,
> decl_quals (decl_or_origin),
> false, context_die);
> +#endif
> }
>
> goto gen_variable_die_location;
> ...
> and using master gdb (which supports DW_OP_variable_value, yay) we get:
> ...
> 7 return a[0]; /* { dg-final { gdb-test . "sizeof (a)"
> "6" } } */
> vla-1.gdb:3: Error in sourced command file:
> value has been optimized out
> ...
>
> When evaluating sizeof (a) in gdb:
> - we look for the concrete type die of a
> - since that one is elided, we fall back on the type die of the abstract
> origin of a
> - that one has an expr location for the upper bound containing a
> DW_OP_variable_value referring to a variable in early debug.
> - that variable has no DW_AT_location, so we end up with "value has been
> optimized out"
>
> AFAIU, that's roughly the issue discussed in
> PR20426 - "gdb does not interpret DWARF annotating imported units fully"
> ( https://sourceware.org/bugzilla/show_bug.cgi?id=20426 ),.
>
> Is my understanding correct that it's not yet clear how this should be
> fixed?
ISTR Jakub recently said in a bug or in a mail the debug consumer
should lookup DW_OP_variable_value like it would be specified in
a user expression and thus it should find the concrete instance
which does have a location (and refers to the DIE refered to by
DW_OP_variable_value via its DW_AT_abstract_origin).
Otherwise DW_OP_variable_value is useless for abstract instances
like those created for inlining and we need - as we currently do! - to
re-emit type DIEs in each inline instance for annotating them
with locations. I suppose for the DWARF proposal an actual
example using VLAs and inlining and abstract and inline instances
is the best motivating example.
Richard.
> Thanks,
> - Tom
>
> > Not sure how we should go forward there - use a configure test or
> > simply tell people to update gdb?
> >
> > Thanks,
> > Richard.
> >
> >> 2018-08-17 Tom de Vries <tdevr...@suse.de>
> >>
> >> * dwarf2out.c (add_scalar_info): Don't add reference to existing die
> >> unless the referenced die describes the added property using
> >> DW_AT_location or DW_AT_const_value. Fall back to exprloc case.
> >> Otherwise, add a DW_AT_location to the referenced die.
> >>
> >> ---
> >> gcc/dwarf2out.c | 32 ++++++++++++++++++++------------
> >> 1 file changed, 20 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> >> index 9ed473088e7..e1dccb42823 100644
> >> --- a/gcc/dwarf2out.c
> >> +++ b/gcc/dwarf2out.c
> >> @@ -20598,7 +20598,7 @@ static void
> >> add_scalar_info (dw_die_ref die, enum dwarf_attribute attr, tree value,
> >> int forms, struct loc_descr_context *context)
> >> {
> >> - dw_die_ref context_die, decl_die;
> >> + dw_die_ref context_die, decl_die = NULL;
> >> dw_loc_list_ref list;
> >> bool strip_conversions = true;
> >> bool placeholder_seen = false;
> >> @@ -20675,7 +20675,7 @@ add_scalar_info (dw_die_ref die, enum
> >> dwarf_attribute attr, tree value,
> >>
> >> if (decl != NULL_TREE)
> >> {
> >> - dw_die_ref decl_die = lookup_decl_die (decl);
> >> + decl_die = lookup_decl_die (decl);
> >>
> >> /* ??? Can this happen, or should the variable have been bound
> >> first? Probably it can, since I imagine that we try to create
> >> @@ -20684,8 +20684,12 @@ add_scalar_info (dw_die_ref die, enum
> >> dwarf_attribute attr, tree value,
> >> later parameter. */
> >> if (decl_die != NULL)
> >> {
> >> - add_AT_die_ref (die, attr, decl_die);
> >> - return;
> >> + if (get_AT (decl_die, DW_AT_location)
> >> + || get_AT (decl_die, DW_AT_const_value))
> >> + {
> >> + add_AT_die_ref (die, attr, decl_die);
> >> + return;
> >> + }
> >> }
> >> }
> >> }
> >> @@ -20729,15 +20733,19 @@ add_scalar_info (dw_die_ref die, enum
> >> dwarf_attribute attr, tree value,
> >> || placeholder_seen)
> >> return;
> >>
> >> - if (current_function_decl == 0)
> >> - context_die = comp_unit_die ();
> >> - else
> >> - context_die = lookup_decl_die (current_function_decl);
> >> + if (!decl_die)
> >> + {
> >> + if (current_function_decl == 0)
> >> + context_die = comp_unit_die ();
> >> + else
> >> + context_die = lookup_decl_die (current_function_decl);
> >> +
> >> + decl_die = new_die (DW_TAG_variable, context_die, value);
> >> + add_AT_flag (decl_die, DW_AT_artificial, 1);
> >> + add_type_attribute (decl_die, TREE_TYPE (value), TYPE_QUAL_CONST,
> >> false,
> >> + context_die);
> >> + }
> >>
> >> - decl_die = new_die (DW_TAG_variable, context_die, value);
> >> - add_AT_flag (decl_die, DW_AT_artificial, 1);
> >> - add_type_attribute (decl_die, TREE_TYPE (value), TYPE_QUAL_CONST, false,
> >> - context_die);
> >> add_AT_location_description (decl_die, DW_AT_location, list);
> >> add_AT_die_ref (die, attr, decl_die);
> >> }
> >>
> >>
> >
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
