[PATCH 1/2] Use gdbarch obstack to allocate types in alloc_type_arch

[PATCH 1/2] Use gdbarch obstack to allocate types in alloc_type_arch

[Patrick Palka]

For the command "gdb gdb" valgrind currently reports 100s of individual
memory leaks, 500 of which originate solely out of the function
alloc_type_arch.  This function allocates a "struct type" associated
with the given gdbarch using malloc but apparently the types allocated
by this function are never freed.

This patch fixes these leaks by making the function alloc_type_arch
allocate these gdbarch-associated types on the gdbarch obstack instead
of on the general heap.  Since, from what I can tell, the types
allocated by this function are all fundamental "wired-in" types, such
types would not benefit from more granular memory management anyway.
They would likely live as long as the gdbarch is alive so allocating
them on the gdbarch obstack makes sense.

With this patch, the number of individual vargrind warnings emitted for
the command "gdb gdb" drops from ~800 to ~300.

Tested on x86_64-unknown-linux-gnu.  Does this fix make sense?  It may
not be ideal (more disciplined memory management may be?), but for the
time being it helps reduce the noise coming from valgrind.  Or maybe
there is a good reason that these types are allocated on the heap...

gdb/ChangeLog:

	* gdbtypes.c (alloc_type_arch): Allocate the type on the given
	gdbarch obstack instead of on the heap.  Update commentary
	accordingly.
---
 gdb/gdbtypes.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index ca86fbd..979ed6c 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -187,7 +187,7 @@ alloc_type (struct objfile *objfile)
 
 /* Allocate a new GDBARCH-associated type structure and fill it
    with some defaults.  Space for the type structure is allocated
-   on the heap.  */
+   on the obstack associated with GDBARCH.  */
 
 struct type *
 alloc_type_arch (struct gdbarch *gdbarch)
@@ -198,8 +198,8 @@ alloc_type_arch (struct gdbarch *gdbarch)
 
   /* Alloc the structure and start off with all fields zeroed.  */
 
-  type = XCNEW (struct type);
-  TYPE_MAIN_TYPE (type) = XCNEW (struct main_type);
+  type = GDBARCH_OBSTACK_ZALLOC (gdbarch, struct type);
+  TYPE_MAIN_TYPE (type) = GDBARCH_OBSTACK_ZALLOC (gdbarch, struct main_type);
 
   TYPE_OBJFILE_OWNED (type) = 0;
   TYPE_OWNER (type).gdbarch = gdbarch;
-- 
2.5.0.rc0.5.g91e10c5.dirty

[PATCH 2/2] Use gdbarch obstack to allocate the TYPE_NAME string in arch_type

[Patrick Palka]

Since the type whose name is being set is now being allocated on the
gdbarch obstack, we should allocate its TYPE_NAME on the obstack too.
This reduces the number of individual valgrind warnings for the command
"gdb gdb" from ~300 to ~150.

Tested on x86_64-unknown-linux-gnu.

[ I have a few more patches on top of these that together bring the total
  number of valgrind warnings for the command "gdb gdb" down to ~30
  but they are more controversial than these two, and if these aren't OK
  then the rest definitely aren't OK.  ]

gdb/ChangeLog:

	* gdbarch.h (gdbarch_obstack_strdup): Declare.
	* gdbarch.c (gdbarch_obstack_strdup): Define.
	* gdbtypes.c (arch_type): Use it.
---
 gdb/gdbarch.c  | 10 ++++++++++
 gdb/gdbarch.h  |  5 +++++
 gdb/gdbtypes.c |  2 +-
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index c289334..a232177 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -449,6 +449,16 @@ gdbarch_obstack_zalloc (struct gdbarch *arch, long size)
   return data;
 }
 
