This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC] Variable objects for STL containers


Nick Roberts wrote:

> 
> I realise that Vladimir is also working on this too but this patch doesn't
> require Python scripting and it's quite simple too, just involving changes
> to varobj.c and mi-cmd-var.c.  It uses a special variable object to keep track
> of the number of STL members as I described previously.  It's for after the
> release of 6.8 and intended as the basis of a discussion.  I don't expect it to
> be approved as is.
> 
> Here's a sample transaction.  Currently the variable object must be created
> after initialisation, e.g.,
> 
> (gdb)
> n
> &"n\n"
> ~"16\t  vector<int> v1;  // create an empty vector of integers\n"
> ^done
> (gdb)
> -var-create - * v
> ^done,name="var1",numchild="1",value="{...}",type="std::vector<int,std::allocator<int> >"
> (gdb)
> -var-list-children --all-values var1
>
^done,numchild="4",children=[child={name="var1.0",exp="0",numchild="0",value="3",type="long"},child={name="var1.1",exp="1",numchild="0",value="0",type="int"},child={name="var1.2",exp="2",numchild="0",value="0",type="int"},child={name="var1.3",exp="3",numchild="0",value="0",type="int"}]
> (gdb)

I don't particularly like the use of this special child to indicate
the number of children. We already have a mechanism to report the number of
children and having yet another mechanism seems just confusing.

Of course, if we use the existing mechanism then -var-update will have
to be able to report the variable objects that have the number of children
changed -- and a frontend will have to be adjusted to handle that mechanism.

However, it's not harder for frontend than the approach above. In fact,
using the approach above, how will frontend know that a varobj is
the special one that reports the number of children? I don't see anything
in the output above to indicate the special role of var1.0

> so that GDB doesn't find a ridiculous (uninitialised) number of children.
> Note there are four children. var1.0 is the special variable object which
> is equal to the actual number of STL members.
> (v._M_impl._M_finish - v._M_impl._M_start)

I must admit I don't know who to solve the problem of uninitialized
classes satisfactory, short of hacking gcc. The only thing we can do is
to use on-demand special presentation -- only when user requests it.
That sounds pretty uncool.

> u
> &"u\n"
> ~"33\t  v[0] = 1;\n"
> ^done
> (gdb)
> u
> &"u\n"
> ~"34\t  v[1] = 11;\n"
> ^done
> (gdb)
> u
> &"u\n"
> ~"35\t  v[2] = 22;\n"
> ^done
> (gdb)
> 
> 
> Remember that line 35 hasn't been executed yet.
> 
> 
> -var-update --all-values *
> ^done,changelist=[{name="var1.1",value="1",in_scope="true",type_changed="false"}
{name="var1.2",value="11",in_scope="true",type_changed="false"}]
> (gdb)
> u
> &"u\n"
> ~"36\t  v.push_back (7);\n"
> ^done
> (gdb)
> 
> 
> Now it has.
> 
> 
> -var-update --all-values *
> ^done,changelist=[{name="var1.3",value="22",in_scope="true",type_changed="false"}]
> (gdb)
> u
> &"u\n"
> ~"38\t  cout << endl;\n"
> ^done
> (gdb)
> 
> 
> Now v.push_back (7) has added a new member which changes the value of var1.0.
> 
> 
> -var-update --all-values *
> ^done,changelist=[{name="var1.0",value="4",in_scope="true",type_changed="false"}]
> (gdb)
> 
> 
> And we can automatically see the new member here.
> 
> 
> -var-list-children --all-values var1
>
^done,numchild="5",children=[child={name="var1.0",exp="0",numchild="0",value="4",type="long"},child={name="var1.1",exp="1",numchild="0",value="1",type="int"},child={name="var1.2",exp="2",numchild="0",value="11",type="int"},child={name="var1.3",exp="3",numchild="0",value="22",type="int"},child={name="var1.4",exp="4",numchild="0",value="7",type="int"}]
> 

I would have preferred for the new variable objects to be present in the
output of -var-update.

