dynamic array's upper bound evaluated as address for AVR target

dynamic array's upper bound evaluated as address for AVR target

[Sivanupandi, Pitchumani]

Tests from gdb.base/vla-datatypes.exp are failed for AVR target.
Test vla-datatypes.c validates size and values of dynamic arrays.

When evaluating the array expression (e.g. print int_vla), debug info for
upper bound of array read as location expression. By default, the location
expressions are treated as DWARF_VALUE_MEMORY (dwarf2expr.c:execute_stack_op).
and converted to target address (dwarf2loc.c:dwarf2_locexpr_baton_eval).

This will lead to incorrect upper bound value for targets like AVR where 
integer_to_address hook defined. For array 'int_vla', upper bound (e.g. 4)
is wrongly converted to avr target sram address (0x80004). And gdb tries to
read 0x100008 (0x80004 * 2) bytes of values from array's start address.
This is incorrect and caused timeout for avr target.

Why is that location expression treated as DWARF_VALUE_MEMORY by default?
Can gdb detect if location expression for literal value (e.g. array bounds)
or not?

Regards,
Pitchumani

Re: dynamic array's upper bound evaluated as address for AVR target

[Pierre-Marie de Rodat]

Pitchumani,

(chiming in because I’ve worked a bit on DWARF expressions in GDB, but 
disclaimer: I’m not a maintainer ;-))

On 10/13/2015 11:07 AM, Sivanupandi, Pitchumani wrote:
> Why is that location expression treated as DWARF_VALUE_MEMORY by default?
> Can gdb detect if location expression for literal value (e.g. array bounds)
> or not?

struct dwarf_expr_context::location is relevant only when evaluating 
DWARF expressions which are location descriptions. These descriptions 
generally compute addresses for objects but this “location” field makes 
it possible to describe objects that are not in memory (for instance in 
registers or several pieces in different places).

Anyway in the case you expose, the DWARF expressions compute bounds 
(i.e. scalar values) and thus this field is not relevant[1].

I was not aware of this hook (integer_to_address) and I could not find 
relevant documentation to learn about what it means, in what specific 
cases it should be used/avoided and why it exists in the first place 
(not being familiar with AVR neither, I don't understand what the 
corresponding code in avr-tdep.c does), so the following is speculation.

dwarf2_locexpr_baton_eval evaluates a DWARF expression assuming it 
computes the address of an object[1] and thus ends up invoking this 
hook. So my understanding is that either we should not call 
dwarf2_locexpr_baton_eval when evaluating things that are not locations 
(such as subrange bounds), either we should adapt 
dwarf2_locexpr_baton_eval to accept a new parameter telling wether the 
caller wants an address or a scalar.

Thoughts?

[1] This makes me feel like the handling of everything that is not 
DWARF_VALUE MEMORY in this function is dead (and incorrect) code: we 
should not get anything else in this context. I checked what happens 
when evaluating expressions in a GDB session: 
dwarf2_evaluate_loc_desc_full is called to get the location of objects, 
not dwarf2_locexpr_baton_eval. I tried to remove stack/register/literal 
handling and had no testsuite regression.

-- 
Pierre-Marie de Rodat

RE: dynamic array's upper bound evaluated as address for AVR target

[Sivanupandi, Pitchumani]

