Re: RFA: fix PR 9350

[Joel Brobecker <brobecker@adacore.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