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?
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);
>> }
>>
>>
>