> -----Original Message-----
> From: Pierre-Marie de Rodat [mailto:derodat@adacore.com]
> Sent: 13 October 2015 20:14
> To: Sivanupandi, Pitchumani; gdb@sourceware.org
> Cc: Andrew Burgess; tom@tromey.com; uweigand@de.ibm.com
> Subject: Re: dynamic array's upper bound evaluated as address for AVR target
> 
> Pitchumani,
> 
> (chiming in because I’ve worked a bit on DWARF expressions in GDB, but
> disclaimer: I’m not a maintainer ;-))
> 
> On 10/13/2015 11:07 AM, Sivanupandi, Pitchumani wrote:
> > Why is that location expression treated as DWARF_VALUE_MEMORY by default?
> > Can gdb detect if location expression for literal value (e.g. array
> > bounds) or not?
> 
> struct dwarf_expr_context::location is relevant only when evaluating DWARF
> expressions which are location descriptions. These descriptions generally
> compute addresses for objects but this “location” field makes it possible to
> describe objects that are not in memory (for instance in registers or
> several pieces in different places).
> 
> Anyway in the case you expose, the DWARF expressions compute bounds (i.e.
> scalar values) and thus this field is not relevant[1].
> 
> I was not aware of this hook (integer_to_address) and I could not find
> relevant documentation to learn about what it means, in what specific cases
> it should be used/avoided and why it exists in the first place (not being
> familiar with AVR neither, I don't understand what the corresponding code in
> avr-tdep.c does), so the following is speculation.

Target hook for integer_to_address does some manipulation on the given value.
In AVR target, it adds SRAM memory mask to the value to make that SRAM address.

> dwarf2_locexpr_baton_eval evaluates a DWARF expression assuming it computes
> the address of an object[1] and thus ends up invoking this hook. So my
> understanding is that either we should not call dwarf2_locexpr_baton_eval
> when evaluating things that are not locations (such as subrange bounds),
> either we should adapt dwarf2_locexpr_baton_eval to accept a new parameter
> telling wether the caller wants an address or a scalar.
> 
> Thoughts?
> 
> [1] This makes me feel like the handling of everything that is not
> DWARF_VALUE MEMORY in this function is dead (and incorrect) code: we should
> not get anything else in this context. I checked what happens when
> evaluating expressions in a GDB session:
> dwarf2_evaluate_loc_desc_full is called to get the location of objects, not
> dwarf2_locexpr_baton_eval. I tried to remove stack/register/literal handling
> and had no testsuite regression.

Function dwarf2_locexpr_baton_eval is called by dwarf2_evaluate_property for
location expression (from resolve_dynamic_range. i.e. to resolve bounds of 
dynamic array e.g. int int_vla[n] where n is function parameter).

Regards,
Pitchumani

Re: dynamic array's upper bound evaluated as address for AVR target

[Pierre-Marie de Rodat]

On 10/14/2015 08:32 AM, Sivanupandi, Pitchumani wrote:
> Target hook for integer_to_address does some manipulation on the given value.
> In AVR target, it adds SRAM memory mask to the value to make that SRAM address.

This is what I observed, but I wonder: why do we need this in the first 
place? In what situation do we have an integer on which we need to apply 
a mask to get an address (i.e. from where does this integer come?).

> Function dwarf2_locexpr_baton_eval is called by dwarf2_evaluate_property for
> location expression (from resolve_dynamic_range. i.e. to resolve bounds of
> dynamic array e.g. int int_vla[n] where n is function parameter).

Then maybe we should add this expect_address parameter to both 
dwarf2_evaluate_property and dwarf2_locexpr_baton_eval…

-- 
Pierre-Marie de Rodat

RE: dynamic array's upper bound evaluated as address for AVR target

[Sivanupandi, Pitchumani]

> -----Original Message-----
> From: Pierre-Marie de Rodat [mailto:derodat@adacore.com]
> Sent: 14 October 2015 12:24
> To: Sivanupandi, Pitchumani; gdb@sourceware.org
> Cc: Andrew Burgess; tom@tromey.com; uweigand@de.ibm.com
> Subject: Re: dynamic array's upper bound evaluated as address for AVR target
> 
> On 10/14/2015 08:32 AM, Sivanupandi, Pitchumani wrote:
> > Target hook for integer_to_address does some manipulation on the given
> value.
> > In AVR target, it adds SRAM memory mask to the value to make that SRAM
> address.
> 
> This is what I observed, but I wonder: why do we need this in the first
> place? In what situation do we have an integer on which we need to apply a
> mask to get an address (i.e. from where does this integer come?).

