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

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

[Joel Brobecker]

[On behalf of Jerome  Guitton]

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.

I'll push the patch in a week or so, pending comments.

Thanks,
-- 
Joel

---
 gdb/findvar.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 2079b4b..8ccf267 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -686,6 +686,17 @@ read_frame_register_value (struct value *value, struct frame_info *frame)
 
       value_contents_copy (value, offset, regval, reg_offset, reg_len);
 
+      /* Release regval right away, as we know we do not need it anymore.
+	 Otherwise, those values just keep accumulating until they finally
+	 get released when the current command finishes (as part of the
+	 all_values chain cleanup).  While this works most of the time,
+	 we have observed that, when using a conditional breakpoint where
+	 the condition gets repeatedly evaluated to false, keeping those
+	 values in memory causes a rapid and measurable growth in memory
+	 consumption, eventually leading us to running out of memory.  */
+      release_value (regval);
+      value_free (regval);
+
       offset += reg_len;
       len -= reg_len;
       reg_offset = 0;
-- 
1.9.1

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

[Pedro Alves]

On 05/08/2015 04:55 PM, Joel Brobecker wrote:
> [On behalf of Jerome  Guitton]
> 
> 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);
...


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.

Thanks,
Pedro Alves

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

[Joel Brobecker]

> > 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

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

[Pedro Alves]

On 05/11/2015 09:53 PM, Joel Brobecker wrote:
>>> 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!

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.

> 
> ...
> 
>> 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

Right, I'm saying that it may not be breaking things just because
(I think) we don't look at lval_register values in the code I
pointed out.

> 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?

Right.

Thanks,
Pedro Alves

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

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1488 bytes --]

> >> 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.

Thanks!
-- 
Joel

[-- Attachment #2: 0001-Memory-leak-reading-frame-register-during-inferior-e.patch --]
[-- Type: text/x-diff, Size: 2305 bytes --]

From 3d2f2a3967c143ba4c0c0c6c731bffd9a2cb726f Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Fri, 13 Feb 2015 11:57:29 +0100
Subject: [PATCH] Memory leak reading frame register during inferior event
 handling

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 handles an inferior event.  Because
the breakpoint condition usually evaluates to false, there can be
a fairly large number of such events to be handled before we eventually
return the prompt to the user (which is when we would normally purge
such values).

This patch fixes the issue by making sure that handle_inferior_event
releases all new values created during its execution.

gdb/ChangeLog:

        * infrun.c (handle_inferior_event_1): Renames handle_inferior_event.
        (handle_inferior_event): New function.
---
 gdb/infrun.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 71cf208..96cddbd 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3680,7 +3680,7 @@ get_inferior_stop_soon (ptid_t ptid)
    once).  */
 
 static void
-handle_inferior_event (struct execution_control_state *ecs)
+handle_inferior_event_1 (struct execution_control_state *ecs)
 {
   enum stop_kind stop_soon;
 
@@ -4202,6 +4202,23 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
     }
 }
 
+/* A wrapper around handle_inferior_event_1, which also makes sure
+   that all temporary struct value objects that were created during
+   the handling of the event get deleted at the end.  */
+
+static void
+handle_inferior_event (struct execution_control_state *ecs)
+{
+  struct value *mark = value_mark ();
+
+  handle_inferior_event_1 (ecs);
+  /* Purge all temporary values created during the event handling,
+     as it could be a long time before we return to the command level
+     where such values would otherwise be purged.  */
+  value_free_to_mark (mark);
+}
+
+
 /* Come here when the program has stopped with a signal.  */
 
 static void
-- 
1.9.1

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

[Doug Evans]

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.

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

[Joel Brobecker]

> > 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.

I guess it depends on whether you think you need the certainty of
the cleanup or not. I think both approaches are valid depending
on the context.

In this case, I asked myself that question, and I didn't see a real
need for it, since my thinking was that, if an exception occurs and
propagates through handle_inferior_event, then chances are it'll
propagate all the way, which would then lead to values being cleaned
up as well. So I went with the current pattern.

But I can change it to a cleanup if people prefer. I don't mind.

-- 
Joel

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

[Pedro Alves]

On 05/15/2015 04:58 PM, Joel Brobecker wrote:

>> > 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?

Looks good to me.

Thanks,
Pedro Alves

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

[Joel Brobecker]

> > 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?
> 
> Looks good to me.

Thank you, Pedro. Patch has been pushed to master.

-- 
Joel