From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31238 invoked by alias); 15 Nov 2006 09:22:54 -0000 Received: (qmail 31227 invoked by uid 22791); 15 Nov 2006 09:22:54 -0000 X-Spam-Check-By: sourceware.org Received: from main.gmane.org (HELO ciao.gmane.org) (80.91.229.2) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 15 Nov 2006 09:22:47 +0000 Received: from list by ciao.gmane.org with local (Exim 4.43) id 1GkGyh-0005M4-Oe for gdb-patches@sources.redhat.com; Wed, 15 Nov 2006 10:22:43 +0100 Received: from 73-198.umostel.ru ([82.179.73.198]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Wed, 15 Nov 2006 10:22:43 +0100 Received: from ghost by 73-198.umostel.ru with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Wed, 15 Nov 2006 10:22:43 +0100 To: gdb-patches@sources.redhat.com From: Vladimir Prus Subject: Re: Variable objects laziness Date: Wed, 15 Nov 2006 09:22:00 -0000 Message-ID: References: <17754.32341.141757.120973@kahikatea.snap.net.nz> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7Bit User-Agent: KNode/0.10.2 X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-11/txt/msg00117.txt.bz2 Nick Roberts wrote: > Index: varobj.c > =================================================================== > RCS file: /cvs/src/src/gdb/varobj.c,v > retrieving revision 1.60 > diff -u -p -r1.60 varobj.c > --- varobj.c 3 May 2006 22:59:38 -0000 1.60 > +++ varobj.c 14 Nov 2006 13:38:35 -0000 > @@ -101,7 +101,9 @@ struct varobj > /* The type of this variable. This may NEVER be NULL. */ > struct type *type; > > - /* The value of this expression or subexpression. This may be NULL. */ > + /* The value of this expression or subexpression. This may be NULL. > + Invariant: if type_changeable (this) is non-zero, the value is > either > + NULL, or not lazy. */ > > > I don't understand the replacement comment Do you don't understand the comment proper, or high-level meaning of it? The comment itself says that after each varobj-related function exits, if type_changeable for some varobj returns true, the the varobj must be either NULL or non-lazy. The higher-level implication is that type_changeable indicates if the values of this varobjs will be compared by -var-update. If it will be compared, then the values may not be lazy. Otherwise, after we've made the step and call -var-update, we don't have the old value in gdb's memory, and it might have changed in target's memory, so we've lost the old value. Does this clarifies things? > > ... > > +/** Assign new value to a variable object. If INITIAL is non-zero, > + this is first assignement after the variable object was just > + created, or changed type. In that case, just assign the value > + and return 0. > + Otherwise, assign the value and if type_changeable returns non-zero, > + find if the new value is different from the current value. > + Return 1 if so, and 0 is the values are equal. */ > +static int > +install_new_value (struct varobj *var, struct value *value, int initial) > +{ > > I don't understand this comment either. INITIAL can be type_changed > i.e not really initial. Sorry, I don't understand. Do you refer to the fact that somewhere else, a variable 'type_changed' is passed as INITIAL parameter? That's correct, because if the type changed, gdb basically recreated varobj afresh. Assignment to that recreated varobj is indeed initial assignment -- you should not compare new value with any old value. > Can you give it more structure and not refer to > internals like type_changeable? Why do you call type_changeable "internals"? Just like install_new_value, it's static function in varobj.c -- it's no more internal than install_new_value itself. It is important aspect of install_new value that it consults type_changeable. To maintain invariant of struct varobj, install_new_value *must* check type_changeable. > ... > > + if (CPLUS_FAKE_CHILD (var)) > + changeable = 0; > + else > + changeable = type_changeable (var); > > type_changeable returns 0 if CPLUS_FAKE_CHILD (var) is true anyway so do > you need this clause? Ah, right. Changed. > As a whole the patch seems to lack clarity (although that might partly be > a reflection on my abilities!) Maybe I can try explaining it from a different angle? The -var-update command must compare old values and new values for some variable objects -- namely those for which type_changeable returns true. In order to do this, old value of such variable objects must not be lazy. At the same time, there's no point to fetch values of other variable objects. So, this leads to invariant I've documented. Present code has a number of calls to value_fetch_lazy. It's troublesome to even examine all those places. So, there's new "install_new_value" function that's the only place where new value of varobj is assigned, and the only place where value_fetch_lazy is called. That function also takes care about maintaining invariant. > (type_of_child): Remove. > (varobj_create): Use install_new_value. > (varobj_set_value): Use value_contents_equal, not > my_value_equal. > > Previously someone (Mark Kettenis?) has gone to a lot of trouble to > replace value_contents_equal with my_value_equal why do you think it's not > needed? Because now install_new_value makes sure that var->value is not lazy when it needs be not lazy. At the same time, install_new_value is handling making new value non-lazy. So essentially, it's guaranteed that both arguments to value_contents_equal are not lazy. > * varobj.c (struct varobj): Clarify comment. > (my_value_equal): Remove. > (install_new_value): New. > > New function presumably. You mean, "New function." as comment in ChangeLog? OK. > I'm having trouble to understand this patch. If it does more than one > thing perhaps you can break it into two patches. I don't think I see any reasonable breakdown. This patch at the same time introduces new invariant, new function that keeps it, and makes all code use the new function. Breaking this apart will mean you have new function that is not used everywhere and so basically useless. - Volodya