This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: RFA: fix PR 9350
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Tom Tromey <tromey at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Wed, 7 Jan 2009 12:34:48 +0400
- Subject: Re: RFA: fix PR 9350
- References: <m3sknv7wj8.fsf@fleche.redhat.com>
Hi Tom,
> This patch fixes PR 9350, a memory leak in gdb.
[...]
> I think most of the fixes are pretty obvious in context. They are
> largely cases of forgetting to run cleanups.
This is actually something that I learnt only relatively recently,
maybe a year or two ago: If you put something on the cleanup queue,
you should perform the cleanup when you're done, or you run
the risk of having a memory leak.
But the only case so far where this happens in the following
scenario:
function_b ()
{
temp_memory = xmalloc (...);
make_cleanup (xfree, temp_memory);
[...] // Function returns without having performed the cleanup
}
function_a ()
{
permanent_memory = xmalloc (...);
old_chain = make_cleanup (xfree, permanent_memory);
[...]
if (everything_ok)
discard_cleanups (old_chain);
else
do_cleanups (old_chain);
}
Do we have a different scenario in your example that causes
a memory leak?
> @@ -2309,14 +2317,14 @@ print_it_typical (bpstat bs)
> (uiout, "reason",
> async_reason_lookup (EXEC_ASYNC_WATCHPOINT_TRIGGER));
> mention (b);
> - ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "value");
> + make_cleanup_ui_out_tuple_begin_end (uiout, "value");
> ui_out_text (uiout, "\nOld value = ");
> watchpoint_value_print (bs->old_val, stb->stream);
> ui_out_field_stream (uiout, "old", stb);
> ui_out_text (uiout, "\nNew value = ");
> watchpoint_value_print (b->val, stb->stream);
> ui_out_field_stream (uiout, "new", stb);
> - do_cleanups (ui_out_chain);
> + do_cleanups (old_chain);
> ui_out_text (uiout, "\n");
> /* More than one watchpoint may have been triggered. */
> return PRINT_UNKNOWN;
Ooops, does it look like you're using uiout after it has been
deleted? (I have seen the same issue a few more time later
in your patch)
Perhaps this function would benefit from having only one place
where the result is returned, thus requiring only one call to
do_cleanups? At first sight, it seems relatively easy to achieve
in this case. That's an open question - I'm fine with just fixing
the above by moving the do_cleanups to just before the return.
> @@ -5441,8 +5452,10 @@ find_condition_and_thread (char *tok, CORE_ADDR pc,
>
> if (toklen >= 1 && strncmp (tok, "if", toklen) == 0)
> {
> + struct expression *expr;
> tok = cond_start = end_tok + 1;
> - parse_exp_1 (&tok, block_for_pc (pc), 0);
> + expr = parse_exp_1 (&tok, block_for_pc (pc), 0);
> + xfree (expr);
> cond_end = tok;
> *cond_string = savestring (cond_start,
> cond_end - cond_start);
Would you mind adding an empty line after the declaration of
variable expr? I have to admit that I am not sure whether this
is a written rule of the GDB coding style, but Mark often commented
on this, and I think it helps structuring the code a bit.
[I think we should have a place in our documentation where this is
explicitly spelled out - it could be our Wiki, or gdbint. But that's
a separate thread].
The rest looked good to me.
--
Joel