> *** varobj.c  05 Feb 2008 09:38:32 +1300      1.103
> --- varobj.c  13 Feb 2008 16:10:13 +1300
> *************** struct varobj
> *** 147,152 ****
> --- 147,156 ----
>        not fetched if either the variable is frozen, or any parents is
>        frozen.  */
>     int not_fetched;
> +
> +   /* 0 means normal varobj,
> +      1 means special varobj to track number of STL members.  */
> +   int stl;
>   };
>   
>   struct cpstack
> *************** static void uninstall_variable (struct v
> *** 178,183 ****
> --- 182,189 ----
>   
>   static struct varobj *create_child (struct varobj *, int, char *);
>   
> + static struct varobj *create_stl_child (struct varobj *, int, char *);
> +
>   /* Utility routines */
>   
>   static struct varobj *new_variable (void);
> *************** static struct value *value_of_root (stru
> *** 217,222 ****
> --- 223,230 ----
>   
>   static struct value *value_of_child (struct varobj *parent, int index);
>   
> + static struct value *value_of_stl_child (struct varobj *parent, int index);
> +
>   static char *my_value_of_variable (struct varobj *var);
>   
>   static char *value_get_print_value (struct value *value,
> *************** varobj_list_children (struct varobj *var
> *** 758,763 ****
> --- 766,815 ----
>     return var->children;
>   }
>   
> + VEC (varobj_p)*
> + varobj_list_stl_children (struct varobj *var)
> + {
> +   struct varobj *child;
> +   struct value *value;
> +   struct expression *expr;
> +   char *stl_child;
> +   char *name;
> +   int i;
> +
> +
> +   stl_child = xstrprintf ("%s.%s - %s.%s",
> +                       var->name, "_M_impl._M_finish",
> +                       var->name, "_M_impl._M_start");
> +   expr = parse_expression (stl_child);
> +   gdb_evaluate_expression (expr, &value);
> +   var->num_children = 1 + (int) unpack_long (value_type (value),
> +                                              value_contents (value));

IIUC, at this point the number of children can decrease. It does not seem like
we're informing the frontend about this, or free the varobjs. We probably should.

> +   xfree (stl_child);
> +
> +   /* If we're called when the list of children is not yet initialized,
> +      allocate enough elements in it.  */
> +   while (VEC_length (varobj_p, var->children) < var->num_children)
> +     VEC_safe_push (varobj_p, var->children, NULL);
> +
> +   for (i = 0; i < var->num_children; i++)
> +     {
> +       varobj_p 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 = xstrprintf ("%d", i);
> +
> +       existing = create_stl_child (var, i, name);
> +       VEC_replace (varobj_p, var->children, i, existing);
> +     }
> +     }
> +
> +   return var->children;
> + }
> +
>   /* Obtain the type of an object Variable as a string similar to the one gdb
>      prints on the console */
>   
> *************** install_new_value (struct varobj *var, s
> *** 1055,1061 ****
>   
>   gdb_assert (var->print_value != NULL && print_value != NULL);
>   if (strcmp (var->print_value, print_value) != 0)
> !             changed = 1;
>   }
>   }
>       }
> --- 1107,1117 ----
>   
>   gdb_assert (var->print_value != NULL && print_value != NULL);
>   if (strcmp (var->print_value, print_value) != 0)
> !             {
> !               changed = 1;
> !               if (var->stl)
> !                 varobj_list_stl_children (var->parent);
> !             }
>   }
>   }
>       }
> *************** varobj_update (struct varobj **varp, str
> *** 1191,1197 ****
>   updated.  */
>         if (v->root->rootvar != v)
>   {
> !       new = value_of_child (v->parent, v->index);
>   if (install_new_value (v, new, 0 /* type not changed */))
>   {
>   /* Note that it's changed */
> --- 1247,1266 ----
>   updated.  */
>         if (v->root->rootvar != v)
>   {
> !       struct expression *expr;
> !       struct value *value;
> !       char* stl_member;
> !
> !       //TODO:  Just deal with vectors for the moment.
> !       stl_member = xstrprintf ("%s._M_impl._M_start",
> !                                varobj_get_expression (v->parent));
> !       expr = parse_expression (stl_member);

Is parse_expression guaranteed not to throw? (Not a rhetoric 
question -- I don't know). If not, then you should use cleanup here.

> !       if (gdb_evaluate_expression (expr, &value))
> !         new = value_of_stl_child (v->parent, v->index);
> !       else
> !         new = value_of_child (v->parent, v->index);
> !       xfree (stl_member);
> !
>   if (install_new_value (v, new, 0 /* type not changed */))
>   {
>   /* Note that it's changed */
> *************** create_child (struct varobj *parent, int
> *** 1448,1453 ****
> --- 1517,1557 ----
>   
>     return child;
>   }
> +
> + static struct varobj *
> + create_stl_child (struct varobj *parent, int index, char *name)
> + {
> +   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;
> +   value = value_of_stl_child (parent, index);
> +   child->parent = parent;
> +   child->root = parent->root;
> +   childs_name = xstrprintf ("%s.%s", parent->obj_name, name);
> +   child->obj_name = childs_name;
> +   if (index == 0) child->stl = 1;
> +   install_variable (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);
> +
> +   return child;

How this one differs from create_child? It appears that the differences are minimal,
so a helper function is called for.

> + }
>   
>   
>   /*
> *************** new_variable (void)
> *** 1476,1481 ****
> --- 1580,1586 ----
>     var->print_value = NULL;
>     var->frozen = 0;
>     var->not_fetched = 0;
> +   var->stl = 0;
>   
>     return var;
>   }
> *************** value_of_child (struct varobj *parent, i
> *** 1768,1773 ****
> --- 1873,1901 ----
>     return value;
>   }
>   
> + static struct value *
> + value_of_stl_child (struct varobj *parent, int index)
> + {
> +   struct expression *expr;
> +   struct value *value;
> +   char *stl_child;
> +
> +   if (index == 0)
> +     /* With i == 0, create a special varobj to detect creation/deletion
> +        of STL members.  */
> +     stl_child = xstrprintf ("%s.%s - %s.%s",
> +                         parent->name, "_M_impl._M_finish",
> +                         parent->name, "_M_impl._M_start");
> +   else
> +     stl_child = xstrprintf ("*(%s.%s + %d)",
> +                         parent->name, "_M_impl._M_start", index - 1);
> +
> +   expr = parse_expression (stl_child);
> +   gdb_evaluate_expression (expr, &value);
> +   xfree (stl_child);
> +   return value;
> + }
> +
>   /* GDB already has a command called "value_of_variable". Sigh. */
>   static char *
>   my_value_of_variable (struct varobj *var)
> 
> 
> 
> *** mi-cmd-var.c      05 Feb 2008 12:13:22 +1300      1.45
> --- mi-cmd-var.c      13 Feb 2008 15:41:50 +1300
> ***************
> *** 28,33 ****
> --- 28,34 ----
>   #include "value.h"
>   #include <ctype.h>
>   #include "gdb_string.h"
> + #include "wrapper.h"
>   
>   const char mi_no_values[] = "--no-values";
>   const char mi_simple_values[] = "--simple-values";
> *************** mi_cmd_var_list_children (char *command,
> *** 361,366 ****
> --- 362,370 ----
>     int numchild;
>     enum print_values print_values;
>     int ix;
> +   struct expression *expr;
> +   struct value *value;
> +   char* stl_member;
>   
>     if (argc != 1 && argc != 2)
>       error (_("mi_cmd_var_list_children: Usage: [PRINT_VALUES] NAME"));
> *************** mi_cmd_var_list_children (char *command,
> *** 373,379 ****
>     if (var == NULL)
>       error (_("Variable object not found"));
>   
> !   children = varobj_list_children (var);
>     ui_out_field_int (uiout, "numchild", VEC_length (varobj_p, children));
>     if (argc == 2)
>       print_values = mi_parse_values_option (argv[0]);
> --- 377,391 ----
>     if (var == NULL)
>       error (_("Variable object not found"));
>   
> !   //TODO:  Just deal with vectors for the moment.
> !   stl_member = xstrprintf ("%s._M_impl._M_start", varobj_get_expression (var));
> !   expr = parse_expression (stl_member);
> !   if (gdb_evaluate_expression (expr, &value))
> !       children = varobj_list_stl_children (var);
> !     else
> !       children = varobj_list_children (var);

I think it's clearer not to break encapsulation of varobj, and have just
varobj_list_children in the public interface, which then can do anything it likes.
This way, we won't need to adjust this place when we add some new kind of 
special type.

> !   xfree (stl_member);
> !
>     ui_out_field_int (uiout, "numchild", VEC_length (varobj_p, children));
>     if (argc == 2)
>       print_values = mi_parse_values_option (argv[0]);
> *************** varobj_update_one (struct varobj *var, e
> *** 651,657 ****
>   cc++;
>   }
>         xfree (changelist);
> !     }Write failed flushing stdout buffer.
> ! write stdout: Broken pipe
> !
>   }
> --- 663,667 ----
>   cc++;
>   }
>         xfree (changelist);
> !     }
>   }

There's one overall thing I'd do differently. In your patch,
we see varobj_list_stl_children, create_stl_child 
and value_of_stl_child, where the first basically computes the number of children,
the third knows how to map from child index into value, and the second is a
helper function.

For scripting purposes, and for simplicity in general, in better if one
has to implement just one function for any new type. Further, while mapping
from index to value is simple for std::vector, it's much less trivial for
list, and even less trivial for map. We better not traverse the list when
getting each element.

So, I will prefer a mechanism where one can write a function taking a varobj.
That function should be able to compute the list of children, and then create
varobj children specifying the value directly. So, in case of std::list,
we'd traverse the list once, get the values and create varobjs with those
values. Of course, we'd need some support -- in particular a version of
create_child that takes a value. We will need to change varobj_update so
that it knows not to call value_of_child on children of special varobj, as well.

- Volodya




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]