From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31001 invoked by alias); 23 Jan 2007 08:56:06 -0000 Received: (qmail 30992 invoked by uid 22791); 23 Jan 2007 08:56:05 -0000 X-Spam-Check-By: sourceware.org Received: from viper.snap.net.nz (HELO viper.snap.net.nz) (202.37.101.8) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 23 Jan 2007 08:55:55 +0000 Received: from kahikatea.snap.net.nz (30.61.255.123.dynamic.snap.net.nz [123.255.61.30]) by viper.snap.net.nz (Postfix) with ESMTP id 147C93D841E; Tue, 23 Jan 2007 21:55:47 +1300 (NZDT) Received: by kahikatea.snap.net.nz (Postfix, from userid 500) id A5BC84F720; Tue, 23 Jan 2007 21:55:46 +1300 (NZDT) From: Nick Roberts MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <17845.52624.949137.508231@kahikatea.snap.net.nz> Date: Tue, 23 Jan 2007 08:56:00 -0000 To: Vladimir Prus Cc: gdb-patches@sources.redhat.com Subject: Re: [PATCH] MI: Free values when updating In-Reply-To: References: <17845.48393.877158.536969@kahikatea.snap.net.nz> X-Mailer: VM 7.19 under Emacs 22.0.92.14 X-IsSubscribed: yes 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/msg00462.txt.bz2 > 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; 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. Then try the patch below which does pass all the tests (just intended for illustration not approval). > 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. -- Nick http://www.inet.net.nz/~nickrob *** varobj.c 16 Jan 2007 18:34:59 +1300 1.79 --- varobj.c 23 Jan 2007 21:24:17 +1300 *************** install_new_value (struct varobj *var, s *** 917,923 **** /* We are not interested in the address of references, and given that in C++ a reference is not rebindable, it cannot meaningfully change. So, get hold of the real value. */ ! if (value) { value = coerce_ref (value); release_value (value); --- 917,927 ---- /* We are not interested in the address of references, and given that in C++ a reference is not rebindable, it cannot meaningfully change. So, get hold of the real value. */ ! if (initial == 2) ! { ! initial = 0; ! } ! else if (value) { value = coerce_ref (value); release_value (value); *************** varobj_update (struct varobj **varp, str *** 1117,1123 **** if (v != *varp) { new = value_of_child (v->parent, v->index); ! if (install_new_value (v, new, 0 /* type not changed */)) { /* Note that it's changed */ VEC_safe_push (varobj_p, result, v); --- 1121,1127 ---- if (v != *varp) { new = value_of_child (v->parent, v->index); ! if (install_new_value (v, new, 2 /* type not changed */)) { /* Note that it's changed */ VEC_safe_push (varobj_p, result, v);