Re: Cleanup varobj children handling

Vladimir Prus
2006-12-26T20:50:21Z

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

On Tuesday 26 December 2006 19:10, Daniel Jacobowitz wrote:
> Thanks for doing this, I think it's a good cleanup. To answer Nick's
> questions: Vlad and I hadn't talked about this change, but I agree with
> him. The FIXME about should use vlists goes along with this one that
> Vlad deleted:
>
> /* FIXME: This should be a generic add to list */
>
> vec.h is basically the generic list structure the code was asking for.
>
> On Thu, Dec 21, 2006 at 06:25:04PM +0300, Vladimir Prus wrote:
> > #include "varobj.h"
> > +#include "vec.h"
> > +
>
> Extra blank line.

Fixed.

> > - /* A list of this object's children */
> > - struct varobj_child *children;
> > + /* Children of this object. */
> > + VEC (varobj_p) *children;
>
> I've been omitting the space before the parenthesis for VEC, since it's
> a macro creating a type and syntactically very different from a
> function call. I can't remember where I got that habit though! Either
> way is fine.

Unfortunately, my emacs immediately shows

VEC (varobj_p)

in red, because I have a some code to catch the (numerous) cases
when I try to use C++ coding style. I guess it will be a bit hard to
to suppress this check for VEC.

> > + /* If we're called when the list of children is not yet initialized,
> > + allocated enough elements in it. */
> > + while (VEC_length (varobj_p, var->children) < var->num_children)
> > + VEC_safe_push (varobj_p, var->children, NULL);
>
> You need the NULLs initialized here, right? Otherwise you could use
> grow instead, but that doesn't initialize.

Yes, I need new elements to be NULL.

