From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10809 invoked by alias); 7 Jan 2009 01:08:42 -0000 Received: (qmail 10799 invoked by uid 22791); 7 Jan 2009 01:08:40 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,J_CHICKENPOX_44,KAM_MX,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mx2.redhat.com (HELO mx2.redhat.com) (66.187.237.31) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 07 Jan 2009 01:08:32 +0000 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n0718UoO004080 for ; Tue, 6 Jan 2009 20:08:30 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx2.corp.redhat.com (8.13.1/8.13.1) with ESMTP id n0718U01009131; Tue, 6 Jan 2009 20:08:30 -0500 Received: from opsy.redhat.com (vpn-12-46.rdu.redhat.com [10.11.12.46]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id n0718TZR012692; Tue, 6 Jan 2009 20:08:29 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id 215E6378095; Tue, 6 Jan 2009 18:08:28 -0700 (MST) To: gdb-patches@sourceware.org Subject: RFA: fix PR 9350 From: Tom Tromey Reply-To: tromey@redhat.com Date: Wed, 07 Jan 2009 01:08:00 -0000 Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-01/txt/msg00066.txt.bz2 This patch fixes PR 9350, a memory leak in gdb. I ran the test case from the PR under valgrind and then fixed all the fixable memory leaks (valgrind reports a bunch of stuff from Python, plus a non-serious leak when registering a gdb command). I think most of the fixes are pretty obvious in context. They are largely cases of forgetting to run cleanups. Built and regtested on x86-64 (compile farm). Please review. thanks, Tom 2009-01-06 Tom Tromey PR breakpoints/9350: * varobj.c (varobj_invalidate): Unconditionally free all_rootvarobj. * symfile.c (syms_from_objfile): Free local_addr when returning normally. * exec.c (exec_file_attach): Do cleanups before returning. (exec_file_command): Likewise. * corefile.c (reopen_exec_file): Do cleanups before returning. * breakpoint.c (insert_breakpoint_locations): Do cleanups before returning. (do_vec_free): New function. (update_global_location_list): Make a cleanup for old_locations. Do cleanups before returning. Remove unused variable 'e'. (find_condition_and_thread): Free result of parsing the expression. (print_it_typical): Do cleanups before returning. (breakpoint_re_set_one): Always free sals.sals. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 2e25490..fbd6159 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -1282,7 +1282,7 @@ insert_breakpoint_locations (void) int process_warning = 0; struct ui_file *tmp_error_stream = mem_fileopen (); - make_cleanup_ui_file_delete (tmp_error_stream); + struct cleanup *cleanups = make_cleanup_ui_file_delete (tmp_error_stream); /* Explicitly mark the warning -- this will only be printed if there was an error. */ @@ -1360,6 +1360,8 @@ You may have requested too many hardware breakpoints/watchpoints.\n"); target_terminal_ours_for_output (); error_stream (tmp_error_stream); } + + do_cleanups (cleanups); } int @@ -2241,13 +2243,12 @@ watchpoint_value_print (struct value *val, struct ui_file *stream) static enum print_stop_action print_it_typical (bpstat bs) { - struct cleanup *old_chain, *ui_out_chain; + struct cleanup *old_chain; struct breakpoint *b; const struct bp_location *bl; struct ui_stream *stb; int bp_temp = 0; - stb = ui_out_stream_new (uiout); - old_chain = make_cleanup_ui_out_stream_delete (stb); + /* bs->breakpoint_at can be NULL if it was a momentary breakpoint which has since been deleted. */ if (bs->breakpoint_at == NULL) @@ -2255,6 +2256,9 @@ print_it_typical (bpstat bs) bl = bs->breakpoint_at; b = bl->owner; + stb = ui_out_stream_new (uiout); + old_chain = make_cleanup_ui_out_stream_delete (stb); + switch (b->type) { case bp_breakpoint: @@ -2277,6 +2281,7 @@ print_it_typical (bpstat bs) } ui_out_field_int (uiout, "bkptno", b->number); ui_out_text (uiout, ", "); + do_cleanups (old_chain); return PRINT_SRC_AND_LOC; break; @@ -2285,6 +2290,7 @@ print_it_typical (bpstat bs) variable? (If so, we report this as a generic, "Stopped due to shlib event" message.) */ printf_filtered (_("Stopped due to shared library event\n")); + do_cleanups (old_chain); return PRINT_NOTHING; break; @@ -2292,12 +2298,14 @@ print_it_typical (bpstat bs) /* Not sure how we will get here. GDB should not stop for these breakpoints. */ printf_filtered (_("Thread Event Breakpoint: gdb should not stop!\n")); + do_cleanups (old_chain); return PRINT_NOTHING; break; case bp_overlay_event: /* By analogy with the thread event, GDB should not stop for these. */ printf_filtered (_("Overlay Event Breakpoint: gdb should not stop!\n")); + do_cleanups (old_chain); return PRINT_NOTHING; break; @@ -2309,14 +2317,14 @@ print_it_typical (bpstat bs) (uiout, "reason", async_reason_lookup (EXEC_ASYNC_WATCHPOINT_TRIGGER)); mention (b); - ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "value"); + make_cleanup_ui_out_tuple_begin_end (uiout, "value"); ui_out_text (uiout, "\nOld value = "); watchpoint_value_print (bs->old_val, stb->stream); ui_out_field_stream (uiout, "old", stb); ui_out_text (uiout, "\nNew value = "); watchpoint_value_print (b->val, stb->stream); ui_out_field_stream (uiout, "new", stb); - do_cleanups (ui_out_chain); + do_cleanups (old_chain); ui_out_text (uiout, "\n"); /* More than one watchpoint may have been triggered. */ return PRINT_UNKNOWN; @@ -2328,11 +2336,11 @@ print_it_typical (bpstat bs) (uiout, "reason", async_reason_lookup (EXEC_ASYNC_READ_WATCHPOINT_TRIGGER)); mention (b); - ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "value"); + make_cleanup_ui_out_tuple_begin_end (uiout, "value"); ui_out_text (uiout, "\nValue = "); watchpoint_value_print (b->val, stb->stream); ui_out_field_stream (uiout, "value", stb); - do_cleanups (ui_out_chain); + do_cleanups (old_chain); ui_out_text (uiout, "\n"); return PRINT_UNKNOWN; break; @@ -2346,7 +2354,7 @@ print_it_typical (bpstat bs) (uiout, "reason", async_reason_lookup (EXEC_ASYNC_ACCESS_WATCHPOINT_TRIGGER)); mention (b); - ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "value"); + make_cleanup_ui_out_tuple_begin_end (uiout, "value"); ui_out_text (uiout, "\nOld value = "); watchpoint_value_print (bs->old_val, stb->stream); ui_out_field_stream (uiout, "old", stb); @@ -2359,12 +2367,12 @@ print_it_typical (bpstat bs) ui_out_field_string (uiout, "reason", async_reason_lookup (EXEC_ASYNC_ACCESS_WATCHPOINT_TRIGGER)); - ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "value"); + make_cleanup_ui_out_tuple_begin_end (uiout, "value"); ui_out_text (uiout, "\nValue = "); } watchpoint_value_print (b->val, stb->stream); ui_out_field_stream (uiout, "new", stb); - do_cleanups (ui_out_chain); + do_cleanups (old_chain); ui_out_text (uiout, "\n"); return PRINT_UNKNOWN; break; @@ -2377,6 +2385,7 @@ print_it_typical (bpstat bs) ui_out_field_string (uiout, "reason", async_reason_lookup (EXEC_ASYNC_FUNCTION_FINISHED)); + do_cleanups (old_chain); return PRINT_UNKNOWN; break; @@ -2385,6 +2394,7 @@ print_it_typical (bpstat bs) ui_out_field_string (uiout, "reason", async_reason_lookup (EXEC_ASYNC_LOCATION_REACHED)); + do_cleanups (old_chain); return PRINT_UNKNOWN; break; @@ -2395,6 +2405,7 @@ print_it_typical (bpstat bs) case bp_watchpoint_scope: case bp_call_dummy: default: + do_cleanups (old_chain); return PRINT_UNKNOWN; } } @@ -5441,8 +5452,10 @@ find_condition_and_thread (char *tok, CORE_ADDR pc, if (toklen >= 1 && strncmp (tok, "if", toklen) == 0) { + struct expression *expr; tok = cond_start = end_tok + 1; - parse_exp_1 (&tok, block_for_pc (pc), 0); + expr = parse_exp_1 (&tok, block_for_pc (pc), 0); + xfree (expr); cond_end = tok; *cond_string = savestring (cond_start, cond_end - cond_start); @@ -6855,6 +6868,16 @@ breakpoint_auto_delete (bpstat bs) } } +/* A cleanup function which destroys a vector. */ + +static void +do_vec_free (void *p) +{ + VEC(bp_location_p) **vec = p; + if (*vec) + VEC_free (bp_location_p, *vec); +} + /* If SHOULD_INSERT is false, do not insert any breakpoint locations into the inferior, only remove already-inserted locations that no longer should be inserted. Functions that delete a breakpoint or @@ -6877,11 +6900,12 @@ update_global_location_list (int should_insert) struct bp_location **next = &bp_location_chain; struct bp_location *loc; struct bp_location *loc2; - struct gdb_exception e; VEC(bp_location_p) *old_locations = NULL; int ret; int ix; - + struct cleanup *cleanups; + + cleanups = make_cleanup (do_vec_free, &old_locations); /* Store old locations for future reference. */ for (loc = bp_location_chain; loc; loc = loc->global_next) VEC_safe_push (bp_location_p, old_locations, loc); @@ -7010,6 +7034,8 @@ update_global_location_list (int should_insert) || (gdbarch_has_global_solist (target_gdbarch) && target_supports_multi_process ()))) insert_breakpoint_locations (); + + do_cleanups (cleanups); } void @@ -7369,7 +7395,7 @@ breakpoint_re_set_one (void *bint) char *s; enum enable_state save_enable; struct gdb_exception e; - + struct cleanup *cleanups; switch (b->type) { @@ -7439,9 +7465,9 @@ breakpoint_re_set_one (void *bint) b->condition_not_parsed = 0; } expanded = expand_line_sal_maybe (sals.sals[0]); + cleanups = make_cleanup (xfree, sals.sals); update_breakpoint_locations (b, expanded); - - xfree (sals.sals); + do_cleanups (cleanups); break; case bp_watchpoint: diff --git a/gdb/corefile.c b/gdb/corefile.c index 2566f9f..57a0cdf 100644 --- a/gdb/corefile.c +++ b/gdb/corefile.c @@ -153,6 +153,7 @@ reopen_exec_file (void) int res; struct stat st; long mtime; + struct cleanup *cleanups; /* Don't do anything if there isn't an exec file. */ if (exec_bfd == NULL) @@ -160,7 +161,7 @@ reopen_exec_file (void) /* If the timestamp of the exec file has changed, reopen it. */ filename = xstrdup (bfd_get_filename (exec_bfd)); - make_cleanup (xfree, filename); + cleanups = make_cleanup (xfree, filename); res = stat (filename, &st); if (exec_bfd_mtime && exec_bfd_mtime != st.st_mtime) @@ -170,6 +171,8 @@ reopen_exec_file (void) this stops GDB from holding the executable open after it exits. */ bfd_cache_close_all (); + + do_cleanups (cleanups); #endif } diff --git a/gdb/exec.c b/gdb/exec.c index 542af0e..8d8c1df 100644 --- a/gdb/exec.c +++ b/gdb/exec.c @@ -194,6 +194,7 @@ exec_file_attach (char *filename, int from_tty) } else { + struct cleanup *cleanups; char *scratch_pathname; int scratch_chan; @@ -228,7 +229,7 @@ exec_file_attach (char *filename, int from_tty) via the exec_bfd->name pointer, so we need to make another copy and leave exec_bfd as the new owner of the original copy. */ scratch_pathname = xstrdup (scratch_pathname); - make_cleanup (xfree, scratch_pathname); + cleanups = make_cleanup (xfree, scratch_pathname); if (!bfd_check_format (exec_bfd, bfd_object)) { @@ -276,6 +277,8 @@ exec_file_attach (char *filename, int from_tty) /* Tell display code (if any) about the changed file name. */ if (deprecated_exec_file_display_hook) (*deprecated_exec_file_display_hook) (filename); + + do_cleanups (cleanups); } bfd_cache_close_all (); observer_notify_executable_changed (); @@ -302,11 +305,13 @@ exec_file_command (char *args, int from_tty) if (args) { + struct cleanup *cleanups; + /* Scan through the args and pick up the first non option arg as the filename. */ argv = gdb_buildargv (args); - make_cleanup_freeargv (argv); + cleanups = make_cleanup_freeargv (argv); for (; (*argv != NULL) && (**argv == '-'); argv++) {; @@ -317,6 +322,8 @@ exec_file_command (char *args, int from_tty) filename = tilde_expand (*argv); make_cleanup (xfree, filename); exec_file_attach (filename, from_tty); + + do_cleanups (cleanups); } else exec_file_attach (NULL, from_tty); diff --git a/gdb/symfile.c b/gdb/symfile.c index 14cb7b8..21328b8 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -899,6 +899,7 @@ syms_from_objfile (struct objfile *objfile, /* Discard cleanups as symbol reading was successful. */ discard_cleanups (old_chain); + xfree (local_addr); } /* Perform required actions after either reading in the initial diff --git a/gdb/value.c b/gdb/value.c index 39df98e..1068f1d 100644 --- a/gdb/value.c +++ b/gdb/value.c @@ -931,7 +931,7 @@ set_internalvar (struct internalvar *var, struct value *val) something in the value chain (i.e., before release_value is called), because after the error free_all_values will get called before long. */ - xfree (var->value); + value_free (var->value); var->value = newval; var->endian = gdbarch_byte_order (current_gdbarch); release_value (newval); diff --git a/gdb/varobj.c b/gdb/varobj.c index 25cc207..5b2ed9c 100644 --- a/gdb/varobj.c +++ b/gdb/varobj.c @@ -2780,7 +2780,7 @@ varobj_invalidate (void) varp++; } - xfree (all_rootvarobj); } + xfree (all_rootvarobj); return; }