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

[Patrick Palka <patrick@parcs.ath.cx>]

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?