+/* See gdbarch.h.  */
+
+char *
+gdbarch_obstack_strdup (struct gdbarch *gdbarch, const char *string)
+{
+  char *obstring = gdbarch_obstack_zalloc (gdbarch, strlen (string) + 1);
+  strcpy (obstring, string);
+  return obstring;
+}
+
 
 /* Free a gdbarch struct.  This should never happen in normal
    operation --- once you've created a gdbarch, you keep it around.
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 7d6a0cf..fc35136 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1614,6 +1614,11 @@ extern void *gdbarch_obstack_zalloc (struct gdbarch *gdbarch, long size);
 #define GDBARCH_OBSTACK_CALLOC(GDBARCH, NR, TYPE) ((TYPE *) gdbarch_obstack_zalloc ((GDBARCH), (NR) * sizeof (TYPE)))
 #define GDBARCH_OBSTACK_ZALLOC(GDBARCH, TYPE) ((TYPE *) gdbarch_obstack_zalloc ((GDBARCH), sizeof (TYPE)))
 
+/* Duplicate STRING, returning an equivalent string that's allocated on the
+   obstack associated with GDBARCH.  The string is freed when the corresponding
+   architecture is also freed.  */
+
+extern char *gdbarch_obstack_strdup (struct gdbarch *gdbarch, const char *string);
 
 /* Helper function.  Force an update of the current architecture.
 
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 979ed6c..86c6282 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -4539,7 +4539,7 @@ arch_type (struct gdbarch *gdbarch,
   TYPE_LENGTH (type) = length;
 
   if (name)
-    TYPE_NAME (type) = xstrdup (name);
+    TYPE_NAME (type) = gdbarch_obstack_strdup (gdbarch, name);
 
   return type;
 }
-- 
2.5.0.rc0.5.g91e10c5.dirty

Re: [PATCH 2/2] Use gdbarch obstack to allocate the TYPE_NAME string in arch_type

[Pedro Alves]

On 06/30/2015 03:28 AM, Patrick Palka wrote:
> Since the type whose name is being set is now being allocated on the
> gdbarch obstack, we should allocate its TYPE_NAME on the obstack too.
> This reduces the number of individual valgrind warnings for the command
> "gdb gdb" from ~300 to ~150.
> 
> Tested on x86_64-unknown-linux-gnu.
> 
> [ I have a few more patches on top of these that together bring the total
>   number of valgrind warnings for the command "gdb gdb" down to ~30
>   but they are more controversial than these two, and if these aren't OK
>   then the rest definitely aren't OK.  ]

Both patches look fine to me.  If this blows up for some reason, I guess
we'll take the opportunity to add a comment explaining why these must go
on the heap.  :-)

Thanks,
Pedro Alves

Re: [PATCH 2/2] Use gdbarch obstack to allocate the TYPE_NAME string in arch_type

[Patrick Palka]

On Tue, Jun 30, 2015 at 5:36 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/30/2015 03:28 AM, Patrick Palka wrote:
>> Since the type whose name is being set is now being allocated on the
>> gdbarch obstack, we should allocate its TYPE_NAME on the obstack too.
>> This reduces the number of individual valgrind warnings for the command
>> "gdb gdb" from ~300 to ~150.
>>
>> Tested on x86_64-unknown-linux-gnu.
>>
>> [ I have a few more patches on top of these that together bring the total
>>   number of valgrind warnings for the command "gdb gdb" down to ~30
>>   but they are more controversial than these two, and if these aren't OK
>>   then the rest definitely aren't OK.  ]
>
> Both patches look fine to me.  If this blows up for some reason, I guess
> we'll take the opportunity to add a comment explaining why these must go
> on the heap.  :-)

Heh, sounds good.  I will hold off committing these patches at least
until 7.10 gets released to avoid unnecessary breakage.

Re: [PATCH 2/2] Use gdbarch obstack to allocate the TYPE_NAME string in arch_type

[Patrick Palka]

On Tue, Jun 30, 2015 at 4:04 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Tue, Jun 30, 2015 at 5:36 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 06/30/2015 03:28 AM, Patrick Palka wrote:
>>> Since the type whose name is being set is now being allocated on the
>>> gdbarch obstack, we should allocate its TYPE_NAME on the obstack too.
>>> This reduces the number of individual valgrind warnings for the command
>>> "gdb gdb" from ~300 to ~150.
>>>
>>> Tested on x86_64-unknown-linux-gnu.
>>>
>>> [ I have a few more patches on top of these that together bring the total
>>>   number of valgrind warnings for the command "gdb gdb" down to ~30
>>>   but they are more controversial than these two, and if these aren't OK
>>>   then the rest definitely aren't OK.  ]
>>
>> Both patches look fine to me.  If this blows up for some reason, I guess
>> we'll take the opportunity to add a comment explaining why these must go
>> on the heap.  :-)
>
> Heh, sounds good.  I will hold off committing these patches at least
> until 7.10 gets released to avoid unnecessary breakage.

Committed. Let's see if anything breaks.

Re: [PATCH 2/2] Use gdbarch obstack to allocate the TYPE_NAME string in arch_type

[Doug Evans]

Patrick Palka <patrick@parcs.ath.cx> writes:
> Since the type whose name is being set is now being allocated on the
> gdbarch obstack, we should allocate its TYPE_NAME on the obstack too.
> This reduces the number of individual valgrind warnings for the command
> "gdb gdb" from ~300 to ~150.
>
> Tested on x86_64-unknown-linux-gnu.
>
> [ I have a few more patches on top of these that together bring the total
>   number of valgrind warnings for the command "gdb gdb" down to ~30
>   but they are more controversial than these two, and if these aren't OK
>   then the rest definitely aren't OK.  ]
>
> gdb/ChangeLog:
>
> 	* gdbarch.h (gdbarch_obstack_strdup): Declare.
> 	* gdbarch.c (gdbarch_obstack_strdup): Define.
> 	* gdbtypes.c (arch_type): Use it.

Hi.
A couple of comments.

1) gdbarch.[ch] are machine generated.
IIUC, you have to edit gdbarch.sh and then run it to regenerate gdbarch.[ch].

2) I would have done this slightly differently.
If the obstack API doesn't provide a strdup functionality,
I wouldn't insist on trying to add it there. But I would like
to see it added to gdb in an application-independent way.
(make it non-gdbarch specific). obstack_strdup sounds sufficiently
useful and generic enough. If one wants to add a
gdbarch_obstack_strdup wrapper on top of that, fine by me.
IOW, add obstack_strdup to gdb_obstack.[ch].

Re: [PATCH 2/2] Use gdbarch obstack to allocate the TYPE_NAME string in arch_type

[Patrick Palka]

On Sat, Aug 29, 2015 at 2:19 PM, Doug Evans <xdje42@gmail.com> wrote:
> Patrick Palka <patrick@parcs.ath.cx> writes:
>> Since the type whose name is being set is now being allocated on the
>> gdbarch obstack, we should allocate its TYPE_NAME on the obstack too.
>> This reduces the number of individual valgrind warnings for the command
>> "gdb gdb" from ~300 to ~150.
>>
>> Tested on x86_64-unknown-linux-gnu.
>>
>> [ I have a few more patches on top of these that together bring the total
>>   number of valgrind warnings for the command "gdb gdb" down to ~30
>>   but they are more controversial than these two, and if these aren't OK
>>   then the rest definitely aren't OK.  ]
>>
>> gdb/ChangeLog:
>>
>>       * gdbarch.h (gdbarch_obstack_strdup): Declare.
>>       * gdbarch.c (gdbarch_obstack_strdup): Define.
>>       * gdbtypes.c (arch_type): Use it.
>
> Hi.
> A couple of comments.
>
> 1) gdbarch.[ch] are machine generated.
> IIUC, you have to edit gdbarch.sh and then run it to regenerate gdbarch.[ch].

Oops :/ I will fix this.

>
> 2) I would have done this slightly differently.
> If the obstack API doesn't provide a strdup functionality,
> I wouldn't insist on trying to add it there. But I would like
> to see it added to gdb in an application-independent way.
> (make it non-gdbarch specific). obstack_strdup sounds sufficiently
> useful and generic enough. If one wants to add a
> gdbarch_obstack_strdup wrapper on top of that, fine by me.
> IOW, add obstack_strdup to gdb_obstack.[ch].

And I will do this as well.  Thanks for checking out these two patches.

[PATCH 2/2] Use gdbarch obstack to allocate the TYPE_NAME string in arch_type

[Patrick Palka]

[ The earlier committed version of this patch was reverted.  ]

Since the type whose name is being set is now being allocated on the
gdbarch obstack, we should allocate its TYPE_NAME on the obstack too.
This reduces the number of individual valgrind warnings for the command
"gdb gdb" from ~300 to ~150.

Tested on x86_64-unknown-linux-gnu.

gdb/ChangeLog:

	* gdb_obstack.h (obstack_strdup): Declare.
	* gdb_obstack.c (obstack_strdup): Declare.
	* gdbarch.sh (gdbarch_obstack_strdup): Declare and define.
	* gdbarch.c: Regenerate.
	* gdbarch.h: Regenerate.
	* gdbtypes.c (arch_type): Use gdbarch_obstack_strdup.
---
 gdb/gdb_obstack.c | 10 ++++++++++
 gdb/gdb_obstack.h |  5 +++++
 gdb/gdbarch.c     |  8 ++++++++
 gdb/gdbarch.h     |  5 +++++
 gdb/gdbarch.sh    | 13 +++++++++++++
 gdb/gdbtypes.c    |  2 +-
 6 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/gdb/gdb_obstack.c b/gdb/gdb_obstack.c
index b972eab..d8d03df 100644
--- a/gdb/gdb_obstack.c
+++ b/gdb/gdb_obstack.c
@@ -45,3 +45,13 @@ obconcat (struct obstack *obstackp, ...)
 
   return obstack_finish (obstackp);
 }
+
+/* See gdb_obstack.h.  */
+
+char *
+obstack_strdup (struct obstack *obstackp, const char *string)
+{
+  char *obstring = obstack_alloc (obstackp, strlen (string) + 1);
+  strcpy (obstring, string);
+  return obstring;
+}
diff --git a/gdb/gdb_obstack.h b/gdb/gdb_obstack.h
index 826c8b2..3272721 100644
--- a/gdb/gdb_obstack.h
+++ b/gdb/gdb_obstack.h
@@ -58,4 +58,9 @@
 
 extern char *obconcat (struct obstack *obstackp, ...) ATTRIBUTE_SENTINEL;
 
