Variable objects laziness

Vladimir Prus
2006-11-14T13:43:44Z

[-- Attachment #1: Type: text/plain, Size: 1297 bytes --]

Hello!

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.

This patch cleans up all this:

- There's just one function that decides if we must fetch the value
- The decision is consistent with the way gdb uses variable objects

No regressions on gdb.mi testsuite on x86. OK?

- Volodya

* varobj.c (struct varobj): Clarify comment.
(my_value_equal): Remove.
(install_new_value): New.
(type_of_child): Remove.
(varobj_create): Use install_new_value.
(varobj_set_value): Use value_contents_equal, not
my_value_equal.
(varobj_update): Use install_new_value.
(create_child): Likewise. Inline type_of_child here.
(value_of_child): Don't fetch the value.
(c_value_of_root): Likewise.
(c_value_of_variable): Likewise.

[-- Attachment #2: laziness.diff --]
[-- Type: text/x-diff, Size: 13160 bytes --]

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. */
struct value *value;

/* Did an error occur evaluating the expression or getting its value? */
@@ -202,8 +204,6 @@ static struct type *get_target_type (str

static enum varobj_display_formats variable_default_display (struct varobj *);

-static int my_value_equal (struct value *, struct value *, int *);
-
static void vpush (struct vstack **pstack, struct varobj *var);

static struct varobj *vpop (struct vstack **pstack);
@@ -212,6 +212,9 @@ static void cppush (struct cpstack **pst

static char *cppop (struct cpstack **pstack);

+static int
+install_new_value (struct varobj *var, struct value *value, int initial);
+
/* Language-specific routines. */

static enum varobj_languages variable_language (struct varobj *var);
@@ -226,8 +229,6 @@ static struct value *value_of_root (stru

static struct value *value_of_child (struct varobj *parent, int index);

-static struct type *type_of_child (struct varobj *var);
-
static int variable_editable (struct varobj *var);

static char *my_value_of_variable (struct varobj *var);
@@ -445,6 +446,7 @@ varobj_create (char *objname,
{
char *p;
enum varobj_languages lang;
+ struct value *value;

/* Parse and evaluate the expression, filling in as much
of the variable's data as possible */
@@ -505,17 +507,17 @@ varobj_create (char *objname,
/* We definitively need to catch errors here.
If evaluate_expression succeeds we got the value we wanted.
But if it fails, we still go on with a call to evaluate_type() */
- if (gdb_evaluate_expression (var->root->exp, &var->value))
+ if (gdb_evaluate_expression (var->root->exp, &value))
{
/* no error */
- release_value (var->value);
- if (value_lazy (var->value))
- gdb_value_fetch_lazy (var->value);
+ release_value (value);
}
else
- var->value = evaluate_type (var->root->exp);
+ value = evaluate_type (var->root->exp);
+
+ var->type = value_type (value);

- var->type = value_type (var->value);
+ install_new_value (var, value, 1 /* Initial assignment. */);

/* Set language info */
lang = variable_language (var);
@@ -825,8 +827,17 @@ varobj_set_value (struct varobj *var, ch
return 0;
}

- if (!my_value_equal (var->value, value, &error))
+ /* All types are the editable must also be changeable. */
+ gdb_assert (type_changeable (var));
+
+ /* Value of a changeable variable object must not be lazy. */
+ gdb_assert (!value_lazy (var->value));
+
+ if (!value_contents_equal (var->value, value))
var->updated = 1;
+
+ /* The new value may be lazy, gdb_value_assign, or
+ rather value_contents, will take care of this. */
if (!gdb_value_assign (var->value, value, &val))
return 0;
value_free (var->value);
@@ -870,6 +881,92 @@ varobj_list (struct varobj ***varlist)
return rootcount;
}

+/** 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)
+{
+ int changeable;
+ int changed = 0;
+
+ var->error = 0;
+ /* We need to know the varobj's type to decide if the value should
+ be fetched or not. C++ fake children (public/protected/private) don't have
+ a type. */
+ gdb_assert (var->type || CPLUS_FAKE_CHILD (var));
+ if (CPLUS_FAKE_CHILD (var))
+ changeable = 0;
+ else
+ changeable = type_changeable (var);
+
+ /* 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. */
+ if (changeable && value && value_lazy (value))
+ {
+ if (!gdb_value_fetch_lazy (value))
+ {
+ var->error = 1;
+ /* 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. */
+ value = NULL;
+ }
+ else
+ var->error = 0;
+ }
+
+ /* If the type is changeable, compare the old and the new values.
+ If the type of varobj changed, no point in comparing values. */
+ 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;
+ else
+ {
+ /* Try to compare the values. That requires that both
+ values are non-lazy. */
+
+ /* Quick comparison of NULL values. */
+ if (var->value == NULL && value == NULL)
+ /* Equal. */
+ ;
+ else if (var->value == NULL || value == NULL)
+ changed = 1;
+ else
+ {
+ gdb_assert (!value_lazy (var->value));
+ gdb_assert (!value_lazy (value));
+
+ if (!value_contents_equal (var->value, value))
+ changed = 1;
+ }
+ }
+ }
+
+ /* We must always keep new values, since children depend on it. */
+ if (var->value != NULL)
+ value_free (var->value);
+ var->value = value;
+ var->updated = 0;
+
+ return changed;
+}
+
+
/* Update the values for a variable and its children. This is a
two-pronged attack. First, re-parse the value for the root's
expression to see if it's changed. Then go all the way
@@ -939,24 +1036,16 @@ varobj_update (struct varobj **varp, str
vpush (&result, *varp);
changed++;
}
- /* If values are not equal, note that it's changed.
- There a couple of exceptions here, though.
- We don't want some types to be reported as "changed". */
- else if (type_changeable (*varp) &&
- ((*varp)->updated || !my_value_equal ((*varp)->value, new, &error)))
+
+ if (install_new_value ((*varp), new, type_changed))
{
- vpush (&result, *varp);
- (*varp)->updated = 0;
+ /* If type_changed is 1, install_new_value will never return
+ non-zero, so we'll never report the same variable twice. */
+ gdb_assert (!type_changed);
+ vpush (&result, (*varp));
changed++;
- /* Its value is going to be updated to NEW. */
- (*varp)->error = error;
}

- /* We must always keep around the new value for this root
- variable expression, or we lose the updated children! */
- value_free ((*varp)->value);
- (*varp)->value = new;
-
/* Initialize a stack */
vpush (&stack, NULL);

@@ -982,21 +1071,13 @@ varobj_update (struct varobj **varp, str

/* Update this variable */
new = value_of_child (v->parent, v->index);
- if (type_changeable (v) &&
- (v->updated || !my_value_equal (v->value, new, &error)))
- {
+ if (install_new_value (v, new, 0 /* type not changed */))
+ {
/* Note that it's changed */
vpush (&result, v);
v->updated = 0;
changed++;
}
- /* Its value is going to be updated to NEW. */
- v->error = error;
-
- /* We must always keep new values, since children depend on it. */
- if (v->value != NULL)
- value_free (v->value);
- v->value = new;

/* Get next child */
v = vpop (&stack);
@@ -1257,15 +1338,14 @@ create_child (struct varobj *parent, int
{
struct varobj *child;
char *childs_name;
+ struct value *value;

child = new_variable ();

/* name is allocated by name_of_child */
child->name = name;
child->index = index;
- child->value = value_of_child (parent, index);
- if ((!CPLUS_FAKE_CHILD (child) && child->value == NULL) || parent->error)
- child->error = 1;
+ value = value_of_child (parent, index);
child->parent = parent;
child->root = parent->root;
childs_name = xstrprintf ("%s.%s", parent->obj_name, name);
@@ -1275,8 +1355,20 @@ create_child (struct varobj *parent, int
/* Save a pointer to this child in the parent */
save_child_in_parent (parent, child);

- /* Note the type of this child */
- child->type = type_of_child (child);
+ /* Compute the type of the child. Must do this before
+ calling install_new_value. */
+ if (value != NULL)
+ /* If the child had no evaluation errors, var->value
+ will be non-NULL and contain a valid type. */
+ child->type = value_type (value);
+ else
+ /* Otherwise, we must compute the type. */
+ child->type = (*child->root->lang->type_of_child) (child->parent,
+ child->index);
+ install_new_value (child, value, 1);
+
+ if ((!CPLUS_FAKE_CHILD (child) && child->value == NULL) || parent->error)
+ child->error = 1;

return child;
}
@@ -1451,42 +1543,6 @@ variable_default_display (struct varobj
return FORMAT_NATURAL;
}

-/* This function is similar to GDB's value_contents_equal, except that
- this one is "safe"; it never longjmps. It determines if the VAL1's
- value is the same as VAL2. If for some reason the value of VAR2
- can't be established, *ERROR2 is set to non-zero. */
-
-static int
-my_value_equal (struct value *val1, struct value *volatile val2, int *error2)
-{
- volatile struct gdb_exception except;
-
- /* As a special case, if both are null, we say they're equal. */
- if (val1 == NULL && val2 == NULL)
- return 1;
- else if (val1 == NULL || val2 == NULL)
- return 0;
-
- /* The contents of VAL1 are supposed to be known. */
- gdb_assert (!value_lazy (val1));
-
- /* Make sure we also know the contents of VAL2. */
- val2 = coerce_array (val2);
- TRY_CATCH (except, RETURN_MASK_ERROR)
- {
- if (value_lazy (val2))
- value_fetch_lazy (val2);
- }
- if (except.reason < 0)
- {
- *error2 = 1;
- return 0;
- }
- gdb_assert (!value_lazy (val2));
-
- return value_contents_equal (val1, val2);
-}
-
/* FIXME: The following should be generic for any pointer */
static void
vpush (struct vstack **pstack, struct varobj *var)
@@ -1678,33 +1734,9 @@ value_of_child (struct varobj *parent, i

value = (*parent->root->lang->value_of_child) (parent, index);

- /* If we're being lazy, fetch the real value of the variable. */
- if (value != NULL && value_lazy (value))
- {
- /* If we fail to fetch the value of the child, return
- NULL so that callers notice that we're leaving an
- error message. */
- if (!gdb_value_fetch_lazy (value))
- value = NULL;
- }
-
return value;
}

-/* What is the type of VAR? */
-static struct type *
-type_of_child (struct varobj *var)
-{
-
- /* If the child had no evaluation errors, var->value
- will be non-NULL and contain a valid type. */
- if (var->value != NULL)
- return value_type (var->value);
-
- /* Otherwise, we must compute the type. */
- return (*var->root->lang->type_of_child) (var->parent, var->index);
-}
-
/* Is this variable editable? Use the variable's type to make
this determination. */
static int
@@ -1898,19 +1930,7 @@ c_value_of_root (struct varobj **var_han
go on */
if (gdb_evaluate_expression (var->root->exp, &new_val))
{
- if (value_lazy (new_val))
- {
- /* We need to catch errors because if
- value_fetch_lazy fails we still want to continue
- (after making val->error = 1) */
- /* FIXME: Shouldn't be using value_contents()? The
- comment on value_fetch_lazy() says it is only called
- from the macro... */
- if (!gdb_value_fetch_lazy (new_val))
- var->error = 1;
- else
- var->error = 0;
- }
+ var->error = 0;
}
else
var->error = 1;
@@ -2094,8 +2114,8 @@ c_value_of_variable (struct varobj *var)
struct cleanup *old_chain = make_cleanup_ui_file_delete (stb);
char *thevalue;

- if (value_lazy (var->value))
- gdb_value_fetch_lazy (var->value);
+ gdb_assert (type_changeable (var));
+ gdb_assert (!value_lazy (var->value));
common_val_print (var->value, stb,
format_code[(int) var->format], 1, 0, 0);
thevalue = ui_file_xstrdup (stb, &dummy);