[PATCH 1/2] Use gdbarch obstack to allocate types in alloc_type_arch
Sat Aug 29 21:26:00 GMT 2015
On Sat, Aug 29, 2015 at 2:05 PM, Doug Evans <email@example.com> wrote:
> Patrick Palka <firstname.lastname@example.org> 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
> 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).
> 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?
More information about the Gdb-patches