+/* Duplicate STRING, returning an equivalent string that's allocated on the
+   obstack OBSTACKP.  */
+
+extern char *obstack_strdup (struct obstack *obstackp, const char *string);
+
 #endif
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 0d4142b..f04eef9 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -449,6 +449,14 @@ gdbarch_obstack_zalloc (struct gdbarch *arch, long size)
   return data;
 }
 
+/* See gdbarch.h.  */
+
+char *
+gdbarch_obstack_strdup (struct gdbarch *arch, const char *string)
+{
+  return obstack_strdup (arch->obstack, string);
+}
+
 
 /* Free a gdbarch struct.  This should never happen in normal
    operation --- once you've created a gdbarch, you keep it around.
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 7df37c9..82e0259 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1618,6 +1618,11 @@ extern void *gdbarch_obstack_zalloc (struct gdbarch *gdbarch, long size);
 #define GDBARCH_OBSTACK_CALLOC(GDBARCH, NR, TYPE) ((TYPE *) gdbarch_obstack_zalloc ((GDBARCH), (NR) * sizeof (TYPE)))
 #define GDBARCH_OBSTACK_ZALLOC(GDBARCH, TYPE) ((TYPE *) gdbarch_obstack_zalloc ((GDBARCH), sizeof (TYPE)))
 
+/* Duplicate STRING, returning an equivalent string that's allocated on the
+   obstack associated with GDBARCH.  The string is freed when the corresponding
+   architecture is also freed.  */
+
+extern char *gdbarch_obstack_strdup (struct gdbarch *arch, const char *string);
 
 /* Helper function.  Force an update of the current architecture.
 
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 34e6a74..388920f 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -1486,6 +1486,11 @@ extern void *gdbarch_obstack_zalloc (struct gdbarch *gdbarch, long size);
 #define GDBARCH_OBSTACK_CALLOC(GDBARCH, NR, TYPE) ((TYPE *) gdbarch_obstack_zalloc ((GDBARCH), (NR) * sizeof (TYPE)))
 #define GDBARCH_OBSTACK_ZALLOC(GDBARCH, TYPE) ((TYPE *) gdbarch_obstack_zalloc ((GDBARCH), sizeof (TYPE)))
 
+/* Duplicate STRING, returning an equivalent string that's allocated on the
+   obstack associated with GDBARCH.  The string is freed when the corresponding
+   architecture is also freed.  */
+
+extern char *gdbarch_obstack_strdup (struct gdbarch *arch, const char *string);
 
 /* Helper function.  Force an update of the current architecture.
 
@@ -1791,6 +1796,14 @@ gdbarch_obstack_zalloc (struct gdbarch *arch, long size)
   return data;
 }
 
+/* See gdbarch.h.  */
+
+char *
+gdbarch_obstack_strdup (struct gdbarch *arch, const char *string)
+{
+  return obstack_strdup (arch->obstack, string);
+}
+
 
 /* Free a gdbarch struct.  This should never happen in normal
    operation --- once you've created a gdbarch, you keep it around.
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 7a18bed..12ff014 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -4549,7 +4549,7 @@ arch_type (struct gdbarch *gdbarch,
   TYPE_LENGTH (type) = length;
 
   if (name)
-    TYPE_NAME (type) = xstrdup (name);
+    TYPE_NAME (type) = gdbarch_obstack_strdup (gdbarch, name);
 
   return type;
 }
-- 
2.5.1.426.g4b27743.dirty

Re: [PATCH 2/2] Use gdbarch obstack to allocate the TYPE_NAME string in arch_type

[Doug Evans]

Patrick Palka <patrick@parcs.ath.cx> writes:
> [ The earlier committed version of this patch was reverted.  ]
>
> Since the type whose name is being set is now being allocated on the
> gdbarch obstack, we should allocate its TYPE_NAME on the obstack too.
> This reduces the number of individual valgrind warnings for the command
> "gdb gdb" from ~300 to ~150.
>
> Tested on x86_64-unknown-linux-gnu.
>
> gdb/ChangeLog:
>
> 	* gdb_obstack.h (obstack_strdup): Declare.
> 	* gdb_obstack.c (obstack_strdup): Declare.
> 	* gdbarch.sh (gdbarch_obstack_strdup): Declare and define.
> 	* gdbarch.c: Regenerate.
> 	* gdbarch.h: Regenerate.
> 	* gdbtypes.c (arch_type): Use gdbarch_obstack_strdup.

LGTM.
Thanks!

Re: [PATCH 1/2] Use gdbarch obstack to allocate types in alloc_type_arch

[Doug Evans]

Patrick Palka <patrick@parcs.ath.cx> writes:
> For the command "gdb gdb" valgrind currently reports 100s of individual
> memory leaks, 500 of which originate solely out of the function
> alloc_type_arch.  This function allocates a "struct type" associated
> with the given gdbarch using malloc but apparently the types allocated
> by this function are never freed.
>
> This patch fixes these leaks by making the function alloc_type_arch
> allocate these gdbarch-associated types on the gdbarch obstack instead
> of on the general heap.  Since, from what I can tell, the types
> allocated by this function are all fundamental "wired-in" types, such
> types would not benefit from more granular memory management anyway.
> They would likely live as long as the gdbarch is alive so allocating
> them on the gdbarch obstack makes sense.
>
> With this patch, the number of individual vargrind warnings emitted for
> the command "gdb gdb" drops from ~800 to ~300.
>
> Tested on x86_64-unknown-linux-gnu.  Does this fix make sense?  It may
> not be ideal (more disciplined memory management may be?), but for the
> time being it helps reduce the noise coming from valgrind.  Or maybe
> there is a good reason that these types are allocated on the heap...

OOC, Are these real leaks?
I wasn't aware of arches ever being freed.
I'm all for improving the S/N ratio of valgrind though.

How are you running valgrind?
I'd like to recreate this for myself.

btw, while looking into this I was reading copy_type_recursive.
The first thing I noticed was this:

  if (! TYPE_OBJFILE_OWNED (type))
    return type;
  ...
  new_type = alloc_type_arch (get_type_arch (type));

and my first thought was "Eh? We're copying an objfile's type but we're
storing it with the objfile's arch???" There's nothing in the name
"copy_type_recursive" that conveys this subtlety.
Then I realized this function is for, for example, saving the value
history when an objfile goes away (grep for it in value.c).
The copied type can't live with the objfile, it's going away, and there's
only one other place that it can live with: the objfile's arch (arches
never go away).

Then I read this comment for copy_type_recursive:

/* Recursively copy (deep copy) TYPE, if it is associated with
   OBJFILE.  Return a new type allocated using malloc, a saved type if
   we have already visited TYPE (using COPIED_TYPES), or TYPE if it is
   not associated with OBJFILE.  */

