[Ada] Fix handling of array renamings

  Compilers can materialize renamings of arrays (or of accesses to arrays)
in Ada into variables whose types are references to the actual array
types.  Before this change, trying to use such an array renaming yielded
an error in GDB:

    (gdb) print my_array(1)
    cannot subscript or call a record
    (gdb) print my_array_ptr(1)
    cannot subscript or call something of type `(null)'

This behavior comes from bad handling for array renamings, in particular
the OP_FUNCALL expression operator handling from ada-lang.c
(ada_evaluate_subexp): in one place we turn the reference into a
pointer, but the code that follows expect the value to be an array.

This patch fixes how we handle references in call/subscript evaluation
so that we turn these references into the actual array values instead of
pointers to them.

gdb/ChangeLog:

	* ada-lang.c (ada_evaluate_subexp) <OP_FUNCALL>: When the input
	value is a reference, actually dereference it in order to get
	the underlying value.

gdb/testsuite/ChangeLog:

	* gdb.ada/array_ptr_renaming.exp: New testcase.
	* gdb.ada/array_ptr_renaming/foo.adb: New file.
	* gdb.ada/array_ptr_renaming/pack.ads: New file.

Tested on x86_64-linux, no regression.
---
 gdb/ada-lang.c                                    | 15 ++++++---
 gdb/testsuite/gdb.ada/array_ptr_renaming.exp      | 39 +++++++++++++++++++++++
 gdb/testsuite/gdb.ada/array_ptr_renaming/foo.adb  | 25 +++++++++++++++
 gdb/testsuite/gdb.ada/array_ptr_renaming/pack.ads | 25 +++++++++++++++
 4 files changed, 100 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.ada/array_ptr_renaming.exp
 create mode 100644 gdb/testsuite/gdb.ada/array_ptr_renaming/foo.adb
 create mode 100644 gdb/testsuite/gdb.ada/array_ptr_renaming/pack.ads
  

On Wed, Sep 02, 2015 at 09:21:13AM +0200, Pierre-Marie de Rodat wrote:
> Compilers can materialize renamings of arrays (or of accesses to arrays)
> in Ada into variables whose types are references to the actual array
> types.  Before this change, trying to use such an array renaming yielded
> an error in GDB:
> 
>     (gdb) print my_array(1)
>     cannot subscript or call a record
>     (gdb) print my_array_ptr(1)
>     cannot subscript or call something of type `(null)'
> 
> This behavior comes from bad handling for array renamings, in particular
> the OP_FUNCALL expression operator handling from ada-lang.c
> (ada_evaluate_subexp): in one place we turn the reference into a
> pointer, but the code that follows expect the value to be an array.
> 
> This patch fixes how we handle references in call/subscript evaluation
> so that we turn these references into the actual array values instead of
> pointers to them.
> 
> gdb/ChangeLog:
> 
> 	* ada-lang.c (ada_evaluate_subexp) <OP_FUNCALL>: When the input
> 	value is a reference, actually dereference it in order to get
> 	the underlying value.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.ada/array_ptr_renaming.exp: New testcase.
> 	* gdb.ada/array_ptr_renaming/foo.adb: New file.
> 	* gdb.ada/array_ptr_renaming/pack.ads: New file.

I had a quick look, and I am (litterally) not sure about
the patch.

Generally speaking, we try very hard to avoid dereferencing objects,
especially arrays or structs, as this opens the door for accidently
fetching the entire object in situations where only one element was
needed. Since we are dealing with array subscripting, this is
necessarily the case here. Perhaps the object's lifetime is constrained
and thus currently would never trigger that excessive fetch. But I need
to look at the rest of the code in that function more carefully to
assess it, and I am too short on time now to do it.

I'll review again when I get back...
  

On 09/03/2015 01:59 AM, Joel Brobecker wrote:
> Generally speaking, we try very hard to avoid dereferencing objects,
> especially arrays or structs, as this opens the door for accidently
> fetching the entire object in situations where only one element was
> needed. Since we are dealing with array subscripting, this is
> necessarily the case here.

I guess you are referring to the call to coerce_ref my patch introduces. 
My understanding was that this does not actually fetch the data but 
instead creates a lazy value (see the call to value_at_lazy in 
value.c:coerce_ref) from the reference "pointer", hence not fetching any 
data beyond maybe the reference "pointer" itself.

> Perhaps the object's lifetime is constrained and thus currently would
> never trigger that excessive fetch.

Hm... I don't understand: about what object are you talking? The struct 
value * in GDB? Why does its lifetime influences whether it is fetched?

> But I need to look at the rest of the code in that function more
> carefully to assess it, and I am too short on time now to do it.
>
> I'll review again when I get back...

Sure, there's no hurry. :-) Thank you for reviewing, Joel!
  

> gdb/ChangeLog:
> 
> 	* ada-lang.c (ada_evaluate_subexp) <OP_FUNCALL>: When the input
> 	value is a reference, actually dereference it in order to get
> 	the underlying value.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.ada/array_ptr_renaming.exp: New testcase.
> 	* gdb.ada/array_ptr_renaming/foo.adb: New file.
> 	* gdb.ada/array_ptr_renaming/pack.ads: New file.

OK with one tiny little suggestion.

> +gdb_test "print ntp"     " = \\(access pack\\.table_type\\) 0x.*"
                                                               ^^^

Suggest using $hex.* instead of 0x.*

Thanks! And sorry for the late review...
  

On 09/22/2015 05:00 PM, Joel Brobecker wrote:
> OK with one tiny little suggestion.
>
>> +gdb_test "print ntp"     " = \\(access pack\\.table_type\\) 0x.*"
>                                                                 ^^^
>
> Suggest using $hex.* instead of 0x.*
>
> Thanks! And sorry for the late review...

Done and pushed. No problem: I thank you for reviewing. :-)