From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16875 invoked by alias); 14 Nov 2006 20:47:32 -0000 Received: (qmail 16864 invoked by uid 22791); 14 Nov 2006 20:47:31 -0000 X-Spam-Check-By: sourceware.org Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.31.1) with ESMTP; Tue, 14 Nov 2006 20:47:23 +0000 Received: from drow by nevyn.them.org with local (Exim 4.54) id 1Gk5Bh-0005zZ-NW; Tue, 14 Nov 2006 15:47:21 -0500 Date: Tue, 14 Nov 2006 20:47:00 -0000 From: Daniel Jacobowitz To: Vladimir Prus Cc: gdb-patches@sources.redhat.com Subject: Re: Variable objects laziness Message-ID: <20061114204721.GA11963@nevyn.them.org> Mail-Followup-To: Vladimir Prus , gdb-patches@sources.redhat.com References: <200611141643.25053.vladimir@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200611141643.25053.vladimir@codesourcery.com> User-Agent: Mutt/1.5.13 (2006-08-11) 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/msg00110.txt.bz2 On Tue, Nov 14, 2006 at 04:43:24PM +0300, Vladimir Prus wrote: > At the moment, MI's variable objects are too eager. For example, if you create > a variable object corresponding to a structure, gdb will read the entire > structure from the memory, even though it never does anything with that value > directly. Structure values are not compared, and you can't access them other > than by creating children variable objects. > > For example, if you have a huge structure containing two structure fields, gdb > will read all the outer structure even though you never open one of the > children structures. > > Worse, the code for fetching the values is scattered over a number of places. I think this is a good patch. I'm going to wait a few days before approving this, however, in case any of the other MI users on the list have comments. Neither before nor after this patch do I really understand what "type_changeable" means so I'll trust you've got it right :-) How does reading the children instead of the parent affect bitfields? Will we do eight reads from the target for this? struct x { unsigned char a:1, b:1, c:1, d:1, e:1, f:1, g:1, h:1; }; How about unions? > +static int > +install_new_value (struct varobj *var, struct value *value, int initial); No newline, since this is a prototype. > + value = evaluate_type (var->root->exp); > + > + var->type = value_type (value); Space/tabs mismatch. > - var->type = value_type (var->value); > + install_new_value (var, value, 1 /* Initial assignment. */); Probably don't need the period here, since this isn't supposed to be a full sentence. If you did need the period, you'd need another space after it. > + /* All types are the editable must also be changeable. */ "All types that are editable" > + /* Value of a changeable variable object must not be lazy. */ "The value of" > + /* The new value may be lazy, gdb_value_assign, or > + rather value_contents, will take care of this. */ Don't use a comma to separate what are basically independent sentences; you should use a period between "lazy" and "gdb_value_assign". > +/** Assign new value to a variable object. If INITIAL is non-zero, Javadoc? Not here :-) Article missing, "Assign a new value" or "Assign VALUE to VAR". > + this is first assignement after the variable object was just "is the first". > + /* The new value might be lazy. If the type is changeable, > + that is we'll be comparing values of this type, fetch the > + value now. Otherwise, on the next update the old value > + will be lazy, which means we've lost that old value. > + We do this for frozen varobjs as well, since this > + function is only called when it's decided that we need > + to fetch the new value of a frozen variable. */ Stray frozen varobjs comment. > + /* Set the value to NULL, so that for the next -var-update, > + we don't try to compare the new value with this value, > + that we can't even read. */ "this value that we couldn't". > + /* If the type is changeable, compare the old and the new values. > + If the type of varobj changed, no point in comparing values. */ "the type of the varobj". > + if (!initial && changeable) > + { > + /* If the value of the varobj was set by -var-set-value, then the > + value in the varobj and in the target is the same. However, that value > + is different from the value that the varobj had after the previous > + -var-update. So need to the varobj as changed. */ > + if (var->updated) > + changed = 1; "need to mark the". Why is this true? I would have thought we only needed to mark the varobj as changed if the value assigned to it was different from the previous one. Oh, I see this was what it did before. OK. > + /* We must always keep new values, since children depend on it. */ "values" is plural, "it" is singular, so this sounds strange; probably "must always keep the new value". -- Daniel Jacobowitz CodeSourcery