Note the "Return a new type using malloc"

This comment is lacking: it would be really nice if it explained
*why* the new type is saved with malloc. This critical feature of this
routine is a bit subtle given the name is just "copy_type_recursive".
Or better yet change the visible (exported) name of the function to
"preserve_type", or some such just as its callers are named preserve_foo,
rename copy_type_recursive to preserve_type_recursive, make it static,
and have the former call the latter. [The _recursive in the name isn't really
something callers care about. If one feels it's important to include
this aspect in the name how about something like preserve_type_deep?]

To make a long story short, I'm guessing that's the history behind why
alloc_type_arch used malloc (I know there's discussion of this
in the thread).

At the least, we'll need to update copy_type_recursive's function
comment and change the malloc reference.

Re: [PATCH 1/2] Use gdbarch obstack to allocate types in alloc_type_arch

[Patrick Palka]

On Sat, Aug 29, 2015 at 2:05 PM, Doug Evans <xdje42@gmail.com> wrote:
> Patrick Palka <patrick@parcs.ath.cx> writes:
>> For the command "gdb gdb" valgrind currently reports 100s of individual
>> memory leaks, 500 of which originate solely out of the function
>> alloc_type_arch.  This function allocates a "struct type" associated
>> with the given gdbarch using malloc but apparently the types allocated
>> by this function are never freed.
>>
>> This patch fixes these leaks by making the function alloc_type_arch
>> allocate these gdbarch-associated types on the gdbarch obstack instead
>> of on the general heap.  Since, from what I can tell, the types
>> allocated by this function are all fundamental "wired-in" types, such
>> types would not benefit from more granular memory management anyway.
>> They would likely live as long as the gdbarch is alive so allocating
>> them on the gdbarch obstack makes sense.
>>
>> With this patch, the number of individual vargrind warnings emitted for
>> the command "gdb gdb" drops from ~800 to ~300.
>>
>> Tested on x86_64-unknown-linux-gnu.  Does this fix make sense?  It may
>> not be ideal (more disciplined memory management may be?), but for the
>> time being it helps reduce the noise coming from valgrind.  Or maybe
>> there is a good reason that these types are allocated on the heap...
>
> OOC, Are these real leaks?
> I wasn't aware of arches ever being freed.
> I'm all for improving the S/N ratio of valgrind though.

