[RFA/commit 2/6] Add valaddr support in dynamic property resolution.

[RFA/commit 2/6] Add valaddr support in dynamic property resolution.

[Joel Brobecker]

This is the second part of enhancing the debugger to print the value
of arrays of records whose size is variable when only standard DWARF
info is available (no GNAT encoding). For instance:

   subtype Small_Type is Integer range 0 .. 10;
   type Record_Type (I : Small_Type := 0) is record
      S : String (1 .. I);
   end record;
   type Array_Type is array (Integer range <>) of Record_Type;

   A1 : Array_Type := (1 => (I => 0, S => <>),
                       2 => (I => 1, S => "A"),
                       3 => (I => 2, S => "AB"));

Currently, GDB prints the following output:

        (gdb) p a1
        $1 = (

The error happens while the ada-valprint module is trying to print
the value of an element of our array. Because of the fact that
the array's element (type Record_Type) has a variant size, the DWARF
info for our array provide the array's stride:

     <1><749>: Abbrev Number: 10 (DW_TAG_array_type)
        <74a>   DW_AT_name        : (indirect string, offset: 0xb6d): pck__T18s
        <74e>   DW_AT_byte_stride : 16
        <74f>   DW_AT_type        : <0x6ea>

And because our array has a stride, ada-valprint treats it the same
way as packed arrays (see ada-valprint.c::ada_val_print_array):

  if (TYPE_FIELD_BITSIZE (type, 0) > 0)
    val_print_packed_array_elements (type, valaddr, offset_aligned,
                                     0, stream, recurse,
                                     original_value, options);

The first thing that we should notice in the call above is that
the "valaddr" buffer and the associated offset (OFFSET_ALIGNED)
is passed, but that the corresponding array's address is not.
This can be explained by looking inside val_print_packed_array_elements,
where we see that the function unpacks each element of our array from
the buffer alone (ada_value_primitive_packed_val), and then prints
the resulting artificial value instead:

      v0 = ada_value_primitive_packed_val (NULL, valaddr + offset,
                                           (i0 * bitsize) / HOST_CHAR_BIT,
                                           (i0 * bitsize) % HOST_CHAR_BIT,
                                           bitsize, elttype);

      [...]
              val_print (elttype, value_contents_for_printing (v0),
                         value_embedded_offset (v0), 0, stream,
                         recurse + 1, v0, &opts, current_language);

Of particular interest, here, is the fact that we call val_print
with a null address, which is OK, since we're providing a buffer
instead (value_contents_for_printing). Also, providing an address
might not always possible, since packing could place elements at
boundaries that are not byte-aligned.

Things go south when val_print tries to see if there is a pretty-printer
that could be applied. In particular, one of the first things that
the Python pretty-printer does is to create a value using our buffer,
and the given address, which in this case is null (see call to
value_from_contents_and_address in gdbpy_apply_val_pretty_printer).

value_from_contents_and_address, in turn immediately tries to resolve
the type, using the given address, which is null. But, because our
array element is a record containing an array whose bound is the value
of one of its elements (the "s" component), the debugging info for
the array's upper bound is a reference...

 <3><71a>: Abbrev Number: 7 (DW_TAG_subrange_type)
    <71b>   DW_AT_type        : <0x724>
    <71f>   DW_AT_upper_bound : <0x703>

... to component "i" of our record...

 <2><703>: Abbrev Number: 5 (DW_TAG_member)
    <704>   DW_AT_name        : i
    <706>   DW_AT_decl_file   : 2
    <707>   DW_AT_decl_line   : 6
    <708>   DW_AT_type        : <0x6d1>
    <70c>   DW_AT_data_member_location: 0

... where that component is located at offset 0 of the start
of the record. dwarf2_evaluate_property correctly determines
the offset where to load the value of the bound from, but then
tries to read that value from inferior memory using the address
that was given, which is null. See case PROP_ADDR_OFFSET in
dwarf2_evaluate_property:

        val = value_at (baton->offset_info.type,
                        pinfo->addr + baton->offset_info.offset);

This triggers a memory error, which then causes the printing to terminate.

Since there are going to be situations where providing an address
alone is not going to be sufficient (packed arrays where array elements
are not stored at byte boundaries), this patch fixes the issue by
enhancing the type resolution to take both address and data. This
follows the same principle as the val_print module, where both
address and buffer ("valaddr") can be passed as arguments. If the data
has already been fetched from inferior memory (or provided by the
debugging info in some form -- Eg a constant), then use that data
instead of reading it from inferior memory.

Note that this should also be a good step towards being able to handle
dynamic types whose value is stored outside of inferior memory
(Eg: in a register).

With this patch, GDB isn't able to print all of A1, but does perform
a little better:

    (gdb) p a1
    $1 = ((i => 0, s => , (i => 1, s => , (i => 2, s => )

There is another issue which is independent of this one, and will
therefore be patched separately.

gdb/ChangeLog:

        * dwarf2loc.h (struct property_addr_info): Add "valaddr" field.
        * dwarf2loc.c (dwarf2_evaluate_property): Add handling of
        pinfo->valaddr.
        * gdbtypes.h (resolve_dynamic_type): Add "valaddr" parameter.
        * gdbtypes.c (resolve_dynamic_struct): Set pinfo.valaddr.
        (resolve_dynamic_type_internal): Set pinfo.valaddr.
        Add handling of addr_stack->valaddr.
        (resolve_dynamic_type): Add "valaddr" parameter.
        Set pinfo.valaddr field.
        * ada-lang.c (ada_discrete_type_high_bound): Update call to
        resolve_dynamic_type.
        (ada_discrete_type_low_bound): Likewise.
        * findvar.c (default_read_var_value): Likewise.
        * value.c (value_from_contents_and_address): Likewise.
---
 gdb/ada-lang.c  |  4 ++--
 gdb/dwarf2loc.c |  9 +++++++--
 gdb/dwarf2loc.h |  3 +++
 gdb/findvar.c   |  4 ++--
 gdb/gdbtypes.c  | 13 ++++++++++---
 gdb/gdbtypes.h  |  4 +++-
 gdb/value.c     |  2 +-
 7 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 124e370..9926cfb 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -794,7 +794,7 @@ min_of_type (struct type *t)
 LONGEST
 ada_discrete_type_high_bound (struct type *type)
 {
-  type = resolve_dynamic_type (type, 0);
+  type = resolve_dynamic_type (type, NULL, 0);
   switch (TYPE_CODE (type))
     {
     case TYPE_CODE_RANGE:
@@ -815,7 +815,7 @@ ada_discrete_type_high_bound (struct type *type)
 LONGEST
 ada_discrete_type_low_bound (struct type *type)
 {
-  type = resolve_dynamic_type (type, 0);
+  type = resolve_dynamic_type (type, NULL, 0);
   switch (TYPE_CODE (type))
     {
     case TYPE_CODE_RANGE:
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index e674933..d811106 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -2526,8 +2526,13 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 	    break;
 	if (pinfo == NULL)
 	  error (_("cannot find reference address for offset property"));
-	val = value_at (baton->offset_info.type,
-			pinfo->addr + baton->offset_info.offset);
+	if (pinfo->valaddr != NULL)
+	  val = value_from_contents
+		  (baton->offset_info.type,
+		   pinfo->valaddr + baton->offset_info.offset);
+	else
+	  val = value_at (baton->offset_info.type,
+			  pinfo->addr + baton->offset_info.offset);
 	*value = value_as_address (val);
 	return 1;
       }
diff --git a/gdb/dwarf2loc.h b/gdb/dwarf2loc.h
index 0932456..f3630ac 100644
--- a/gdb/dwarf2loc.h
+++ b/gdb/dwarf2loc.h
@@ -111,6 +111,9 @@ struct property_addr_info
      being resolved.  */
   struct type *type;
 
+  /* If not NULL, a buffer containing the object's value.  */
+  const gdb_byte *valaddr;
+
   /* The address of that object.  */
   CORE_ADDR addr;
 
diff --git a/gdb/findvar.c b/gdb/findvar.c
index 128bf5e..2079b4b 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -438,7 +438,7 @@ default_read_var_value (struct symbol *var, struct frame_info *frame)
       if (is_dynamic_type (type))
 	{
 	  /* Value is a constant byte-sequence and needs no memory access.  */
-	  type = resolve_dynamic_type (type, /* Unused address.  */ 0);
+	  type = resolve_dynamic_type (type, NULL, /* Unused address.  */ 0);
 	}
       /* Put the constant back in target format. */
       v = allocate_value (type);
@@ -470,7 +470,7 @@ default_read_var_value (struct symbol *var, struct frame_info *frame)
       if (is_dynamic_type (type))
 	{
 	  /* Value is a constant byte-sequence and needs no memory access.  */
-	  type = resolve_dynamic_type (type, /* Unused address.  */ 0);
+	  type = resolve_dynamic_type (type, NULL, /* Unused address.  */ 0);
 	}
       v = allocate_value (type);
       memcpy (value_contents_raw (v), SYMBOL_VALUE_BYTES (var),
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index fe00ea7..7bcc9e7 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1984,6 +1984,7 @@ resolve_dynamic_struct (struct type *type,
 		 " (invalid location kind)"));
 
       pinfo.type = check_typedef (TYPE_FIELD_TYPE (type, i));
+      pinfo.valaddr = addr_stack->valaddr;
       pinfo.addr = addr_stack->addr;
       pinfo.next = addr_stack;
 
@@ -2050,7 +2051,11 @@ resolve_dynamic_type_internal (struct type *type,
 	    struct property_addr_info pinfo;
 
 	    pinfo.type = check_typedef (TYPE_TARGET_TYPE (type));
-	    pinfo.addr = read_memory_typed_address (addr_stack->addr, type);
+	    pinfo.valaddr = NULL;
+	    if (addr_stack->valaddr != NULL)
+	      pinfo.addr = extract_typed_address (addr_stack->valaddr, type);
+	    else
+	      pinfo.addr = read_memory_typed_address (addr_stack->addr, type);
 	    pinfo.next = addr_stack;
 
 	    resolved_type = copy_type (type);
@@ -2092,9 +2097,11 @@ resolve_dynamic_type_internal (struct type *type,
 /* See gdbtypes.h  */
 
 struct type *
-resolve_dynamic_type (struct type *type, CORE_ADDR addr)
+resolve_dynamic_type (struct type *type, const gdb_byte *valaddr,
+		      CORE_ADDR addr)
 {
-  struct property_addr_info pinfo = {check_typedef (type), addr, NULL};
+  struct property_addr_info pinfo
+    = {check_typedef (type), valaddr, addr, NULL};
 
   return resolve_dynamic_type_internal (type, &pinfo, 1);
 }
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 883418f..abb1a09 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1791,7 +1791,9 @@ extern void get_signed_type_minmax (struct type *, LONGEST *, LONGEST *);
    ADDR specifies the location of the variable the type is bound to.
    If TYPE has no dynamic properties return TYPE; otherwise a new type with
    static properties is returned.  */
-extern struct type *resolve_dynamic_type (struct type *type, CORE_ADDR addr);
+extern struct type *resolve_dynamic_type (struct type *type,
+					  const gdb_byte *valaddr,
+					  CORE_ADDR addr);
 
 /* * Predicate if the type has dynamic values, which are not resolved yet.  */
 extern int is_dynamic_type (struct type *type);
diff --git a/gdb/value.c b/gdb/value.c
index cb56849..6ad2643 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3497,7 +3497,7 @@ value_from_contents_and_address (struct type *type,
 				 const gdb_byte *valaddr,
 				 CORE_ADDR address)
 {
-  struct type *resolved_type = resolve_dynamic_type (type, address);
+  struct type *resolved_type = resolve_dynamic_type (type, valaddr, address);
   struct type *resolved_type_no_typedef = check_typedef (resolved_type);
   struct value *v;
 
-- 
1.9.1

[Ada/DWARF] printing array of variant records

[Joel Brobecker]

Hello,

This patch series aims at enhancing GDB when printing arrays of
records whose contents and size is dynamic, in the case where
we're relying purely on standard DWARF info (no GNAT encodings).

The patch series has been tested on x86_64-linux using the FSF
testsuite, as well as a number of other platforms (Solaris, Windows,
crosses) using AdaCore's testsuite.

Most patches are small fixes and enhancements. Patch #2 (Add valaddr
support in dynamic property resolution) is interesting, thought,
as it is one answer to a question I asked at the very beginning
when we started reviewing patches introducing the concept of dynamic
type resolution: what if the data is not stored in inferior memory?
As it turns out, this patch series encounters one such case, because
we're dealing with data that is not necessarily byte-aligned. And
AdaCore also encountered another situation, with a function returning
a reference to a dynamic object, with that reference being returned
by register; hence no address either. Patch #2 enhances our framework
by using the contents-and-address idea.

If there are no objections, I'd like to commit the patch series
sometime early next week.

Thanks!

[FYI 5/6] GDB crash trying to subscript array of variant record.

[Joel Brobecker]

Consider the following declarations:

   subtype Small_Type is Integer range 0 .. 10;
   type Record_Type (I : Small_Type := 0) is record
      S : String (1 .. I);
   end record;
   A2 : Array_Type := (1 => (I => 2, S => "AB"),
                       2 => (I => 1, S => "A"),
                       3 => (I => 0, S => <>));

Compiled with -fgnat-encodings=minimal, and trying to print
one element of our array, valgrind reports an invalid memory
access. On certain GNU/Linux boxes, malloc even reports it as
well, and causes GDB to crash.

    (gdb) print a2(1)
     *** glibc detected *** /[...]/gdb:
         malloc(): memory corruption: 0x0a30ba48 ***
    [crash]

The invalid memory access occurs because of a simple buffer
overflow in ada_value_primitive_packed_val. When this function
is called, it is given a bit_size of 128 (or 16 bytes), which
corresponds to the stride of our array. But the actual size of
each element depends on its value. In particular, A2(1) is a record
whose size is only 6 bytes.

What happens in our example is that we start building a new value
(v) where the element is to be unpacked, with any of its dynamic
properties getting resolved as well. We then unpack the data into
this value's buffer:

  unpacked = (unsigned char *) value_contents (v);
  [...]
  nsrc = len;
  [...]
  while (nsrc > 0)
    {
      [...]
          unpacked[targ] = accum & ~(~0L << HOST_CHAR_BIT);
          [...]
          targ += delta;
      [...]
      nsrc -= 1;
      [...]
    }

In the loop above, targ starts at zero (for LE architectures),
and len is 16. With delta being +1, we end up iterating 16 times,
writing 16 bytes into a 6-bytes buffer.

This patch fixes the issue by adjusting BIT_SIZE and recomputing
LEN after having resolved our type if the resolved type turns out
to be smaller than bit_size.

gdb/ChangeLog:

        * ada-lang.c (ada_value_primitive_packed_val): Recompute
        BIT_SIZE and LEN if the size of the resolved type is smaller
        than BIT_SIZE * HOST_CHAR_BIT.
---
 gdb/ada-lang.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index d71a243..bbc1732 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2419,6 +2419,17 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
     {
       v = value_at (type, value_address (obj) + offset);
       type = value_type (v);
+      if (TYPE_LENGTH (type) * HOST_CHAR_BIT < bit_size)
+	{
+	  /* This can happen in the case of an array of dynamic objects,
+	     where the size of each element changes from element to element.
+	     In that case, we're initially given the array stride, but
+	     after resolving the element type, we find that its size is
+	     less than this stride.  In that case, adjust bit_size to
+	     match TYPE's length, and recompute LEN accordingly.  */
+	  bit_size = TYPE_LENGTH (type) * HOST_CHAR_BIT;
+	  len = TYPE_LENGTH (type) + (bit_offset + HOST_CHAR_BIT - 1) / 8;
+	}
       bytes = (unsigned char *) alloca (len);
       read_memory (value_address (v), bytes, len);
     }
-- 
1.9.1

[FYI 6/6] testsuite/gdb.ada/var_rec_arr: New testcase.

[Joel Brobecker]

gdb/testsuite/ChangeLog:

        * gdb.ada/var_rec_arr: New testcase.
---
 gdb/testsuite/gdb.ada/var_rec_arr.exp              | 51 ++++++++++++++++++++++
 gdb/testsuite/gdb.ada/var_rec_arr/foo_na09_042.adb | 23 ++++++++++
 gdb/testsuite/gdb.ada/var_rec_arr/pck.adb          | 28 ++++++++++++
 gdb/testsuite/gdb.ada/var_rec_arr/pck.ads          | 38 ++++++++++++++++
 4 files changed, 140 insertions(+)
 create mode 100644 gdb/testsuite/gdb.ada/var_rec_arr.exp
 create mode 100644 gdb/testsuite/gdb.ada/var_rec_arr/foo_na09_042.adb
 create mode 100644 gdb/testsuite/gdb.ada/var_rec_arr/pck.adb
 create mode 100644 gdb/testsuite/gdb.ada/var_rec_arr/pck.ads

diff --git a/gdb/testsuite/gdb.ada/var_rec_arr.exp b/gdb/testsuite/gdb.ada/var_rec_arr.exp
new file mode 100644
index 0000000..82ca857
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/var_rec_arr.exp
@@ -0,0 +1,51 @@
+# 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_na09_042
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug]] != "" } {
+  return -1
+}
+
+clean_restart ${testfile}
+
+set bp_location [gdb_get_line_number "STOP" ${testdir}/foo_na09_042.adb]
+runto "foo_na09_042.adb:$bp_location"
+
+gdb_test "print a1" \
+         " = \\(\\(i => 0, s => \"\"\\), \\(i => 1, s => \"A\"\\), \\(i => 2, s => \"AB\"\\)\\)"
+
+gdb_test "print a1(1)" \
+         " = \\(i => 0, s => \"\"\\)"
+
+gdb_test "print a1(2)" \
+         " = \\(i => 1, s => \"A\"\\)"
+
+gdb_test "print a1(3)" \
+         " = \\(i => 2, s => \"AB\"\\)"
+
+gdb_test "print a2" \
+         " = \\(\\(i => 2, s => \"AB\"\\), \\(i => 1, s => \"A\"\\), \\(i => 0, s => \"\"\\)\\)"
+
+gdb_test "print a2(1)" \
+         " = \\(i => 2, s => \"AB\"\\)"
+
+gdb_test "print a2(2)" \
+         " = \\(i => 1, s => \"A\"\\)"
+
+gdb_test "print a2(3)" \
+         " = \\(i => 0, s => \"\"\\)"
diff --git a/gdb/testsuite/gdb.ada/var_rec_arr/foo_na09_042.adb b/gdb/testsuite/gdb.ada/var_rec_arr/foo_na09_042.adb
new file mode 100644
index 0000000..6737b6e
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/var_rec_arr/foo_na09_042.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/>.
+
+with Pck; use Pck;
+
+procedure Foo_NA09_042 is
+   R1 : Record_Type := Ident (A1 (3));
+begin
+   Do_Nothing (A1'Address);  -- STOP
+   Do_Nothing (R1'Address);
+end Foo_NA09_042;
diff --git a/gdb/testsuite/gdb.ada/var_rec_arr/pck.adb b/gdb/testsuite/gdb.ada/var_rec_arr/pck.adb
new file mode 100644
index 0000000..cb6e114
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/var_rec_arr/pck.adb
@@ -0,0 +1,28 @@
+--  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
+
+   function Ident (R : Record_Type) return Record_Type is
+   begin
+      return R;
+   end Ident;
+
+   procedure Do_Nothing (A : System.Address) is
+   begin
+      null;
+   end Do_Nothing;
+
+end Pck;
diff --git a/gdb/testsuite/gdb.ada/var_rec_arr/pck.ads b/gdb/testsuite/gdb.ada/var_rec_arr/pck.ads
new file mode 100644
index 0000000..902cc9f
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/var_rec_arr/pck.ads
@@ -0,0 +1,38 @@
+--  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;
+
+package Pck is
+
+   subtype Small_Type is Integer range 0 .. 10;
+   type Record_Type (I : Small_Type := 0) is record
+      S : String (1 .. I);
+   end record;
+   function Ident (R : Record_Type) return Record_Type;
+
+   type Array_Type is array (Integer range <>) of Record_Type;
+
+   procedure Do_Nothing (A : System.Address);
+
+   A1 : Array_Type := (1 => (I => 0, S => <>),
+                       2 => (I => 1, S => "A"),
+                       3 => (I => 2, S => "AB"));
+
+   A2 : Array_Type := (1 => (I => 2, S => "AB"),
+                       2 => (I => 1, S => "A"),
+                       3 => (I => 0, S => <>));
+
+end Pck;
-- 
1.9.1

[FYI 4/6] [Ada] array of variant record subscripting

[Joel Brobecker]

Consider the following (Ada) array...

   A1 : Array_Type := (1 => (I => 0, S => <>),
                       2 => (I => 1, S => "A"),
                       3 => (I => 2, S => "AB"));

... where Array_Type is declared as follow:

   subtype Small_Type is Integer range 0 .. 10;
   type Record_Type (I : Small_Type := 0) is record
      S : String (1 .. I);
   end record;
   type Array_Type is array (Integer range <>) of Record_Type;

Trying to print the value of each element individually does not
always work. Printing the value of the first one does:

(gdb) p a1(1)
    $1 = (i => 0, s => "")

But printing the value of the subsequent ones often does not.
For instance:

    (gdb) p a1(2)
    $2 = (i => 1, s => "")  <<<--- s should be "A"
    (gdb) p a1(3)
    $3 = (i => 2, s => "")  <<<--- s should be "AB"

I traced the problem to ada_value_primitive_packed_val,
which is trying to perform the array subscripting by
extracting the value of the corresponding array element
into a buffer where the contents is now byte-aligned.

The element type that ada_value_primitive_packed_val gets passed
is a dynamic type. As it happens, that dynamic type can get resolved
thanks to:

      v = value_at (type, value_address (obj));
      type = value_type (v);

However, obj represents the array, so the address given in the call
to value_at represents the value of the first element. As a result,
the solution of component S's upper bound always gets resolved based
on the value of component I in the  first element of the array, whose
value is 0, thus leading to GDB mistakely resolving the element type
where S's upper bound is always 0.

The proper fix would be to systematically resolve the element type
first. But, this requires us to extract-and-realign the element's
value so as to be able to pass it as "valaddr" to resolve_dynamic_type.
In the meantime, it's easy to make the situation a little better by
passing "value_address (obj) + offset" as the object address. This
only works when BIT_OFFSET is nul, but that should be the case when
the element type is anything but a scalar, which seems to be the only
situation where it seems important to resolve the type now. And we're
not that worse off otherwise.

But we'll try to find a better solution in a separate patch.

gdb/ChangeLog:

        * ada-lang.c (ada_value_primitive_packed_val): Use a more
        correct address in call to value_at.  Adjust call to
        value_address accordingly.
---
 gdb/ada-lang.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 9926cfb..d71a243 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2417,10 +2417,10 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
     }
   else if (VALUE_LVAL (obj) == lval_memory && value_lazy (obj))
     {
-      v = value_at (type, value_address (obj));
+      v = value_at (type, value_address (obj) + offset);
       type = value_type (v);
       bytes = (unsigned char *) alloca (len);
-      read_memory (value_address (v) + offset, bytes, len);
+      read_memory (value_address (v), bytes, len);
     }
   else
     {
-- 
1.9.1

[RFA/commit 1/6] preserve the bit stride when resolving an array type.

[Joel Brobecker]

Consider the following (Ada) variable...

   A1 : Array_Type := (1 => (I => 0, S => <>),
                       2 => (I => 1, S => "A"),
                       3 => (I => 2, S => "AB"));

... where Array_Type is an array of records whose size is variable:

   subtype Small_Type is Integer range 0 .. 10;
   type Record_Type (I : Small_Type := 0) is record
      S : String (1 .. I);
   end record;
   type Array_Type is array (Integer range <>) of Record_Type;

Trying to print the value of this array currently results in the following
error:

    (gdb) p a1
    Cannot access memory at address 0x61c000

What happens in this case, is that the compiler describes our array
as an array with a specific stride (and bounds being static 1..3):

 <1><749>: Abbrev Number: 10 (DW_TAG_array_type)
    <74a>   DW_AT_name        : (indirect string, offset: 0xb6d): pck__T18s
    <74e>   DW_AT_byte_stride : 16
    <74f>   DW_AT_type        : <0x6ea>
 <2><757>: Abbrev Number: 11 (DW_TAG_subrange_type)
    <758>   DW_AT_type        : <0x75e>
    <75c>   DW_AT_upper_bound : 3

This is because we cannot use, in this case, the size of the record
to determine that stride, since the size of the record depends on
its contents. So the compiler helps us by providing that stride.

The problems start when trying to resolve that type. Because the elements
contained in that array type are dynamic, the array itself is considered
dynamic, and thus we end up creating a resolved version of that array.
And during that resolution, we were not handling the case where the array
had a stride. See gdbtypes.c::resolve_dynamic_array...

  return create_array_type (copy_type (type),
                            elt_type,
                            range_type);

As a result, we created an array whose stride was based on the size
of elt_type, which a record whose size isn't static and irrelevant
regardless.

This patch fixes is by calling create_array_type_with_stride instead.

As it happens, there is another issue for us to be able to print
the value of our array, but those are independent of this patch
and will be handled separately. For now, the patch allows us to
get rid of the first error, and the output is now:

     (gdb) p a1
     $1 = (

gdb/ChangeLog:

	* gdbtypes.c (resolve_dynamic_array): Use
	create_array_type_with_stride instead of create_array_type.
---
 gdb/gdbtypes.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 217ec70..fe00ea7 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1898,9 +1898,9 @@ resolve_dynamic_array (struct type *type,
   else
     elt_type = TYPE_TARGET_TYPE (type);
 
-  return create_array_type (copy_type (type),
-			    elt_type,
-			    range_type);
+  return create_array_type_with_stride (copy_type (type),
+					elt_type, range_type,
+					TYPE_FIELD_BITSIZE (type, 0));
 }
 
 /* Resolve dynamic bounds of members of the union TYPE to static
-- 
1.9.1

[FYI 3/6] [Ada] Resolve dynamic type before trying to print it.

[Joel Brobecker]

This is another required step towards trying to print the value of
an array of variant records. For instance:

   A1 : Array_Type := (1 => (I => 0, S => <>),
                       2 => (I => 1, S => "A"),
                       3 => (I => 2, S => "AB"));

... where Array_Type is an array of records whose size is variable:

   subtype Small_Type is Integer range 0 .. 10;
   type Record_Type (I : Small_Type := 0) is record
      S : String (1 .. I);
   end record;
   type Array_Type is array (Integer range <>) of Record_Type;

What happens is that the ada-valprint modules gets passed an array
whose element type is not resolved yet (since each element of the
array needs to be resolved separately). the module then recurses,
and eventually gets called with the first element of the array.
But because the element hasn't been resolved yet, we end up having
trouble printing its value soon after.

This patch fixes the issue by calling resolve_dynamic_type before
trying to print it.

With this patch, GDB is finally able to print the complete value
for variable "A1":

     (gdb) p a1
     $1 = ((i => 0, s => ""), (i => 1, s => "A"), (i => 2, s => "AB"))

gdb/ChangeLog:

        * ada-valprint.c (ada_val_print_1): Resolve TYPE before trying
        to print it.
---
 gdb/ada-valprint.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdb/ada-valprint.c b/gdb/ada-valprint.c
index 34539de..720854e 100644
--- a/gdb/ada-valprint.c
+++ b/gdb/ada-valprint.c
@@ -1095,6 +1095,8 @@ ada_val_print_1 (struct type *type, const gdb_byte *valaddr,
 
   offset_aligned = offset + ada_aligned_value_addr (type, valaddr) - valaddr;
   type = printable_val_type (type, valaddr + offset_aligned);
+  type = resolve_dynamic_type (type, valaddr + offset_aligned,
+			       address + offset_aligned);
 
   switch (TYPE_CODE (type))
     {
-- 
1.9.1

Re: [Ada/DWARF] printing array of variant records

[Joel Brobecker]

> This patch series aims at enhancing GDB when printing arrays of
> records whose contents and size is dynamic, in the case where
> we're relying purely on standard DWARF info (no GNAT encodings).
> 
> The patch series has been tested on x86_64-linux using the FSF
> testsuite, as well as a number of other platforms (Solaris, Windows,
> crosses) using AdaCore's testsuite.
> 
> Most patches are small fixes and enhancements. Patch #2 (Add valaddr
> support in dynamic property resolution) is interesting, thought,
> as it is one answer to a question I asked at the very beginning
> when we started reviewing patches introducing the concept of dynamic
> type resolution: what if the data is not stored in inferior memory?
> As it turns out, this patch series encounters one such case, because
> we're dealing with data that is not necessarily byte-aligned. And
> AdaCore also encountered another situation, with a function returning
> a reference to a dynamic object, with that reference being returned
> by register; hence no address either. Patch #2 enhances our framework
> by using the contents-and-address idea.
> 
> If there are no objections, I'd like to commit the patch series
> sometime early next week.

FYI: I've retested the patch series on x86_64-linux, and just pushed it.

-- 
Joel