This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Variable objects laziness
- From: Daniel Jacobowitz <drow at false dot org>
- To: Vladimir Prus <vladimir at codesourcery dot com>
- Cc: gdb-patches at sources dot redhat dot com
- Date: Tue, 14 Nov 2006 15:47:21 -0500
- Subject: Re: Variable objects laziness
- References: <200611141643.25053.vladimir@codesourcery.com>
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