From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32426 invoked by alias); 23 Jan 2007 09:15:33 -0000 Received: (qmail 32399 invoked by uid 22791); 23 Jan 2007 09:15:30 -0000 X-Spam-Check-By: sourceware.org Received: from zigzag.lvk.cs.msu.su (HELO zigzag.lvk.cs.msu.su) (158.250.17.23) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 23 Jan 2007 09:15:26 +0000 Received: from Debian-exim by zigzag.lvk.cs.msu.su with spam-scanned (Exim 4.50) id 1H9HkM-0006zX-PM for gdb-patches@sources.redhat.com; Tue, 23 Jan 2007 12:15:23 +0300 Received: from localhost ([127.0.0.1] helo=ip6-localhost) by zigzag.lvk.cs.msu.su with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA:32) (Exim 4.50) id 1H9HkD-0006zH-6c; Tue, 23 Jan 2007 12:15:09 +0300 From: Vladimir Prus To: Nick Roberts Subject: Re: [PATCH] MI: Free values when updating Date: Tue, 23 Jan 2007 09:15:00 -0000 User-Agent: KMail/1.9.1 Cc: gdb-patches@sources.redhat.com References: <17845.48393.877158.536969@kahikatea.snap.net.nz> <17845.52624.949137.508231@kahikatea.snap.net.nz> In-Reply-To: <17845.52624.949137.508231@kahikatea.snap.net.nz> MIME-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_cIdtFyCaJ0kmQow" Message-Id: <200701231215.08114.ghost@cs.msu.su> 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: 2007-01/txt/msg00463.txt.bz2 --Boundary-00=_cIdtFyCaJ0kmQow Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Content-length: 2601 On Tuesday 23 January 2007 11:55, Nick Roberts wrote: > > I don't understand this change at all. It means that if you have > > a reference variable then: > > > > 1. For initial value, dereferenced value will be saved > > 2. For second and subsequent values, the value of reference itself will > > be saved. > > > > This is exactly what the code block in question tries to prevent -- > > it tries to make sure we always store dereferenced value. > > > > When I try to run tests with this patch applied, I get: > > > > Running ../.././gdb/testsuite/gdb.mi/mi-var-cmd.exp ... > > ERROR: Couldn't send -var-create i * i to GDB. > > ERROR: Couldn't send -var-create l * l to GDB. > > ERROR: Couldn't send -var-create linteger * linteger to GDB. > > > > and many more errors like this. Did you run the testsuite after change? > > OK, it's not the right patch but I do think that release_values shouldn't be > called when updating; Why? If we're about to store a value in varobj->value, we must call release_value on that value, otherwise the value is going to be deleted at a random point in time, and our varobj will be rather useless. > it's just that this patch stops calling it at other times > when it's needed. Without any change, do enable timings (if you have that > patch), create a variable object of a large array and all its children then > repeatedly do "-var-update *". It should take longer and longer to execute. Why? Is it because the memory consumption of gdb grows, or because the list of released values grows without ever being cleared, or for some other reason? > Then try the patch below which does pass all the tests (just intended for > illustration not approval). Could you perhaps try the attached one, which I think is ready for checkin. The install_new_value function documents that the incoming value should not be released yet, in one place that requirement was not met. No regressions on x86. > > Can you explain where the memory leak comes from. At the end of the function I see: > > > > if (var->value != NULL) > > value_free (var->value); > > > > and the value passed to function itself does not have release_value called on it, > > so should be freed automatically. > > I've not said it's a memory leak but it's computationally wasteful to > call release_value for each variable object when updating. As I've said, we need to do it to avoid the value to be deleted at random point in time. So there must be something else? Does you free_values patch makes any difference here? - Volodya --Boundary-00=_cIdtFyCaJ0kmQow Content-Type: text/x-diff; charset="iso-8859-1"; name="varobj_release_value.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="varobj_release_value.diff" Content-length: 662 Index: varobj.c =================================================================== RCS file: /cvs/src/src/gdb/varobj.c,v retrieving revision 1.79 diff -u -p -r1.79 varobj.c --- varobj.c 16 Jan 2007 02:12:49 -0000 1.79 +++ varobj.c 23 Jan 2007 09:12:07 -0000 @@ -1987,11 +1987,7 @@ c_value_of_root (struct varobj **var_han /* We need to catch errors here, because if evaluate expression fails we just want to make val->error = 1 and go on */ - if (gdb_evaluate_expression (var->root->exp, &new_val)) - { - release_value (new_val); - } - + gdb_evaluate_expression (var->root->exp, &new_val); return new_val; } --Boundary-00=_cIdtFyCaJ0kmQow--