This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: RFA: fix PR 9350
- From: Tom Tromey <tromey at redhat dot com>
- To: Joel Brobecker <brobecker at adacore dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Wed, 07 Jan 2009 07:48:32 -0700
- Subject: Re: RFA: fix PR 9350
- References: <m3sknv7wj8.fsf@fleche.redhat.com> <20090107083448.GJ3664@adacore.com>
- Reply-to: Tom Tromey <tromey at redhat dot com>
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
Joel> This is actually something that I learnt only relatively recently,
Joel> maybe a year or two ago: If you put something on the cleanup queue,
Joel> you should perform the cleanup when you're done, or you run
Joel> the risk of having a memory leak.
Yeah. Yesterday I was contemplating writing a gcc plugin to detect
this error. But back to reality...
Joel> Do we have a different scenario in your example that causes
Joel> a memory leak?
Yeah, this patch reveals a number of leak styles.
In syms_from_objfile, we installed a cleanup but then discarded it.
This is another cleanup oddity -- because they are handled linearly,
code must either be careful to create them in the right order so that
a sequence of discard_ and do_ calls can be run at the end; or the
code must duplicate the action.
In update_global_location_list, we simply were not installing any
cleanup for the local VEC.
In varobj_invalidate, the freeing was only done on one branch of an
`if', though the condition of the `if' unconditionally allocated
memory.
>> - do_cleanups (ui_out_chain);
>> + do_cleanups (old_chain);
Joel> Ooops, does it look like you're using uiout after it has been
Joel> deleted? (I have seen the same issue a few more time later
Joel> in your patch)
No, it just looks that way because ui_out_chain had a funny name.
The old code looked like:
- ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "value");
So ui_out_chain was just used for finalizing a tuple.
`uiout' itself is still valid; the additional cleanup we run (via
`old_chain') is to finalize `stb'.
Joel> Perhaps this function would benefit from having only one place
Joel> where the result is returned, thus requiring only one call to
Joel> do_cleanups? At first sight, it seems relatively easy to achieve
Joel> in this case. That's an open question - I'm fine with just fixing
Joel> the above by moving the do_cleanups to just before the return.
I only did it this way because it was the prevailing style in the
function. I will switch it and retest and resubmit.
Tom