> > @@ -721,13 +701,18 @@ varobj_list_children (struct varobj *var
> > /* Mark as the end in case we bail out */
> > *((*childlist) + i) = NULL;
> >
> > - /* check if child exists, if not create */
> > - name = name_of_child (var, i);
> > - child = child_exists (var, name);
> > - if (child == NULL)
> > - child = create_child (var, i, name);
> > + varobj_p *existing = VEC_address (varobj_p, var->children) + i;
>
> Please declare existing at the top of the block, instead of in the
> middle; we only require C90 compatibility. Do you really need to use
> VEC_address here? You could use VEC_index to read it and VEC_replace
> to modify it; I find that easier to read than a pointer into the vec's
> storage.

I've changed to use VEC_replace. I don't see much improvement, though.

> > + for (i = 0; i < VEC_length (varobj_p, (*varp)->children); ++i)
> > + {
> > + VEC_safe_push (varobj_p, stack,
> > + VEC_index (varobj_p, (*varp)->children, i));
> > }
>
> Here and elsewhere, VEC_iterate avoids having to call VEC_index -
> that's what it's for.

I personally find VEC_iterate to be less clear -- because it does not
correspond to any iteration pattern in any language I know. Do you insist
on using it?

> > - /* Copy from result stack to list */
> > - vleft = changed;
> > - *cv = vpop (&result);
> > - while ((*cv != NULL) && (vleft > 0))
> > - {
> > - vleft--;
> > - cv++;
> > - *cv = vpop (&result);
> > - }
> > - if (vleft)
> > - warning (_("varobj_update: assertion failed - vleft <> 0"));
> > + cv = *changelist;
> >
> > - if (changed > 1)
> > + for (i = 0; i < changed; ++i)
> > {
> > - /* Now we revert the order. */
> > - for (i = 0; i < changed; i++)
> > - *(*changelist + i) = *(templist + changed - 1 - i);
> > - *(*changelist + changed) = NULL;
> > + *cv = VEC_index (varobj_p, result, i);
> > + gdb_assert (*cv != NULL);
> > + ++cv;
> > }
> > + *cv = 0;
> >
> > if (type_changed)
> > return -2;
>
> It looks to me as if the old code was somewhat careful about the
> ordering, and the old ordering was a bit nicer. But it also looks to
> me as if you're preserving the same ordering - vpop takes what would be
> the "last" item in the vstack. So why does this patch reverse the order
> for the testsuite?

Here's what happens. The old code used vpush to construct the result,
and then a loop ("revert the order" comment above) that vpops elements
and places them to the right position. The new code uses VEC_safe_push.
We don't need to reverse the order -- because with VEC we have
random access anyway. So, no order change here.

However, the order of children is different. Old code used a hand-made list,
with each new constructed child added the the front of the list. New code
uses VEC, with each new child added at the end of the list.

So, for children we iterate them in natural order and push them to working
stack. We pop the last child, see that it changed, and push it to the result vector.
At this point, the order of children of any given varobj is reversed.

I've just worked this around by pushing the children in reverse order.

> > @@ -1227,7 +1179,7 @@ delete_variable_1 (struct cpstack **resu
> > discarding the list afterwards */
> > if ((remove_from_parent_p) && (var->parent != NULL))
> > {
> > - remove_child_from_parent (var->parent, var);
> > + VEC_replace (varobj_p, var->parent->children, var->index, NULL);
> > }
> >
> > if (var->obj_name != NULL)
>
> I think that this will cause crashes in -var-update, since that walks
> the list of children without checking for NULL. If I'm right, you
> probably want to add NULL checks rather than use VEC_ordered_remove,
> so that the indexes still tell you which child it is?

Ick! You're right, this will segfault. Not a common case though -- I don't think
anybody's going to delete select children of a varobj. Fixed.

Please find the revised patch attached, as well as delta relative to
the previous version.

- Volodya

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

--- gdb/varobj.c (/patches/gdb/path_1/gdb_mainline) (revision 2957)
+++ gdb/varobj.c (/patches/gdb/path_2_children/gdb_mainline) (revision 2957)
@@ -31,6 +31,7 @@
#include "gdb_string.h"

#include "varobj.h"
+#include "vec.h"

/* Non-zero if we want to see trace of varobj level stuff. */

@@ -79,6 +80,10 @@ struct varobj_root
struct varobj_root *next;
};

+typedef struct varobj *varobj_p;
+
+DEF_VEC_P (varobj_p);
+
/* Every variable in the system has a structure of this type defined
for it. This structure holds all information necessary to manipulate
a particular object variable. Members which must be freed are noted. */
@@ -115,8 +120,8 @@ struct varobj
/* If this object is a child, this points to its immediate parent. */
struct varobj *parent;

- /* A list of this object's children */
- struct varobj_child *children;
+ /* Children of this object. */
+ VEC (varobj_p) *children;

/* Description of the root variable. Points to root variable for children. */
struct varobj_root *root;
@@ -128,29 +133,6 @@ struct varobj
int updated;
};