Yeah this is just to reduce the number of warnings emitted by
valgrind. They aren't real leaks (and don't amount to much
memory-wise).

>
> How are you running valgrind?
> I'd like to recreate this for myself.

I'm doing "valgrind --leak-check=full --log-file=valgrind.log $GDB
$GDB" and then exiting via "quit" at the prompt.

>
> btw, while looking into this I was reading copy_type_recursive.
> The first thing I noticed was this:
>
>   if (! TYPE_OBJFILE_OWNED (type))
>     return type;
>   ...
>   new_type = alloc_type_arch (get_type_arch (type));
>
> and my first thought was "Eh? We're copying an objfile's type but we're
> storing it with the objfile's arch???" There's nothing in the name
> "copy_type_recursive" that conveys this subtlety.
> Then I realized this function is for, for example, saving the value
> history when an objfile goes away (grep for it in value.c).
> The copied type can't live with the objfile, it's going away, and there's
> only one other place that it can live with: the objfile's arch (arches
> never go away).

I see.

>
> Then I read this comment for copy_type_recursive:
>
> /* Recursively copy (deep copy) TYPE, if it is associated with
>    OBJFILE.  Return a new type allocated using malloc, a saved type if
>    we have already visited TYPE (using COPIED_TYPES), or TYPE if it is
>    not associated with OBJFILE.  */
>
> Note the "Return a new type using malloc"
>
> This comment is lacking: it would be really nice if it explained
> *why* the new type is saved with malloc. This critical feature of this
> routine is a bit subtle given the name is just "copy_type_recursive".
> Or better yet change the visible (exported) name of the function to
> "preserve_type", or some such just as its callers are named preserve_foo,
> rename copy_type_recursive to preserve_type_recursive, make it static,
> and have the former call the latter. [The _recursive in the name isn't really
> something callers care about. If one feels it's important to include
> this aspect in the name how about something like preserve_type_deep?]
>
> To make a long story short, I'm guessing that's the history behind why
> alloc_type_arch used malloc (I know there's discussion of this
> in the thread).
>
> At the least, we'll need to update copy_type_recursive's function
> comment and change the malloc reference.

