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

  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(-)
  

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.
  

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?
  

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.
  

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.
  

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.