[PATCH] [Ada] Enhance type printing for arrays with variable-sized elements

[PATCH] [Ada] Enhance type printing for arrays with variable-sized elements

[Pierre-Marie de Rodat]

This change is relevant only for standard DWARF (as opposed to the GNAT
encodings extensions): at the time of writing it only makes a difference
with GCC patches that are to be integrated: see the patch series
submission at
<https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01353.html>.

Given the following Ada declarations:

   subtype Small_Int is Natural range 0 .. 100;
   type R_Type (L : Small_Int := 0) is record
      S : String (1 .. L);
   end record;
   type A_Type is array (Natural range <>) of R_Type;

   A : A_Type := (1 => (L => 0, S => ""),
                  2 => (L => 2, S => "ab"));

Before this change, we would get the following GDB session:

    (gdb) ptype a
    type = array (1 .. 2) of foo.r_type <packed: 838-bit elements>

This is wrong: "a" is not a packed array.  This output comes from the
fact that, because R_Type has a dynamic size (with a maximum), the
compiler has to describe in the debugging information the size allocated
for each array element (i.e. the stride, in DWARF parlance: see
DW_AT_byte_stride).  Ada type printing currently assumes that arrays
with a stride are packed, hence the above output.

In practice, GNAT never performs bit-packing for arrays that contain
variable-sized elements.  Leveraging this fact, this patch enhances type
printing so that ptype does not pretend that arrays are packed when they
have a stride and they contain dynamic elements.  After this change, we
get the following expected output:

    (gdb) ptype a
    type = array (1 .. 2) of foo.r_type

gdb/ChangeLog:

	* ada-typeprint.c (print_array_type): Do not describe arrays as
	packed when they embed dynamic elements.

gdb/testsuite/ChangeLog:

	* gdb.ada/array_of_variable_length.exp: New testcase.
	* gdb.ada/array_of_variable_length/foo.adb: New file.
	* gdb.ada/array_of_variable_length/pck.adb: New file.
	* gdb.ada/array_of_variable_length/pck.ads: New file.

Tested on x86_64-linux, no regression.
---
 gdb/ada-typeprint.c                                | 11 ++++++--
 gdb/testsuite/gdb.ada/array_of_variable_length.exp | 33 ++++++++++++++++++++++
 .../gdb.ada/array_of_variable_length/foo.adb       | 21 ++++++++++++++
 .../gdb.ada/array_of_variable_length/pck.adb       | 23 +++++++++++++++
 .../gdb.ada/array_of_variable_length/pck.ads       | 32 +++++++++++++++++++++
 5 files changed, 117 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.ada/array_of_variable_length.exp
 create mode 100644 gdb/testsuite/gdb.ada/array_of_variable_length/foo.adb
 create mode 100644 gdb/testsuite/gdb.ada/array_of_variable_length/pck.adb
 create mode 100644 gdb/testsuite/gdb.ada/array_of_variable_length/pck.ads