Good points...  I'll post a patch that removes the malloc reference in
the documentation for copy_type_recursive.

Some background for this change: The TYPE_OBJFILE_OWNED macro tells us
who owns a given type, and according to the macro's documentation a
type is always owned either by an objfile or by a gdbarch.  Given this
binary encoding of ownership it doesn't seem to make much sense for
_any_ type to be allocated by malloc.  All types should be allocated
on an objfile obstack or on a gdbarch obstack.

To support allocating a type by malloc, I think the type ownership
scheme should have three possible cases: that a type is owned by an
objfile, or that it's owned by a gdbarch, or that it's owned by the
caller.  This new last case could correspond to a type that's
allocated by malloc instead of on an obstack.

Does this make sense, or maybe I am misunderstanding what "owning" means here?

Re: [PATCH 1/2] Use gdbarch obstack to allocate types in alloc_type_arch

[Doug Evans]

On Sat, Aug 29, 2015 at 2:26 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>...
> Some background for this change: The TYPE_OBJFILE_OWNED macro tells us
> who owns a given type, and according to the macro's documentation a
> type is always owned either by an objfile or by a gdbarch.  Given this
> binary encoding of ownership it doesn't seem to make much sense for
> _any_ type to be allocated by malloc.  All types should be allocated
> on an objfile obstack or on a gdbarch obstack.

