[PATCH] [Ada] Fix handling of array renamings

[PATCH] [Ada] Fix handling of array renamings

[Pierre-Marie de Rodat]

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

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index a7809ff..3e45316 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -10629,10 +10629,17 @@ ada_evaluate_subexp (struct type *expect_type, struct expression *exp,
 	   therefore already coerced to a simple array.  Nothing further
 	   to do.  */
         ;
-      else if (TYPE_CODE (value_type (argvec[0])) == TYPE_CODE_REF
-               || (TYPE_CODE (value_type (argvec[0])) == TYPE_CODE_ARRAY
-                   && VALUE_LVAL (argvec[0]) == lval_memory))
-        argvec[0] = value_addr (argvec[0]);
+      else if (TYPE_CODE (value_type (argvec[0])) == TYPE_CODE_REF)
+	{
+	  /* Make sure we dereference references so that all the code below
+	     feels like it's really handling the referenced value.  Wrapping
+	     types (for alignment) may be there, so make sure we strip them as
+	     well.  */
+	  argvec[0] = ada_to_fixed_value (coerce_ref (argvec[0]));
+	}
+      else if (TYPE_CODE (value_type (argvec[0])) == TYPE_CODE_ARRAY
+	       && VALUE_LVAL (argvec[0]) == lval_memory)
+	argvec[0] = value_addr (argvec[0]);
 
       type = ada_check_typedef (value_type (argvec[0]));
 
diff --git a/gdb/testsuite/gdb.ada/array_ptr_renaming.exp b/gdb/testsuite/gdb.ada/array_ptr_renaming.exp
new file mode 100644
index 0000000..a33202e
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/array_ptr_renaming.exp
@@ -0,0 +1,39 @@
+# Copyright 2015 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib "ada.exp"
+
+standard_ada_testfile foo
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug]] != "" } {
+  return -1
+}
+
+clean_restart ${testfile}
+
+set bp_location [gdb_get_line_number "BREAK" ${testdir}/foo.adb]
+runto "foo.adb:$bp_location"
+
+gdb_test "print nt"    " = \\(10, 20\\)"
+gdb_test "print nt(1)" " = 10"
+
+# Accesses to arrays and unconstrained arrays have the same runtime
+# representation with GNAT (fat pointers).  In this case, GDB "forgets" that
+# it's dealing with an access and prints directly the array contents.  This
+# should be fixed some day.
+setup_kfail "gdb/NNNN" *-*-*
+gdb_test "print ntp"     " = \\(access pack\\.table_type\\) 0x.*"
+gdb_test "print ntp.all" " = \\(3 => 30, 40\\)"
+gdb_test "print ntp(3)"  " = 30"
diff --git a/gdb/testsuite/gdb.ada/array_ptr_renaming/foo.adb b/gdb/testsuite/gdb.ada/array_ptr_renaming/foo.adb
new file mode 100644
index 0000000..ead98bc
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/array_ptr_renaming/foo.adb
@@ -0,0 +1,25 @@
+--  Copyright 2015 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+with System;
+with Pack;
+
+procedure Foo is
+   NT  : Pack.Table_Type renames Pack.Table;
+   NTP : Pack.Table_Ptr_Type  renames Pack.Table_Ptr;
+begin
+   NT := NT; --  BREAK
+   NTP := NTP;
+end Foo;
diff --git a/gdb/testsuite/gdb.ada/array_ptr_renaming/pack.ads b/gdb/testsuite/gdb.ada/array_ptr_renaming/pack.ads
new file mode 100644
index 0000000..d88d046
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/array_ptr_renaming/pack.ads
@@ -0,0 +1,25 @@
+--  Copyright 2015 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package Pack is
+
+   type Table_Type is
+     array (Natural range <>) of Integer;
+   type Table_Ptr_Type is access all Table_Type;
+
+   Table     : Table_Type := (1 => 10, 2 => 20);
+   Table_Ptr : aliased Table_Ptr_Type := new Table_Type'(3 => 30, 4 => 40);
+
+end Pack;
-- 
2.5.1

Re: [PATCH] [Ada] Fix handling of array renamings

[Joel Brobecker]

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...

-- 
Joel

Re: [PATCH] [Ada] Fix handling of array renamings

[Pierre-Marie de Rodat]

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!

-- 
Pierre-Marie de Rodat

Re: [PATCH] [Ada] Fix handling of array renamings

[Joel Brobecker]

> 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...
-- 
Joel

Re: [PATCH] [Ada] Fix handling of array renamings

[Pierre-Marie de Rodat]

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. :-)

-- 
Pierre-Marie de Rodat