[debug] Fix handling of vlas in lto

Message ID 20180817210744.GA15027@delia
State New
Headers show
Series
  • [debug] Fix handling of vlas in lto
Related show

Commit Message

Tom de Vries Aug. 17, 2018, 9:07 p.m.
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.

2018-08-17  Tom de Vries  <tdevries@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(-)

Comments

Richard Biener Aug. 20, 2018, 1:09 p.m. | #1
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.  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  <tdevries@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 <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
Tom de Vries Aug. 21, 2018, 2:10 p.m. | #2
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  <tdevries@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 Aug. 22, 2018, 12:12 p.m. | #3
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  <tdevries@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 <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
Eric Botcazou June 28, 2019, 10:21 p.m. | #4
> 2018-08-17  Tom de Vries  <tdevries@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.


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 Botcazou
package 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;

Patch

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