I can imagine types being allocated by malloc and tracked via whatever
object "owns" them. All that matters is that when the owner is freed
all its owned objects are freed too. Whether those objects are
malloc'd or in an obstack is just an implementation detail.

I know that that's not how arch-owned types worked prior to your patch.
I'm just saying they *could* have worked that way.
Moving them to an obstack obviates the need for keeping a copy of
the pointer so it can later be freed.

> To support allocating a type by malloc, I think the type ownership
> scheme should have three possible cases: that a type is owned by an
> objfile, or that it's owned by a gdbarch, or that it's owned by the
> caller.  This new last case could correspond to a type that's
> allocated by malloc instead of on an obstack.
>
> Does this make sense, or maybe I am misunderstanding what "owning" means here?

I think you've got it right.

There's still, technically, an open issue of "What happens if an arch
is freed and there's still a value in the history that uses that type?
Would we then have to preserve that type again? (bleah)"

I don't have too strong an opinion on what's right here.
If someone wants to allow for arches being freed and thus
"preserved" types have to be "owned" by something else, ok,
but I don't see it as an urgent need. We *could* just say that
arches are never freed for now.

Re: [PATCH 1/2] Use gdbarch obstack to allocate types in alloc_type_arch

[Patrick Palka]

On Sat, Aug 29, 2015 at 5:35 PM, Doug Evans <xdje42@gmail.com> wrote:
> On Sat, Aug 29, 2015 at 2:26 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>>...
>> Some background for this change: The TYPE_OBJFILE_OWNED macro tells us
>> who owns a given type, and according to the macro's documentation a
>> type is always owned either by an objfile or by a gdbarch.  Given this
>> binary encoding of ownership it doesn't seem to make much sense for
>> _any_ type to be allocated by malloc.  All types should be allocated
>> on an objfile obstack or on a gdbarch obstack.
>
> I can imagine types being allocated by malloc and tracked via whatever
> object "owns" them. All that matters is that when the owner is freed
> all its owned objects are freed too. Whether those objects are
> malloc'd or in an obstack is just an implementation detail.
>
> I know that that's not how arch-owned types worked prior to your patch.
> I'm just saying they *could* have worked that way.
> Moving them to an obstack obviates the need for keeping a copy of
> the pointer so it can later be freed.

