This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]