It is not meant for integer to address conversion, rather transforming the
location expression value as target address (value.c:value_as_address) if
target has some special handling. In AVR case, to differentiate the memory
type (flash, sram, eeprom) we have this mask so that debugger can identify
the correct memory type from that address.

> > Function dwarf2_locexpr_baton_eval is called by
> > dwarf2_evaluate_property for location expression (from
> > resolve_dynamic_range. i.e. to resolve bounds of dynamic array e.g. int
> int_vla[n] where n is function parameter).
> 
> Then maybe we should add this expect_address parameter to both
> dwarf2_evaluate_property and dwarf2_locexpr_baton_eval…

Ok. I'll try it out.

Thanks.

Regards,
Pitchumani

Re: dynamic array's upper bound evaluated as address for AVR target

[Pierre-Marie de Rodat]

On 10/14/2015 09:54 AM, Sivanupandi, Pitchumani wrote:
> It is not meant for integer to address conversion, rather transforming the
> location expression value as target address (value.c:value_as_address) if
> target has some special handling. In AVR case, to differentiate the memory
> type (flash, sram, eeprom) we have this mask so that debugger can identify
> the correct memory type from that address.

Thank you for your answers, and sorry for the new questions, I just try 
to understand both the need and the solution in place for it. ;-)

If all these memories share the same address space, why don’t location 
expressions (so compiler generated) compute directly masked addresses? 
What addresses does the inferior processes? (like: in registers, etc.) 
are these masked as well?

>> Then maybe we should add this expect_address parameter to both
>> dwarf2_evaluate_property and dwarf2_locexpr_baton_eval…
>
> Ok. I'll try it out.

Thank you! As I said previously, I’m not a maintainer: I added Joel as a 
recipient since he’s both a maintainer and the original author of the 
dwarf2 property mechanism.

-- 
Pierre-Marie de Rodat

Re: dynamic array's upper bound evaluated as address for AVR target

[Ulrich Weigand]

Pierre-Marie de Rodat wrote:
> On 10/14/2015 09:54 AM, Sivanupandi, Pitchumani wrote:
> >> Then maybe we should add this expect_address parameter to both
> >> dwarf2_evaluate_property and dwarf2_locexpr_baton_eval…
> >
> > Ok. I'll try it out.
> 
> Thank you! As I said previously, I’m not a maintainer: I added Joel as a 
> recipient since he’s both a maintainer and the original author of the 
> dwarf2 property mechanism.

I've just had a quick look at the "dwarf2 property" mechanism, and it seems
to me that our current implementation mixes up two different things.  We
currently use attr_to_dynamic_prop for three property types:

DW_AT_lower_bound
DW_AT_upper_bound
DW_AT_count
DW_AT_static_link
DW_AT_data_location

However, the DWARF standard describes the first three in a quite different
manner than the latter two.

For DW_AT_static_link, we have:

  If a subroutine or entry point is nested, it may have a DW_AT_static_link
  attribute, whose value is a location description that computes the frame
  base of the relevant instance of the subroutine that immediately encloses
  the subroutine or entry point.

For DW_AT_data_location, we have:

  The DW_AT_data_location attribute may be used with any type that provides
  one or more levels of hidden indirection and/or run-time parameters in its
  representation. Its value is a location description. The result of evaluating
  this description yields the location of the data for an object.

So those two are *location descriptions* that evaluate to an address.  They
should be treated just like DW_AT_frame_base is today, i.e. using the
dwarf2_loclist_baton/dwarf2_locexpr_baton mechanism.

On the other hand, for the first three attrbutes, the standard says:

  The subrange entry may have the attributes DW_AT_lower_bound and
  DW_AT_upper_bound to specify, respectively, the lower and upper bound
  values of the subrange. The DW_AT_upper_bound attribute may be replaced
  by a DW_AT_count attribute, whose value describes the number of elements
  in the subrange rather than the value of the last element. The value of
  each of these attributes is determined as described in Section 2.19.

