Re: [PATCH][debug] Fix handling of vlas in lto

[Eric Botcazou]

> 2018-08-17  Tom de Vries  
> 
>   * 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.

This breaks Ada though, i.e. any array type whose bound depends on a 
discriminant is affected:

   type Array_Type is array (Integer range <>) of Integer;
   type Record_Type (N : Integer) is record
  A : Array_Type (1 .. N);
   end record;

.uleb128 0x6# (DIE (0x66) DW_TAG_array_type)
.long   .LASF5  # DW_AT_name: "p__record_type__T4s"
.long   0x34# DW_AT_type
.long   0x79# DW_AT_sibling
.uleb128 0x7# (DIE (0x73) DW_TAG_subrange_type)
.long   0x2d# DW_AT_type
.byte   0   # end of children of DIE 0x66

Testcase attached, compile with -fgnat-encodings=minimal.

-- 
Eric Botcazoupackage P is

   type Array_Type is array (Integer range <>) of Integer;
   type Record_Type (N : Integer := 16) is record
  A : Array_Type (1 .. N);
   end record;

   R : Record_Type;

end P;

Re: [PATCH][debug] Fix handling of vlas in lto

[Richard Biener]

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:
> >> ...
> >>   DIE0: DW_TAG_subrange_type (0x7f85029a90f0)
> >> DW_AT_upper_bound: location descriptor:
> >>   (0x7f85029a9230) DW_OP_GNU_variable_value die -> 0 (0x7f85029a94b0), > >> 0
> >>   DIE0: 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:
> >> ...
> >>   DIE0: DW_TAG_subrange_type (0x7f5183b57d70)
> >> DW_AT_upper_bound: die -> 0 (0x7f5183b576e0)
> >>   DIE0: 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 "".
> >>
> >> 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:
> ...
> 7return 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 look

Re: [PATCH][debug] Fix handling of vlas in lto

[Tom de Vries]

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:
>> ...
>>   DIE0: DW_TAG_subrange_type (0x7f85029a90f0)
>> DW_AT_upper_bound: location descriptor:
>>   (0x7f85029a9230) DW_OP_GNU_variable_value die -> 0 (0x7f85029a94b0), 0
>>   DIE0: 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:
>> ...
>>   DIE0: DW_TAG_subrange_type (0x7f5183b57d70)
>> DW_AT_upper_bound: die -> 0 (0x7f5183b576e0)
>>   DIE0: 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 "".
>>
>> 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:
...
7return 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  
>>
>>  * 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.
>>  Ot

Re: [PATCH][debug] Fix handling of vlas in lto

[Richard Biener]

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:
> ...
>   DIE0: DW_TAG_subrange_type (0x7f85029a90f0)
> DW_AT_upper_bound: location descriptor:
>   (0x7f85029a9230) DW_OP_GNU_variable_value die -> 0 (0x7f85029a94b0), 0
>   DIE0: 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:
> ...
>   DIE0: DW_TAG_subrange_type (0x7f5183b57d70)
> DW_AT_upper_bound: die -> 0 (0x7f5183b576e0)
>   DIE0: 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 "".
> 
> 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.  Note that with a gdb with DW_OP_variable_value 
support we should be able to elide the VLA type in the concrete
instance...

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  
> 
>   * 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)
> 

[PATCH][debug] Fix handling of vlas in lto

[Tom de Vries]

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:
...
  DIE0: DW_TAG_subrange_type (0x7f85029a90f0)
DW_AT_upper_bound: location descriptor:
  (0x7f85029a9230) DW_OP_GNU_variable_value die -> 0 (0x7f85029a94b0), 0
  DIE0: 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:
...
  DIE0: DW_TAG_subrange_type (0x7f5183b57d70)
DW_AT_upper_bound: die -> 0 (0x7f5183b576e0)
  DIE0: 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 "".

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.

2018-08-17  Tom de Vries  

* 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_CONS