-/* Every variable keeps a linked list of its children, described
- by the following structure. */
-/* FIXME: Deprecated. All should use vlist instead */
-
-struct varobj_child
-{
-
- /* Pointer to the child's data */
- struct varobj *child;
-
- /* Pointer to the next child */
- struct varobj_child *next;
-};
-
-/* A stack of varobjs */
-/* FIXME: Deprecated. All should use vlist instead */
-
-struct vstack
-{
- struct varobj *var;
- struct vstack *next;
-};
-
struct cpstack
{
char *name;
@@ -178,14 +160,8 @@ static int install_variable (struct varo

static void uninstall_variable (struct varobj *);

-static struct varobj *child_exists (struct varobj *, char *);
-
static struct varobj *create_child (struct varobj *, int, char *);

-static void save_child_in_parent (struct varobj *, struct varobj *);
-
-static void remove_child_from_parent (struct varobj *, struct varobj *);
-
/* Utility routines */

static struct varobj *new_variable (void);
@@ -204,10 +180,6 @@ static struct type *get_target_type (str

static enum varobj_display_formats variable_default_display (struct varobj *);

-static void vpush (struct vstack **pstack, struct varobj *var);
-
-static struct varobj *vpop (struct vstack **pstack);
-
static void cppush (struct cpstack **pstack, char *name);

static char *cppop (struct cpstack **pstack);
@@ -713,21 +685,34 @@ varobj_list_children (struct varobj *var
if (var->num_children == -1)
var->num_children = number_of_children (var);

+ /* If we're called when the list of children is not yet initialized,
+ allocated enough elements in it. */
+ while (VEC_length (varobj_p, var->children) < var->num_children)
+ VEC_safe_push (varobj_p, var->children, NULL);
+
/* List of children */
*childlist = xmalloc ((var->num_children + 1) * sizeof (struct varobj *));

for (i = 0; i < var->num_children; i++)
{
+ varobj_p existing;
+
/* Mark as the end in case we bail out */
*((*childlist) + i) = NULL;

- /* check if child exists, if not create */
- name = name_of_child (var, i);
- child = child_exists (var, name);
- if (child == NULL)
- child = create_child (var, i, name);
+ existing = VEC_index (varobj_p, var->children, i);
+
+ if (existing == NULL)
+ {
+ /* Either it's the first call to varobj_list_children for
+ this variable object, and the child was never created,
+ or it was explicitly deleted by the client. */
+ name = name_of_child (var, i);
+ existing = create_child (var, i, name);
+ VEC_replace (varobj_p, var->children, i, existing);
+ }

- *((*childlist) + i) = child;
+ *((*childlist) + i) = existing;
}

/* End of list is marked by a NULL pointer */
@@ -1034,8 +1019,8 @@ varobj_update (struct varobj **varp, str
struct varobj **cv;
struct varobj **templist = NULL;
struct value *new;
- struct vstack *stack = NULL;
- struct vstack *result = NULL;
+ VEC (varobj_p) *stack = NULL;
+ VEC (varobj_p) *result = NULL;
struct frame_id old_fid;
struct frame_info *fi;

@@ -1071,94 +1056,64 @@ varobj_update (struct varobj **varp, str
return -1;
}

- /* Initialize a stack for temporary results */
- vpush (&result, NULL);
-
/* If this is a "use_selected_frame" varobj, and its type has changed,
them note that it's changed. */
if (type_changed)
- {
- vpush (&result, *varp);
- changed++;
- }
+ VEC_safe_push (varobj_p, result, *varp);

if (install_new_value ((*varp), new, type_changed))
{
/* 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++;
+ VEC_safe_push (varobj_p, result, *varp);
}

- /* Initialize a stack */
- vpush (&stack, NULL);
-
- /* Push the root's children */
- if ((*varp)->children != NULL)
- {
- struct varobj_child *c;
- for (c = (*varp)->children; c != NULL; c = c->next)
- vpush (&stack, c->child);
- }
+ VEC_safe_push (varobj_p, stack, *varp);

/* Walk through the children, reconstructing them all. */
- v = vpop (&stack);
- while (v != NULL)
+ while (!VEC_empty (varobj_p, stack))
{
- /* Push any children */
- if (v->children != NULL)
- {
- struct varobj_child *c;
- for (c = v->children; c != NULL; c = c->next)
- vpush (&stack, c->child);
- }
+ v = VEC_pop (varobj_p, stack);

- /* Update this variable */
- new = value_of_child (v->parent, v->index);
- if (install_new_value (v, new, 0 /* type not changed */))
- {
- /* Note that it's changed */
- vpush (&result, v);
- v->updated = 0;
- changed++;
+ /* Push any children. Use reverse order so that first
+ child is popped from the work stack first, and so
+ will be added to result first. This does not
+ affect correctness, just "nicer". */
+ for (i = VEC_length (varobj_p, v->children)-1; i >= 0; --i)
+ {
+ varobj_p c = VEC_index (varobj_p, v->children, i);
+ /* Child may be NULL is explicitly deleted by -var-delete. */
+ if (c != NULL)
+ VEC_safe_push (varobj_p, stack, c);
+ }
+
+ /* Update this variable, unless it's root, which is already
+ updated. */
+ 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);
+ v->updated = 0;
+ }
}
-
- /* Get next child */
- v = vpop (&stack);
}

/* Alloc (changed + 1) list entries */
- /* FIXME: add a cleanup for the allocated list(s)
- because one day the select_frame called below can longjump */
+ changed = VEC_length (varobj_p, result);
*changelist = xmalloc ((changed + 1) * sizeof (struct varobj *));
- if (changed > 1)
- {
- templist = xmalloc ((changed + 1) * sizeof (struct varobj *));
- cv = templist;
- }
- else
- cv = *changelist;
-
- /* Copy from result stack to list */
- vleft = changed;
- *cv = vpop (&result);
- while ((*cv != NULL) && (vleft > 0))
- {
- vleft--;
- cv++;
- *cv = vpop (&result);
- }
- if (vleft)
- warning (_("varobj_update: assertion failed - vleft <> 0"));
+ cv = *changelist;

- if (changed > 1)
+ for (i = 0; i < changed; ++i)
{
- /* Now we revert the order. */
- for (i = 0; i < changed; i++)
- *(*changelist + i) = *(templist + changed - 1 - i);
- *(*changelist + changed) = NULL;
+ *cv = VEC_index (varobj_p, result, i);
+ gdb_assert (*cv != NULL);
+ ++cv;
}
+ *cv = 0;

if (type_changed)
return -2;
@@ -1194,18 +1149,17 @@ delete_variable_1 (struct cpstack **resu
struct varobj *var, int only_children_p,
int remove_from_parent_p)
{
- struct varobj_child *vc;
- struct varobj_child *next;
+ int i;

/* Delete any children of this variable, too. */
- for (vc = var->children; vc != NULL; vc = next)
- {
+ for (i = 0; i < VEC_length (varobj_p, var->children); ++i)
+ {
+ varobj_p child = VEC_index (varobj_p, var->children, i);
if (!remove_from_parent_p)
- vc->child->parent = NULL;
- delete_variable_1 (resultp, delcountp, vc->child, 0, only_children_p);
- next = vc->next;
- xfree (vc);
+ child->parent = NULL;
+ delete_variable_1 (resultp, delcountp, child, 0, only_children_p);
}
+ VEC_free (varobj_p, var->children);

/* if we were called to delete only the children we are done here */
if (only_children_p)
@@ -1227,7 +1181,7 @@ delete_variable_1 (struct cpstack **resu
discarding the list afterwards */
if ((remove_from_parent_p) && (var->parent != NULL))
{
- remove_child_from_parent (var->parent, var);
+ VEC_replace (varobj_p, var->parent->children, var->index, NULL);
}

if (var->obj_name != NULL)
@@ -1356,22 +1310,6 @@ uninstall_variable (struct varobj *var)

}

-/* Does a child with the name NAME exist in VAR? If so, return its data.
- If not, return NULL. */
-static struct varobj *
-child_exists (struct varobj *var, char *name)
-{
- struct varobj_child *vc;
-
- for (vc = var->children; vc != NULL; vc = vc->next)
- {
- if (strcmp (vc->child->name, name) == 0)
- return vc->child;
- }
-
- return NULL;
-}
-
/* Create and install a child of the parent of the given name */
static struct varobj *
create_child (struct varobj *parent, int index, char *name)
@@ -1392,9 +1330,6 @@ create_child (struct varobj *parent, int
child->obj_name = childs_name;
install_variable (child);

- /* Save a pointer to this child in the parent */
- save_child_in_parent (parent, child);
-
/* Compute the type of the child. Must do this before
calling install_new_value. */
if (value != NULL)
@@ -1412,46 +1347,6 @@ create_child (struct varobj *parent, int

return child;
}
-
-/* FIXME: This should be a generic add to list */
-/* Save CHILD in the PARENT's data. */
-static void
-save_child_in_parent (struct varobj *parent, struct varobj *child)
-{
- struct varobj_child *vc;
-
- /* Insert the child at the top */
- vc = parent->children;
- parent->children =
- (struct varobj_child *) xmalloc (sizeof (struct varobj_child));
-
- parent->children->next = vc;
- parent->children->child = child;
-}
-
-/* FIXME: This should be a generic remove from list */
-/* Remove the CHILD from the PARENT's list of children. */
-static void
-remove_child_from_parent (struct varobj *parent, struct varobj *child)
-{
- struct varobj_child *vc, *prev;
-
- /* Find the child in the parent's list */
- prev = NULL;
- for (vc = parent->children; vc != NULL;)
- {
- if (vc->child == child)
- break;
- prev = vc;
- vc = vc->next;
- }
-
- if (prev == NULL)
- parent->children = vc->next;
- else
- prev->next = vc->next;
-
-}
\f

/*
@@ -1585,36 +1480,6 @@ variable_default_display (struct varobj

/* FIXME: The following should be generic for any pointer */
static void
-vpush (struct vstack **pstack, struct varobj *var)
-{
- struct vstack *s;
-
- s = (struct vstack *) xmalloc (sizeof (struct vstack));
- s->var = var;
- s->next = *pstack;
- *pstack = s;
-}
-
-/* FIXME: The following should be generic for any pointer */
-static struct varobj *
-vpop (struct vstack **pstack)
-{
- struct vstack *s;
- struct varobj *v;
-
- if ((*pstack)->var == NULL && (*pstack)->next == NULL)
- return NULL;
-
- s = *pstack;
- v = s->var;
- *pstack = (*pstack)->next;
- xfree (s);
-
- return v;
-}
-
-/* FIXME: The following should be generic for any pointer */
-static void
cppush (struct cpstack **pstack, char *name)
{
struct cpstack *s;
--- gdb/Makefile.in (/patches/gdb/path_1/gdb_mainline) (revision 2957)
+++ gdb/Makefile.in (/patches/gdb/path_2_children/gdb_mainline) (revision 2957)
@@ -2842,7 +2842,7 @@ value.o: value.c $(defs_h) $(gdb_string_
$(gdb_assert_h) $(regcache_h) $(block_h)
varobj.o: varobj.c $(defs_h) $(exceptions_h) $(value_h) $(expression_h) \
$(frame_h) $(language_h) $(wrapper_h) $(gdbcmd_h) $(gdb_assert_h) \
- $(gdb_string_h) $(varobj_h)
+ $(gdb_string_h) $(varobj_h) $(vec_h)
vaxbsd-nat.o: vaxbsd-nat.c $(defs_h) $(inferior_h) $(regcache_h) $(target_h) \
$(vax_tdep_h) $(inf_ptrace_h) $(bsd_kvm_h)
vax-nat.o: vax-nat.c $(defs_h) $(inferior_h) $(gdb_assert_h) $(vax_tdep_h) \

Property changes on:
___________________________________________________________________
Name: csl:base
+/all/patches/gdb/path_1/gdb_mainline
Name: svk:merge
+d48a11ec-ee1c-0410-b3f5-c20844f99675:/patches/gdb/path_1/gdb_mainline:2869
+d48a11ec-ee1c-0410-b3f5-c20844f99675:/patches/gdb/path_expression/gdb_mainline:2562
e7755896-6108-0410-9592-8049d3e74e28:/mirrors/gdb/trunk:157978

[-- Attachment #3: path_2_children_delta.diff --]
[-- Type: text/x-diff, Size: 6876 bytes --]

--- gdb/testsuite/gdb.mi/mi-var-child.exp (revision 2956)
+++ gdb/testsuite/gdb.mi/mi-var-child.exp (local)
@@ -835,7 +835,7 @@ mi_execute_to "exec-step 7" "end-steppin
# Test: c_variable-5.8
# Desc: check that long_array[3-9] changed
mi_gdb_test "-var-update *" \
- "\\^done,changelist=\\\[\{name=\"struct_declarations.long_array.9\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.8\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.7\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.6\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.5\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.4\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.3\",in_scope=\"true\",type_changed=\"false\"\}\\\]" \
+ "\\^done,changelist=\\\[\{name=\"struct_declarations.long_array.3\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.4\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.5\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.6\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.7\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.8\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.9\",in_scope=\"true\",type_changed=\"false\"\}\\\]" \
"update all vars struct_declarations.long_array.3-9 changed"

--- gdb/testsuite/gdb.mi/mi2-var-child.exp (revision 2956)
+++ gdb/testsuite/gdb.mi/mi2-var-child.exp (local)
@@ -831,7 +831,7 @@ mi_execute_to "exec-step 7" "end-steppin
# Test: c_variable-5.8
# Desc: check that long_array[3-9] changed
mi_gdb_test "-var-update *" \
- "\\^done,changelist=\\\[\{name=\"struct_declarations.long_array.9\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.8\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.7\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.6\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.5\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.4\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.3\",in_scope=\"true\",type_changed=\"false\"\}\\\]" \
+ "\\^done,changelist=\\\[\{name=\"struct_declarations.long_array.3\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.4\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.5\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.6\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.7\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.8\",in_scope=\"true\",type_changed=\"false\"\},\{name=\"struct_declarations.long_array.9\",in_scope=\"true\",type_changed=\"false\"\}\\\]" \
"update all vars struct_declarations.long_array.3-9 changed"

--- gdb/varobj.c (revision 2956)
+++ gdb/varobj.c (local)
@@ -33,7 +33,6 @@
#include "varobj.h"
#include "vec.h"

-
/* Non-zero if we want to see trace of varobj level stuff. */

int varobjdebug = 0;
@@ -684,9 +683,7 @@ varobj_list_children (struct varobj *var
*childlist = NULL;

if (var->num_children == -1)
- {
- var->num_children = number_of_children (var);
- }
+ var->num_children = number_of_children (var);

/* If we're called when the list of children is not yet initialized,
allocated enough elements in it. */
@@ -698,21 +695,24 @@ varobj_list_children (struct varobj *var

for (i = 0; i < var->num_children; i++)
{
+ varobj_p existing;
+
/* Mark as the end in case we bail out */
*((*childlist) + i) = NULL;

- varobj_p *existing = VEC_address (varobj_p, var->children) + i;
+ existing = VEC_index (varobj_p, var->children, i);

- if (*existing == NULL)
+ if (existing == NULL)
{
/* Either it's the first call to varobj_list_children for
this variable object, and the child was never created,
or it was explicitly deleted by the client. */
name = name_of_child (var, i);
- *existing = create_child (var, i, name);
+ existing = create_child (var, i, name);
+ VEC_replace (varobj_p, var->children, i, existing);
}

- *((*childlist) + i) = *existing;
+ *((*childlist) + i) = existing;
}

/* End of list is marked by a NULL pointer */
@@ -1059,9 +1059,7 @@ varobj_update (struct varobj **varp, str
/* If this is a "use_selected_frame" varobj, and its type has changed,
them note that it's changed. */
if (type_changed)
- {
- VEC_safe_push (varobj_p, result, *varp);
- }
+ VEC_safe_push (varobj_p, result, *varp);

if (install_new_value ((*varp), new, type_changed))
{
@@ -1071,32 +1069,36 @@ varobj_update (struct varobj **varp, str
VEC_safe_push (varobj_p, result, *varp);
}

- /* Push the root's children */
- for (i = 0; i < VEC_length (varobj_p, (*varp)->children); ++i)
- {
- VEC_safe_push (varobj_p, stack,
- VEC_index (varobj_p, (*varp)->children, i));
- }
+ VEC_safe_push (varobj_p, stack, *varp);

/* Walk through the children, reconstructing them all. */
while (!VEC_empty (varobj_p, stack))
{
v = VEC_pop (varobj_p, stack);

- /* Push any children */
- for (i = 0; i < VEC_length (varobj_p, v->children); ++i)
- {
- VEC_safe_push (varobj_p, stack,
- VEC_index (varobj_p, v->children, i));
- }
-
- /* Update this variable */
- 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);
- v->updated = 0;
+ /* Push any children. Use reverse order so that first
+ child is popped from the work stack first, and so
+ will be added to result first. This does not
+ affect correctness, just "nicer". */
+ for (i = VEC_length (varobj_p, v->children)-1; i >= 0; --i)
+ {
+ varobj_p c = VEC_index (varobj_p, v->children, i);
+ /* Child may be NULL is explicitly deleted by -var-delete. */
+ if (c != NULL)
+ VEC_safe_push (varobj_p, stack, c);
+ }
+
+ /* Update this variable, unless it's root, which is already
+ updated. */
+ 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);
+ v->updated = 0;
+ }
}
}