[PATCH 1/2] Preserve typedef layer when getting struct element

[PATCH 1/2] Preserve typedef layer when getting struct element

[Pierre-Marie de Rodat]

[-- Attachment #1: Type: text/plain, Size: 1764 bytes --]

On behalf of Joel:

Consider the following declarations:

    type Int_Access is access Integer;
    type Record_Type is record
       IA : Int_Access;
    end record;

    R : Record_Type;

Printing the type name of "R.IA" yields:

     (gdb) whatis r.ia
     type = access integer

It should be:

     (gdb) whatis r.ia
     type = bar.int_access

Looking at the debugging info, field "r.ia" is defined as
a typedef which has the name of the field type:

         .uleb128 0x3    # (DIE (0x4e) DW_TAG_typedef)
         .long   .LASF4  # DW_AT_name: "bar__int_access"
         .long   0x8b    # DW_AT_type

... with the typedef's target type being an anonymous pointer
type:

         .uleb128 0x7    # (DIE (0x8b) DW_TAG_pointer_type)
         .byte   0x8     # DW_AT_byte_size
         .long   0x91    # DW_AT_type

What happens here is that a couple of function in ada-lang.c
always start by stripping all typedef layers when handling
struct fields, with the effect of making us lose the type name
in this case.

We did not understand this at the time the code was written,
but typedefs should be stripped only when we know we do not
need them. So this patch, adjust the code to avoid the stripping
while handling the fields, and adds it back in the lone place
which handles the result of processing and didn't know how to
handle typedefs struct fields yet.

gdb/ChangeLog:

         * ada-lang.c (ada_is_tagged_type): Add call to ada_check_typedef.
         (ada_lookup_struct_elt_type): Remove calls to ada_check_typedef.
         (template_to_static_fixed_type): Call ada_check_typedef only
         when necessary.

gdb/testsuite/ChangeLog:

         * gdb.ada/rec_comp: New testcase.

No regression on x86_64-linux. Ok to push?

-- 
Pierre-Marie de Rodat

[-- Attachment #2: 0001-Ada-Preserve-typedef-layer-when-getting-struct-eleme.patch --]
[-- Type: text/x-diff, Size: 9202 bytes --]

From 567d1e0adbc7179bec6795cbbfa19248058373b2 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Thu, 12 Feb 2015 14:39:14 +0400
Subject: [PATCH 1/2] [Ada] Preserve typedef layer when getting struct element

Consider the following declarations:

   type Int_Access is access Integer;
   type Record_Type is record
      IA : Int_Access;
   end record;

   R : Record_Type;

Printing the type name of "R.IA" yields:

    (gdb) whatis r.ia
    type = access integer

It should be:

    (gdb) whatis r.ia
    type = bar.int_access

Looking at the debugging info, field "r.ia" is defined as
a typedef which has the name of the field type:

        .uleb128 0x3    # (DIE (0x4e) DW_TAG_typedef)
        .long   .LASF4  # DW_AT_name: "bar__int_access"
        .long   0x8b    # DW_AT_type

... with the typedef's target type being an anonymous pointer
type:

        .uleb128 0x7    # (DIE (0x8b) DW_TAG_pointer_type)
        .byte   0x8     # DW_AT_byte_size
        .long   0x91    # DW_AT_type

What happens here is that a couple of function in ada-lang.c
always start by stripping all typedef layers when handling
struct fields, with the effect of making us lose the type name
in this case.

We did not understand this at the time the code was written,
but typedefs should be stripped only when we know we do not
need them. So this patch, adjust the code to avoid the stripping
while handling the fields, and adds it back in the lone place
which handles the result of processing and didn't know how to
handle typedefs struct fields yet.

gdb/ChangeLog:

        * ada-lang.c (ada_is_tagged_type): Add call to ada_check_typedef.
        (ada_lookup_struct_elt_type): Remove calls to ada_check_typedef.
        (template_to_static_fixed_type): Call ada_check_typedef only
        when necessary.

gdb/testsuite/ChangeLog:

        * gdb.ada/rec_comp: New testcase.
---
 gdb/ada-lang.c                                  | 13 ++++++---
 gdb/testsuite/gdb.ada/rec_comp.exp              | 37 +++++++++++++++++++++++++
 gdb/testsuite/gdb.ada/rec_comp/bar_o203_012.adb | 32 +++++++++++++++++++++
 gdb/testsuite/gdb.ada/rec_comp/pck.adb          | 23 +++++++++++++++
 gdb/testsuite/gdb.ada/rec_comp/pck.ads          | 22 +++++++++++++++
 5 files changed, 123 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.ada/rec_comp.exp
 create mode 100644 gdb/testsuite/gdb.ada/rec_comp/bar_o203_012.adb
 create mode 100644 gdb/testsuite/gdb.ada/rec_comp/pck.adb
 create mode 100644 gdb/testsuite/gdb.ada/rec_comp/pck.ads

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 124e370..e147d5a 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -6399,6 +6399,8 @@ ada_is_tagged_type (struct type *type, int refok)
 int
 ada_is_tag_type (struct type *type)
 {
+  type = ada_check_typedef (type);
+
   if (type == NULL || TYPE_CODE (type) != TYPE_CODE_PTR)
     return 0;
   else
@@ -7322,7 +7324,7 @@ ada_lookup_struct_elt_type (struct type *type, char *name, int refok,
         {
           if (dispp != NULL)
             *dispp += TYPE_FIELD_BITPOS (type, i) / 8;
-          return ada_check_typedef (TYPE_FIELD_TYPE (type, i));
+          return TYPE_FIELD_TYPE (type, i);
         }
 
       else if (ada_is_wrapper_field (type, i))
@@ -7354,7 +7356,7 @@ ada_lookup_struct_elt_type (struct type *type, char *name, int refok,
               disp = 0;
 	      if (v_field_name != NULL 
 		  && field_name_match (v_field_name, name))
-		t = ada_check_typedef (TYPE_FIELD_TYPE (field_type, j));
+		t = TYPE_FIELD_TYPE (field_type, j);
 	      else
 		t = ada_lookup_struct_elt_type (TYPE_FIELD_TYPE (field_type,
 								 j),
@@ -8177,11 +8179,14 @@ template_to_static_fixed_type (struct type *type0)
 
   for (f = 0; f < nfields; f += 1)
     {
-      struct type *field_type = ada_check_typedef (TYPE_FIELD_TYPE (type0, f));
+      struct type *field_type = TYPE_FIELD_TYPE (type0, f);
       struct type *new_type;
 
       if (is_dynamic_field (type0, f))
-        new_type = to_static_fixed_type (TYPE_TARGET_TYPE (field_type));
+	{
+	  field_type = ada_check_typedef (field_type);
+          new_type = to_static_fixed_type (TYPE_TARGET_TYPE (field_type));
+	}
       else
         new_type = static_unwrap_type (field_type);
       if (type == type0 && new_type != field_type)
diff --git a/gdb/testsuite/gdb.ada/rec_comp.exp b/gdb/testsuite/gdb.ada/rec_comp.exp
new file mode 100644
index 0000000..c462643
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/rec_comp.exp
@@ -0,0 +1,37 @@
+# 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 bar_o203_012
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug ]] != "" } {
+  return -1
+}
+
+clean_restart ${testfile}
+
+set bp_location [gdb_get_line_number "STOP" ${testdir}/bar_o203_012.adb]
+if ![runto "bar_o203_012.adb:$bp_location" ] then {
+  perror "Couldn't run ${testfile}"
+  return
+}
+
+gdb_test "whatis r.ia" " = bar_o203_012.int_access"
+
+gdb_test "ptype r" \
+    " = record\r\n *ia: bar_o203_012\.int_access;\r\nend record"
+
+gdb_test "ptype r.ia" " = access <$decimal-byte integer>"
diff --git a/gdb/testsuite/gdb.ada/rec_comp/bar_o203_012.adb b/gdb/testsuite/gdb.ada/rec_comp/bar_o203_012.adb
new file mode 100644
index 0000000..9783f51
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/rec_comp/bar_o203_012.adb
@@ -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/>.
+
+with Pck; use Pck;
+
+procedure Bar_O203_012 is
+   type Int_Access is access Integer;
+   type Record_Type is record
+      IA : Int_Access;
+   end record;
+
+   R : Record_Type;
+   IA : Int_Access;
+begin
+   R.IA := new Integer'(3); -- STOP
+   IA := R.IA;
+   Do_Nothing (R'Address);
+   Do_Nothing (IA'Address);
+end Bar_O203_012;
+
diff --git a/gdb/testsuite/gdb.ada/rec_comp/pck.adb b/gdb/testsuite/gdb.ada/rec_comp/pck.adb
new file mode 100644
index 0000000..c0465af
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/rec_comp/pck.adb
@@ -0,0 +1,23 @@
+--  Copyright 2011-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 : System.Address) is
+   begin
+      null;
+   end Do_Nothing;
+
+end Pck;
diff --git a/gdb/testsuite/gdb.ada/rec_comp/pck.ads b/gdb/testsuite/gdb.ada/rec_comp/pck.ads
new file mode 100644
index 0000000..594847c
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/rec_comp/pck.ads
@@ -0,0 +1,22 @@
+--  Copyright 2011-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
+
+   procedure Do_Nothing (A : System.Address);
+
+end Pck;
-- 
2.3.4

[PATCH 2/2] Cache all static structures and reset cache during resolution

[Pierre-Marie de Rodat]

[-- Attachment #1: Type: text/plain, Size: 1432 bytes --]

Here is a little cleanup after the previous patch:

Currently, ada-lang.c:template_to_static_fixed_type (working on
structure types only) caches its result into the unused TYPE_TARGET_TYPE
field. This introduces inconsistencies when the input type is
specialized, for instance during type resolution: the cached static
fixed type is copied along with the original type, but it's no longer
adapted to the copy once the copy is modified:
template_to_static_fixed_type has to compute another static fixed type
for it.

This change first introduces a cache reset during type resolution for
structure types so that this inconsistency does not happen anymore. It
also makes template_to_static_fixed_type smarter with respect to types
that do not need static fixed copies so that less computations is done
in general.

This inconsistency was spotted thanks to code reading, not because of
any sort of failure and we did not manage to exhibit a failure yet, so
no testcase for this.

gdb/ChangeLog:

         * ada-lang.c (template_to_static_fixed_type): Return input type
         when it is already fixed. Cache the input type itself when not
         creating a static fixed copy. Make it explicit that we never
         molestate the input type.
         * gdbtypes.c (resolve_dynamic_struct): Reset the
         TYPE_TARGET_TYPE field for resolved copies.

No regression as well on x86_64-linux. Ok to push?

-- 
Pierre-Marie de Rodat

[-- Attachment #2: 0002-Ada-Cache-all-static-structures-and-reset-cache-duri.patch --]
[-- Type: text/x-diff, Size: 4794 bytes --]

From d0c3ff848af86a6174ecc83022b58e5af76f4a98 Mon Sep 17 00:00:00 2001
From: Pierre-Marie de Rodat <derodat@adacore.com>
Date: Tue, 31 Mar 2015 17:18:12 +0200
Subject: [PATCH 2/2] [Ada] Cache all static structures and reset cache during
 resolution

Currently, ada-lang.c:template_to_static_fixed_type (working on
structure types only) caches its result into the unused TYPE_TARGET_TYPE
field. This introduces inconsistencies when the input type is
specialized, for instance during type resolution: the cached static
fixed type is copied along with the original type, but it's no longer
adapted to the copy once the copy is modified:
template_to_static_fixed_type has to compute another static fixed type
for it.

This change first introduces a cache reset during type resolution for
structure types so that this inconsistency does not happen anymore. It
also makes template_to_static_fixed_type smarter with respect to types
that do not need static fixed copies so that less computations is done
in general.

This inconsistency was spotted thanks to code reading, not because of
any sort of failure and we did not manage to exhibit a failure yet, so
no testcase for this.

gdb/ChangeLog:

	* ada-lang.c (template_to_static_fixed_type): Return input type
	when it is already fixed. Cache the input type itself when not
	creating a static fixed copy. Make it explicit that we never
	molestate the input type.
	* gdbtypes.c (resolve_dynamic_struct): Reset the
	TYPE_TARGET_TYPE field for resolved copies.
---
 gdb/ada-lang.c | 52 ++++++++++++++++++++++++++++++++++------------------
 gdb/gdbtypes.c |  4 ++++
 2 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index e147d5a..d034049 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -8171,11 +8171,21 @@ template_to_static_fixed_type (struct type *type0)
   int nfields;
   int f;
 
+  /* No need no do anything if the input type is already fixed.  */
+  if (TYPE_FIXED_INSTANCE (type0))
+    return type0;
+
+  /* Likewise if we already have computed the static approximation.  */
   if (TYPE_TARGET_TYPE (type0) != NULL)
     return TYPE_TARGET_TYPE (type0);
 
-  nfields = TYPE_NFIELDS (type0);
+  /* Don't clone TYPE0 until we are sure we are going to need a copy.  */
   type = type0;
+  nfields = TYPE_NFIELDS (type0);
+
+  /* Whether or not we cloned TYPE0, cache the result so that we don't do
+     recompute all over next time.  */
+  TYPE_TARGET_TYPE (type0) = type;
 
   for (f = 0; f < nfields; f += 1)
     {
@@ -8189,24 +8199,30 @@ template_to_static_fixed_type (struct type *type0)
 	}
       else
         new_type = static_unwrap_type (field_type);
-      if (type == type0 && new_type != field_type)
-        {
-          TYPE_TARGET_TYPE (type0) = type = alloc_type_copy (type0);
-          TYPE_CODE (type) = TYPE_CODE (type0);
-          INIT_CPLUS_SPECIFIC (type);
-          TYPE_NFIELDS (type) = nfields;
-          TYPE_FIELDS (type) = (struct field *)
-            TYPE_ALLOC (type, nfields * sizeof (struct field));
-          memcpy (TYPE_FIELDS (type), TYPE_FIELDS (type0),
-                  sizeof (struct field) * nfields);
-          TYPE_NAME (type) = ada_type_name (type0);
-          TYPE_TAG_NAME (type) = NULL;
-	  TYPE_FIXED_INSTANCE (type) = 1;
-          TYPE_LENGTH (type) = 0;
-        }
-      TYPE_FIELD_TYPE (type, f) = new_type;
-      TYPE_FIELD_NAME (type, f) = TYPE_FIELD_NAME (type0, f);
+
+      if (new_type != field_type)
+	{
+	  /* Clone TYPE0 only the first time we get a new field type.  */
+	  if (type == type0)
+	    {
+	      TYPE_TARGET_TYPE (type0) = type = alloc_type_copy (type0);
+	      TYPE_CODE (type) = TYPE_CODE (type0);
+	      INIT_CPLUS_SPECIFIC (type);
+	      TYPE_NFIELDS (type) = nfields;
+	      TYPE_FIELDS (type) = (struct field *)
+		TYPE_ALLOC (type, nfields * sizeof (struct field));
+	      memcpy (TYPE_FIELDS (type), TYPE_FIELDS (type0),
+		      sizeof (struct field) * nfields);
+	      TYPE_NAME (type) = ada_type_name (type0);
+	      TYPE_TAG_NAME (type) = NULL;
+	      TYPE_FIXED_INSTANCE (type) = 1;
+	      TYPE_LENGTH (type) = 0;
+	    }
+	  TYPE_FIELD_TYPE (type, f) = new_type;
+	  TYPE_FIELD_NAME (type, f) = TYPE_FIELD_NAME (type0, f);
+	}
     }
+
   return type;
 }
 
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 217ec70..ff80035 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -2013,6 +2013,10 @@ resolve_dynamic_struct (struct type *type,
   TYPE_LENGTH (resolved_type)
     = (resolved_type_bit_length + TARGET_CHAR_BIT - 1) / TARGET_CHAR_BIT;
 
+  /* The Ada language uses this field as a cache for static fixed types: reset
+     it as RESOLVED_TYPE must have its own static fixed type.  */
+  TYPE_TARGET_TYPE (resolved_type) = NULL;
+
   return resolved_type;
 }
 
-- 
2.3.4

Re: [PATCH 1/2] Preserve typedef layer when getting struct element

[Joel Brobecker]

> gdb/ChangeLog:
> 
>         * ada-lang.c (ada_is_tagged_type): Add call to ada_check_typedef.
>         (ada_lookup_struct_elt_type): Remove calls to ada_check_typedef.
>         (template_to_static_fixed_type): Call ada_check_typedef only
>         when necessary.
> 
> gdb/testsuite/ChangeLog:
> 
>         * gdb.ada/rec_comp: New testcase.
> 
> No regression on x86_64-linux. Ok to push?

Thanks, Pierre-Marie. This one looks good and can be checked in
on its own.

-- 
Joel

Re: [PATCH 2/2] Cache all static structures and reset cache during resolution

[Joel Brobecker]

> gdb/ChangeLog:
>
>         * ada-lang.c (template_to_static_fixed_type): Return input type
>         when it is already fixed. Cache the input type itself when not
>         creating a static fixed copy. Make it explicit that we never
>         molestate the input type.
>         * gdbtypes.c (resolve_dynamic_struct): Reset the
>         TYPE_TARGET_TYPE field for resolved copies.

This one looks good to me too, but let's give people until
Mon Apr 13th to comment on the change in gdbtypes.c. I believe
this is the simplest approach, and, to me, the fact that the
TYPE_TARGET_TYPE is currently only used by Ada doesn't make it
a problem for handling it in generic code. But if others object,
then we can use the hack you had in ada-lang before you came up
with this version of the patch (the hack is based on the fixed
version of the type having a backlink to the original type it fixed,
thus allowing us to verify whether the cached TYPE_TARGET_TYPE
was copied from another type and is therefore irrelevant to this
type).

-- 
Joel

Re: [PATCH 2/2] Cache all static structures and reset cache during resolution

[Pierre-Marie de Rodat]

On 04/03/2015 07:52 PM, Joel Brobecker wrote:
> This one looks good to me too, but let's give people until
> Mon Apr 13th to comment on the change in gdbtypes.c.

No comment so far: shall I push this to master?

Thank you in advance!

-- 
Pierre-Marie de Rodat

Re: [PATCH 2/2] Cache all static structures and reset cache during resolution

[Joel Brobecker]

> On 04/03/2015 07:52 PM, Joel Brobecker wrote:
> >This one looks good to me too, but let's give people until
> >Mon Apr 13th to comment on the change in gdbtypes.c.
> 
> No comment so far: shall I push this to master?

I just re-reviewed the patch, JIC, and the answer is still yes :-).
So, go ahead and push.

-- 
Joel

Re: [PATCH 2/2] Cache all static structures and reset cache during resolution

[Pierre-Marie de Rodat]

On 04/24/2015 06:33 PM, Joel Brobecker wrote:
> I just re-reviewed the patch, JIC, and the answer is still yes :-).
> So, go ahead and push.

Thank you very much! Both patches are pushed. :-)

-- 
Pierre-Marie de Rodat