This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA/commit] Memory leak in on reading frame register
- From: Doug Evans <dje at google dot com>
- To: Joel Brobecker <brobecker at adacore dot com>
- Cc: Pedro Alves <palves at redhat dot com>, gdb-patches <gdb-patches at sourceware dot org>, Jerome Guitton <guitton at adacore dot com>
- Date: Fri, 15 May 2015 15:35:10 -0700
- Subject: Re: [RFA/commit] Memory leak in on reading frame register
- Authentication-results: sourceware.org; auth=none
- References: <1431100524-7793-1-git-send-email-brobecker at adacore dot com> <55508A83 dot 3060605 at redhat dot com> <20150511205312 dot GE4767 at adacore dot com> <5551CB20 dot 4090104 at redhat dot com> <20150515155823 dot GL4767 at adacore dot 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.