Re: [RFA/commit] Memory leak in on reading frame register

[Joel Brobecker <brobecker at adacore dot com>]

> > When using a conditional breakpoint where the condition evaluated
> > to false a large number of times before the program stopped,
> > a user reported that GDB's memory consumption was growing very
> > quickly until it ran out of memory.
> > 
> > The problem was tracked down to temporary struct values being created
> > each time the program stops and we evaluate those conditions. This
> > patch fixes the issue by releasing the temporary values, and adds
> > a comment explaining why we do that.
> > 
> > gdb/ChangeLog:
> > 
> >         Jerome Guitton  <guitton@adacore.com>:
> > 	* findvar.c (read_frame_register_value): Fix a memory leak.
> > 
> > Tested on x86_64-linux. No regression.
> > 
> 
> Not sure about this.
> 
> How come this in bpstat_check_breakpoint_conditions didn't
> handle this issue already? :
> 
> ...
>       /* We use value_mark and value_free_to_mark because it could
> 	 be a long time before we return to the command level and
> 	 call free_all_values.  We can't call free_all_values
> 	 because we might be in the middle of evaluating a
> 	 function call.  */
>       struct value *mark = value_mark ();
> 
> ...
>       value_free_to_mark (mark);

An excellent question, which I will try to research in the next
couple of days!

...

> Otherwise, what is releasing other kinds of temporary values?
> Are we leaking them?  E.g., with:
> 
> int global_val;
> void foo () {}
> int main () { while (1) foo (); }
> 
> and then:
> 
> (gdb) break foo if global_var == 1
> 
> an/or:
> 
> (gdb) break foo if (global_var + 1) == 2
> 
> 
> Maybe nothing breaks with this patch as its deleting register lval
> values, but the case above would involve lval_memory values,
> and if we did something for those like in this patch, I fear
> that places that want to walk an expression's value chain,
> like update_watchpoint / can_use_hardware_watchpoint would break.

But I confess I don't quite understand what you mean by the above.
Are you saying that the current patch may be OK (because we're
creating and deleting a value that we know is independent of all
other values), but that it sets a precendent for other forms where
it might not be OK?

-- 
Joel