Where section 2.19 reads:

  Some attributes that apply to types specify a property (such as the lower
  bound of an array) that is an integer value, where the value may be known
  during compilation or may be computed dynamically during execution.
  The value of these attributes is determined based on the class as follows:
  • For a constant, the value of the constant is the value of the attribute.
  • For a reference, the value is a reference to another entity which
    specifies the value of the attribute.
  • For an exprloc, the value is interpreted as a DWARF expression;
    evaluation of the expression yields the value of the attribute.

where the value may be a DWARF *expression*, but not a location description,
and it results in an integer, not an address.

Note that a plain DWARF expression can actually from a valid location
description (namely a simple memory location expression), but there are
many more options how a location description can look.

I think the current GDB "dynamic property" mechanism was probably intended
to capture what DWARF section 2.19 specifies, but due to its simultaneous
use for DW_AT_data_location and DW_AT_static_link, things got confused.

It would probably be best to clearly separate this out into two separate
mechanisms, one that precisely implements dynamic properties as defined
in section 2.19, and one that implements location descriptions.  (Note
that those mechanism should ideally then be used for some of the *other*
places where the DWARF standard also allows either a dynamic property or
a location description, but GDB doesn't accept them yet.)

This would then have the side effect that we could distinguish return
types: location descriptions always return an address, but section 2.19
dynamic attributes always return an integer.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

RE: dynamic array's upper bound evaluated as address for AVR target

[Sivanupandi, Pitchumani]

> -----Original Message-----
> From: Pierre-Marie de Rodat [mailto:derodat@adacore.com]
> Sent: 14 October 2015 13:56
> To: Sivanupandi, Pitchumani; Joel Brobecker
> Cc: gdb@sourceware.org; Andrew Burgess; tom@tromey.com; uweigand@de.ibm.com
> Subject: Re: dynamic array's upper bound evaluated as address for AVR target
> 
> On 10/14/2015 09:54 AM, Sivanupandi, Pitchumani wrote:
> > It is not meant for integer to address conversion, rather transforming
> > the location expression value as target address
> > (value.c:value_as_address) if target has some special handling. In AVR
> > case, to differentiate the memory type (flash, sram, eeprom) we have
> > this mask so that debugger can identify the correct memory type from that
> address.
> 
> Thank you for your answers, and sorry for the new questions, I just try to
> understand both the need and the solution in place for it. ;-)
> 
> If all these memories share the same address space, why don’t location
> expressions (so compiler generated) compute directly masked addresses?
> What addresses does the inferior processes? (like: in registers, etc.) are
> these masked as well?

AVR has different address spaces (Harvard architecture). Compiler
(avr-gcc) generates fictitious addresses to support it.
Also refer:
AVR Background in avr-tdep.c
https://sourceware.org/ml/gdb/2014-10/msg00142.html

Regards,
Pitchumani

Re: dynamic array's upper bound evaluated as address for AVR target

[Joel Brobecker]

> I think the current GDB "dynamic property" mechanism was probably intended
> to capture what DWARF section 2.19 specifies, but due to its simultaneous
> use for DW_AT_data_location and DW_AT_static_link, things got confused.
> 
> It would probably be best to clearly separate this out into two separate
> mechanisms, one that precisely implements dynamic properties as defined
> in section 2.19, and one that implements location descriptions.  (Note
> that those mechanism should ideally then be used for some of the *other*
> places where the DWARF standard also allows either a dynamic property or
> a location description, but GDB doesn't accept them yet.)
> 
> This would then have the side effect that we could distinguish return
> types: location descriptions always return an address, but section 2.19
> dynamic attributes always return an integer.

Sounds correct to me.

-- 
Joel

Re: dynamic array's upper bound evaluated as address for AVR target

[Pierre-Marie de Rodat]

On 10/14/2015 02:26 PM, Joel Brobecker wrote:
> Sounds correct to me.

And to me as well: thank you Ulrich for this crystal clear explanation. 
:-) I will give it a try next week, then.

-- 
Pierre-Marie de Rodat

Re: dynamic array's upper bound evaluated as address for AVR target

[Pierre-Marie de Rodat]