diff --git a/gdb/ada-typeprint.c b/gdb/ada-typeprint.c
index fd85138..1fb0f0e 100644
--- a/gdb/ada-typeprint.c
+++ b/gdb/ada-typeprint.c
@@ -386,6 +386,7 @@ print_array_type (struct type *type, struct ui_file *stream, int show,
 {
   int bitsize;
   int n_indices;
+  struct type *elt_type = NULL;
 
   if (ada_is_constrained_packed_array_type (type))
     type = ada_coerce_to_simple_array_type (type);
@@ -448,11 +449,15 @@ print_array_type (struct type *type, struct ui_file *stream, int show,
 	fprintf_filtered (stream, "%s<>", i == i0 ? "" : ", ");
     }
 
+  elt_type = ada_array_element_type (type, n_indices);
   fprintf_filtered (stream, ") of ");
   wrap_here ("");
-  ada_print_type (ada_array_element_type (type, n_indices), "", stream,
-		  show == 0 ? 0 : show - 1, level + 1, flags);
-  if (bitsize > 0)
+  ada_print_type (elt_type, "", stream, show == 0 ? 0 : show - 1, level + 1,
+		  flags);
+  /* Arrays with variable-length elements are never bit-packed in practice but
+     compilers have to describe their stride so that we can properly fetch
+     individual elements.  Do not say the array is packed in this case.  */
+  if (bitsize > 0 && !is_dynamic_type (elt_type))
     fprintf_filtered (stream, " <packed: %d-bit elements>", bitsize);
 }
 
diff --git a/gdb/testsuite/gdb.ada/array_of_variable_length.exp b/gdb/testsuite/gdb.ada/array_of_variable_length.exp
new file mode 100644
index 0000000..625d0a8
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/array_of_variable_length.exp
@@ -0,0 +1,33 @@
+# 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"
+
+# Pck.A is an array that embeds elements with variable size so compilers will
+# emit DWARF attributes such as DW_AT_byte_stride to tell GDB how to fetch
+# individual elements.  Array stride is also a way to describe packed arrays:
+# make sure we do not consider Pck.A as a packed array.
+gdb_test "ptype pck.a" "array \\(1 \\.\\. 2\\) of pck\\.r_type"
diff --git a/gdb/testsuite/gdb.ada/array_of_variable_length/foo.adb b/gdb/testsuite/gdb.ada/array_of_variable_length/foo.adb
new file mode 100644
index 0000000..b8846cf
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/array_of_variable_length/foo.adb
@@ -0,0 +1,21 @@
+--  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 Pck; use Pck;
+
+procedure Foo is
+begin
+   Do_Nothing (A); --  BREAK
+end Foo;
diff --git a/gdb/testsuite/gdb.ada/array_of_variable_length/pck.adb b/gdb/testsuite/gdb.ada/array_of_variable_length/pck.adb
new file mode 100644
index 0000000..a6ffddc
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/array_of_variable_length/pck.adb
@@ -0,0 +1,23 @@
+--  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 body Pck is
+
+   procedure Do_Nothing (A : A_Type) is
+   begin
+      null;
+   end Do_Nothing;
+
+end Pck;
diff --git a/gdb/testsuite/gdb.ada/array_of_variable_length/pck.ads b/gdb/testsuite/gdb.ada/array_of_variable_length/pck.ads
new file mode 100644
index 0000000..ce2a21e
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/array_of_variable_length/pck.ads
@@ -0,0 +1,32 @@
+--  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 Pck is
+
+   subtype Small_Int is Natural range 0 .. 100;
+
+   type R_Type (L : Small_Int := 0) is record
+      S : String (1 .. L);
+   end record;
+
+   type A_Type is array (Natural range <>) of R_Type;
+
+   A : A_Type :=
+     (1 => (L => 0, S => ""),
+      2 => (L => 2, S => "ab"));
+
+   procedure Do_Nothing (A : A_Type);
+
+end Pck;
-- 
2.5.1

Re: [PATCH] [Ada] Enhance type printing for arrays with variable-sized elements

[Joel Brobecker]

> Before this change, we would get the following GDB session:
> 
>     (gdb) ptype a
>     type = array (1 .. 2) of foo.r_type <packed: 838-bit elements>
> 
> This is wrong: "a" is not a packed array.  This output comes from the
> fact that, because R_Type has a dynamic size (with a maximum), the
> compiler has to describe in the debugging information the size allocated
> for each array element (i.e. the stride, in DWARF parlance: see
> DW_AT_byte_stride).  Ada type printing currently assumes that arrays
> with a stride are packed, hence the above output.
> 
> In practice, GNAT never performs bit-packing for arrays that contain
> variable-sized elements.  Leveraging this fact, this patch enhances type
> printing so that ptype does not pretend that arrays are packed when they
> have a stride and they contain dynamic elements.  After this change, we
> get the following expected output:
> 
>     (gdb) ptype a
>     type = array (1 .. 2) of foo.r_type
> 
> gdb/ChangeLog:
> 
> 	* ada-typeprint.c (print_array_type): Do not describe arrays as
> 	packed when they embed dynamic elements.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.ada/array_of_variable_length.exp: New testcase.
> 	* gdb.ada/array_of_variable_length/foo.adb: New file.
> 	* gdb.ada/array_of_variable_length/pck.adb: New file.
> 	* gdb.ada/array_of_variable_length/pck.ads: New file.

This looks like an easy improvement. Thanks, Pierre-Marie!

I'm curious - what happens if you do:

    (gdb) print a
    (gdb) ptype $

The concern is that $ refers to the resolved version of "A", and
that therefore we might lose the dynamic property. But I think it will
work in this case, because we do not resolve the array's element type
(each element of the actual array has to be resolved individually,
since the actual type changes from element to element).

It's worth extending your new testcase with the above scenario as well,
I think.

Other than that, the patch itself looks pretty good to me.

-- 
Joel

Re: [PATCH] [Ada] Enhance type printing for arrays with variable-sized elements

[Pierre-Marie de Rodat]

On 09/15/2015 06:06 PM, Joel Brobecker wrote:
> This looks like an easy improvement. Thanks, Pierre-Marie!

Yes it is. ;-)

> I'm curious - what happens if you do:
>
>      (gdb) print a
>      (gdb) ptype $
>
> The concern is that $ refers to the resolved version of "A", and
> that therefore we might lose the dynamic property. But I think it will
> work in this case, because we do not resolve the array's element type
> (each element of the actual array has to be resolved individually,
> since the actual type changes from element to element).
>
> It's worth extending your new testcase with the above scenario as well,
> I think.

Huh, very interesting, thanks for raising this! Your intuition is 
correct: this already works for this reason. I've extended the testcase 
to check exactly this.

> Other than that, the patch itself looks pretty good to me.

The updated patch is pushed. Thank you for reviewing!

-- 
Pierre-Marie de Rodat