That makes sense.

>
>> To support allocating a type by malloc, I think the type ownership
>> scheme should have three possible cases: that a type is owned by an
>> objfile, or that it's owned by a gdbarch, or that it's owned by the
>> caller.  This new last case could correspond to a type that's
>> allocated by malloc instead of on an obstack.
>>
>> Does this make sense, or maybe I am misunderstanding what "owning" means here?
>
> I think you've got it right.
>
> There's still, technically, an open issue of "What happens if an arch
> is freed and there's still a value in the history that uses that type?
> Would we then have to preserve that type again? (bleah)"

Ah, yeah..

>
> I don't have too strong an opinion on what's right here.
> If someone wants to allow for arches being freed and thus
> "preserved" types have to be "owned" by something else, ok,
> but I don't see it as an urgent need. We *could* just say that
> arches are never freed for now.

Makes sense.

I reverted this patch, with a revised one incoming.

Re: [PATCH 1/2] Use gdbarch obstack to allocate types in alloc_type_arch

[Patrick Palka]

On Sat, Aug 29, 2015 at 6:29 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Sat, Aug 29, 2015 at 5:35 PM, Doug Evans <xdje42@gmail.com> wrote:
>> On Sat, Aug 29, 2015 at 2:26 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>>>...
>>> Some background for this change: The TYPE_OBJFILE_OWNED macro tells us
>>> who owns a given type, and according to the macro's documentation a
>>> type is always owned either by an objfile or by a gdbarch.  Given this
>>> binary encoding of ownership it doesn't seem to make much sense for
>>> _any_ type to be allocated by malloc.  All types should be allocated
>>> on an objfile obstack or on a gdbarch obstack.
>>
>> I can imagine types being allocated by malloc and tracked via whatever
>> object "owns" them. All that matters is that when the owner is freed
>> all its owned objects are freed too. Whether those objects are
>> malloc'd or in an obstack is just an implementation detail.
>>
>> I know that that's not how arch-owned types worked prior to your patch.
>> I'm just saying they *could* have worked that way.
>> Moving them to an obstack obviates the need for keeping a copy of
>> the pointer so it can later be freed.
>
> That makes sense.
>
>>
>>> To support allocating a type by malloc, I think the type ownership
>>> scheme should have three possible cases: that a type is owned by an
>>> objfile, or that it's owned by a gdbarch, or that it's owned by the
>>> caller.  This new last case could correspond to a type that's
>>> allocated by malloc instead of on an obstack.
>>>
>>> Does this make sense, or maybe I am misunderstanding what "owning" means here?
>>
>> I think you've got it right.
>>
>> There's still, technically, an open issue of "What happens if an arch
>> is freed and there's still a value in the history that uses that type?
>> Would we then have to preserve that type again? (bleah)"
>
> Ah, yeah..
>
>>
>> I don't have too strong an opinion on what's right here.
>> If someone wants to allow for arches being freed and thus
>> "preserved" types have to be "owned" by something else, ok,
>> but I don't see it as an urgent need. We *could* just say that
>> arches are never freed for now.
>
> Makes sense.
>
> I reverted this patch, with a revised one incoming.

Er, sorry, I reverted and revised the 2nd patch, not this one.