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

Message ID 551E8BFB.1010000@adacore.com
State New, archived
Headers

Commit Message

Pierre-Marie de Rodat April 3, 2015, 12:47 p.m. UTC
  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?
  

Comments

Joel Brobecker April 3, 2015, 5:52 p.m. UTC | #1
> 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).
  
Pierre-Marie de Rodat April 24, 2015, 2:15 p.m. UTC | #2
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!
  
Joel Brobecker April 24, 2015, 4:33 p.m. UTC | #3
> 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.
  
Pierre-Marie de Rodat April 27, 2015, 9:08 a.m. UTC | #4
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. :-)
  

Patch

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