Message ID | 1431100524-7793-1-git-send-email-brobecker@adacore.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 70497 invoked by alias); 8 May 2015 15:55:37 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 70487 invoked by uid 89); 8 May 2015 15:55:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL, BAYES_00, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 08 May 2015 15:55:36 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id CDD74287DD for <gdb-patches@sourceware.org>; Fri, 8 May 2015 11:55:34 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id Bqkgovw++wrL for <gdb-patches@sourceware.org>; Fri, 8 May 2015 11:55:34 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id A0B9128790 for <gdb-patches@sourceware.org>; Fri, 8 May 2015 11:55:34 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 27E7840DAA; Fri, 8 May 2015 08:55:34 -0700 (PDT) From: Joel Brobecker <brobecker@adacore.com> To: gdb-patches@sourceware.org Subject: [RFA/commit] Memory leak in on reading frame register Date: Fri, 8 May 2015 08:55:24 -0700 Message-Id: <1431100524-7793-1-git-send-email-brobecker@adacore.com> |
Commit Message
Joel Brobecker
May 8, 2015, 3:55 p.m. UTC
[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,
Comments
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
> > 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?
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
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;