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

[Doug Evans <dje@google.com>]

On Fri, May 15, 2015 at 8:58 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>> >> 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!
>>
>> Thanks.  I wonder whether the leaks come from constructing the
>> current frame at each stop, instead of from evaluating
>> breakpoint conditions.  E.g.., if we do a "step" over:
>>
>>    while (1);
>>
>> ... are we constantly leaking values until the user does
>> ctrl-c?
>>
>> That would suggest to me to that we should be doing
>> value_mark/value_free_to_mark around each
>> handle_inferior_event.
>
> A very accurate guess, as it turns out. Condition evaluation
> is not the problem, here, but indeed, we a couple of calls to
> handle_inferior in addition to each call to
> bpstat_check_breakpoint_conditions. The former are responsible
> for the leak.
>
> How about the attached patch?
>
> gdb/ChangeLog:
>
>         * infrun.c (handle_inferior_event_1): Renames handle_inferior_event.
>         (handle_inferior_event): New function.
>
> Tested on x86_64-linux. No regression.

Not that this has to be changed here, but I'm wondering why all value mark/frees
aren't done via cleanups. I can imagine sometimes it's not,
technically, necessary,
and I can imagine there's some history/inertia here,
but having two ways to do this (using a cleanup or not) leaves the reader
having to wonder if using a cleanup was errantly skipped.