On 10/14/2015 12:15 PM, Sivanupandi, Pitchumani wrote:
> AVR has different address spaces (Harvard architecture). Compiler
> (avr-gcc) generates fictitious addresses to support it.
> Also refer:
> AVR Background in avr-tdep.c
> https://sourceware.org/ml/gdb/2014-10/msg00142.html

That all makes sense, now, thank you. :-)

-- 
Pierre-Marie de Rodat

Re: dynamic array's upper bound evaluated as address for AVR target

[Pierre-Marie de Rodat]

On 10/14/2015 09:37 AM, Pierre-Marie de Rodat wrote:
> And to me as well: thank you Ulrich for this crystal clear explanation.
> :-) I will give it a try next week, then.

Quick update: I started to dig into this. I still have a hard time 
wrapping the current implementation in my mind[1] and thinking about 
what we should do exactly:

   * Have different types for expressions and location lists (hence 
potentially renaming existing code): looks like a big commit!

   * Introduce another entry point to evaluate dynamic properties as 
expressions. Looks like a small change but a weaker design: how to 
evaluate depends a property depends on the property, not the evaluation 
context, so it would be great to keep types distinct.

I cannot make any commitment on a schedule to implement this, though.


[1] Especially considering that dynamic properties handle only locations 
that resolve to an inferior address. Location resolution for variables 
don’t rely on this mechanism, which is some kind of inconsistency. On 
the other hand, dynamic properties are DWARF-specific…

-- 
Pierre-Marie de Rodat

Re: dynamic array's upper bound evaluated as address for AVR target

[Ulrich Weigand]

Pierre-Marie de Rodat wrote:

> Quick update: I started to dig into this. I still have a hard time 
> wrapping the current implementation in my mind[1] and thinking about 
> what we should do exactly:
> 
>    * Have different types for expressions and location lists (hence 
> potentially renaming existing code): looks like a big commit!
> 
>    * Introduce another entry point to evaluate dynamic properties as 
> expressions. Looks like a small change but a weaker design: how to 
> evaluate depends a property depends on the property, not the evaluation 
> context, so it would be great to keep types distinct.
> 
> I cannot make any commitment on a schedule to implement this, though.

It seems to me the easiest path to implement the change incrementally
might be:

a.) We currently have:

enum dynamic_prop_kind
{
  PROP_UNDEFINED, /* Not defined.  */
  PROP_CONST,     /* Constant.  */
  PROP_ADDR_OFFSET, /* Address offset.  */
  PROP_LOCEXPR,   /* Location expression.  */
  PROP_LOCLIST    /* Location list.  */
};

As a first step, add a value
  PROP_EXPR     /* DWARF expression.  */
to this list.

b.) Add code to dwarf2_evaluate_property / dwarf2_compile_property_to_c
to support PROP_EXPR.

This is a bit tricky since there is no real infrastructure to evaluate
DWARF expressions, as opposed to location expressions.  Note that the
existing routine dwarf_entry_parameter_to_value uses the trick to append
DW_OP_stack_value to the expression to force evaluation as a DWARF
expression instead of location.  The same could probably be done here.

c.) For the range-related dynamic properties, create PROP_EXPR property
nodes instead of PROP_LOC* property nodes.  This probably means splitting
the routine attr_to_dynamic_prop into two variants, one for locations
and one for dynamic properties.

-- At this point, everything should be functionally correct, the rest
   is simply cleanup ---

d.) Create some new common mechanism to handle DWARF locations

e.) Change the various places where DWARF locations are handled to use
this new common mechanism:
 - DW_AT_location      [ currently uses SYMBOL_COMPUTED_OPS ]
 - DW_AT_frame_base    [ currently uses SYMBOL_BLOCK_OPS ]
 - DW_AT_data_location [ currently uses dynamic_prop ]
 - DW_AT_static_link   [ currently uses dynamic_prop ]

f.) Finally, remove (now unused) support for PROP_LOCEXPR/PROP_LOCLIST
from the dynamic property code.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com