From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14915 invoked by alias); 7 Jan 2009 08:35:07 -0000 Received: (qmail 14777 invoked by uid 22791); 7 Jan 2009 08:35:06 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 07 Jan 2009 08:34:59 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 150882A95D7; Wed, 7 Jan 2009 03:34:57 -0500 (EST) 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 de29+rrSZavb; Wed, 7 Jan 2009 03:34:57 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 2B3E52A95CD; Wed, 7 Jan 2009 03:34:56 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id E3436E7ACD; Wed, 7 Jan 2009 12:34:48 +0400 (RET) Date: Wed, 07 Jan 2009 08:35:00 -0000 From: Joel Brobecker To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: RFA: fix PR 9350 Message-ID: <20090107083448.GJ3664@adacore.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.2i 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/msg00073.txt.bz2 Hi Tom, > This patch fixes PR 9350, a memory leak in gdb. [...] > I think most of the fixes are pretty obvious in context. They are > largely cases of forgetting to run cleanups. This is actually something that I learnt only relatively recently, maybe a year or two ago: If you put something on the cleanup queue, you should perform the cleanup when you're done, or you run the risk of having a memory leak. But the only case so far where this happens in the following scenario: function_b () { temp_memory = xmalloc (...); make_cleanup (xfree, temp_memory); [...] // Function returns without having performed the cleanup } function_a () { permanent_memory = xmalloc (...); old_chain = make_cleanup (xfree, permanent_memory); [...] if (everything_ok) discard_cleanups (old_chain); else do_cleanups (old_chain); } Do we have a different scenario in your example that causes a memory leak? > @@ -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; Ooops, does it look like you're using uiout after it has been deleted? (I have seen the same issue a few more time later in your patch) Perhaps this function would benefit from having only one place where the result is returned, thus requiring only one call to do_cleanups? At first sight, it seems relatively easy to achieve in this case. That's an open question - I'm fine with just fixing the above by moving the do_cleanups to just before the return. > @@ -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); Would you mind adding an empty line after the declaration of variable expr? I have to admit that I am not sure whether this is a written rule of the GDB coding style, but Mark often commented on this, and I think it helps structuring the code a bit. [I think we should have a place in our documentation where this is explicitly spelled out - it could be our Wiki, or gdbint. But that's a separate thread]. The rest looked good to me. -- Joel