[PATCH:gdb/mi] Update a specified list of variable objects

[PATCH:gdb/mi] Update a specified list of variable objects

[Nick Roberts]

Since -var-list-children currently creates variable objects of all children
and this may be a large number, I would like to be able to restrict
-var-update to just update those variable objects whose expressions are
visible in the front end.  This could be done by updating them individually
but that might result in many round trips.  I would therefore like to commit
the following patch which allows a list of variable objects to be specified in
the argument to -var-update:

          -var-update [PRINT_VALUES] NAME1 NAME2 ...

-- 
Nick                                           http://www.inet.net.nz/~nickrob


2009-09-22  Nick Roberts  <nickrob@snap.net.nz>

	* mi/mi-cmd-var.c (mi_cmd_var_update): Accept a list of
	variable objects to update.


*** mi-cmd-var.c.~1.61.~	2009-09-16 18:30:24.000000000 +1200
--- mi-cmd-var.c	2009-09-17 14:54:15.000000000 +1200
*************** mi_cmd_var_update (char *command, char *
*** 660,678 ****
    struct cleanup *cleanup;
    char *name;
    enum print_values print_values;
  
!   if (argc != 1 && argc != 2)
!     error (_("mi_cmd_var_update: Usage: [PRINT_VALUES] NAME."));
  
!   if (argc == 1)
!     name = argv[0];
!   else
!     name = (argv[1]);
  
!   if (argc == 2)
!     print_values = mi_parse_values_option (argv[0]);
!   else
!     print_values = PRINT_NO_VALUES;
  
    if (mi_version (uiout) <= 1)
      cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, "changelist");
--- 660,705 ----
    struct cleanup *cleanup;
    char *name;
    enum print_values print_values;
+   int optind = 0;
+   char *optarg;
  
!   enum opt
!     {
!       NO_VALUES, SIMPLE_VALUES, ALL_VALUES
!     };
  
!   static struct mi_opt opts[] =
!   {
!       { "-no-values", NO_VALUES, 0 },
!       { "-simple-values", SIMPLE_VALUES, 0 },
!       { "-all-values", ALL_VALUES, 0 },
!       { 0, 0, 0 }
!     };
  
!   while (1)
!     {
!       int opt = mi_getopt ("mi_cmd_var_update",
! 			   argc, argv, opts, &optind, &optarg);
!       if (opt < 0)
! 	break;
!       switch ((enum opt) opt)
! 	{
! 	case NO_VALUES:
! 	  print_values = PRINT_NO_VALUES;
! 	  break;
! 	case SIMPLE_VALUES:
! 	  print_values = PRINT_SIMPLE_VALUES;
! 	  break;
! 	case ALL_VALUES:
! 	  print_values = PRINT_ALL_VALUES;
! 	  break;
! 	}
!     }
! 
!   if (optind >= argc)
!     error (_("mi_cmd_var_update: Usage: [PRINT_VALUES] NAME1 NAME2 ..."));
! 
!   name = argv[optind];
  
    if (mi_version (uiout) <= 1)
      cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, "changelist");
*************** mi_cmd_var_update (char *command, char *
*** 697,706 ****
      }
    else
      {
!       /* Get varobj handle, if a valid var obj name was specified */
!       struct varobj *var = varobj_get_handle (name);
! 
!       varobj_update_one (var, print_values, 1 /* explicit */);
      }
  
    do_cleanups (cleanup);
--- 724,737 ----
      }
    else
      {
!       int index = optind;
!       while (argv[index]) {
! 	/* Get varobj handle, if a valid var obj name was specified */
! 	struct varobj *var = varobj_get_handle (argv[index]);
! 
! 	varobj_update_one (var, print_values, 1 /* explicit */);
! 	index++;
!       }
      }
  
    do_cleanups (cleanup);

Re: [PATCH:gdb/mi] Update a specified list of variable objects

[Vladimir Prus]

Nick Roberts wrote:

> Since -var-list-children currently creates variable objects of all children
> and this may be a large number, I would like to be able to restrict
> -var-update to just update those variable objects whose expressions are
> visible in the front end.  This could be done by updating them individually
> but that might result in many round trips.  I would therefore like to commit
> the following patch which allows a list of variable objects to be specified in
> the argument to -var-update:
> 
>           -var-update [PRINT_VALUES] NAME1 NAME2 ...
> 

Hi Nick,

the general idea seems good. There is a minor issue. You have made this change

        -  if (argc == 2)
        -    print_values = mi_parse_values_option (argv[0]);
        -  else
        -    print_values = PRINT_NO_VALUES;
        +  while (1)
        +    {
        +      int opt = mi_getopt ("mi_cmd_var_update",
        +                         argc, argv, opts, &optind, &optarg);

mi_parse_values_option used to accept 0, 1 and 2, in addition to
string values, and your change breaks that. So, I observe this:

(gdb)
-var-create foo @ 10
^done,name="foo",numchild="0",value="10",type="int",has_more="0"
(gdb)
-var-update 0 foo
^error,msg="Variable object not found"

and it works for me with 6.8

Would it be easier to rename mi_parse_values_option to 
mi_parse_values_option_nothrow and write new mi_parse_values_option that
would call mi_parse_values_option_nothrow and call error as necessary?


- Volodya

Re: [PATCH:gdb/mi] Update a specified list of variable objects

[Nick Roberts]

 > mi_parse_values_option used to accept 0, 1 and 2, in addition to
 > string values, and your change breaks that. So, I observe this:
 > 
 > (gdb)
 > -var-create foo @ 10
 > ^done,name="foo",numchild="0",value="10",type="int",has_more="0"
 > (gdb)
 > -var-update 0 foo
 > ^error,msg="Variable object not found"
 > 
 > and it works for me with 6.8

I've tried to address this point below.  It's even in unified diff format!

-- 
Nick                                           http://www.inet.net.nz/~nickrob


--- mi-cmd-var.c.~1.61~	2009-09-16 18:30:24.000000000 +1200
+++ mi-cmd-var.c	2009-09-22 11:16:57.000000000 +1200
@@ -27,6 +27,7 @@
 #include "varobj.h"
 #include "value.h"
 #include <ctype.h>
+#include "gdb_regex.h"
 #include "gdb_string.h"
 #include "mi-getopt.h"
 #include "gdbthread.h"
@@ -660,19 +661,58 @@ mi_cmd_var_update (char *command, char *
   struct cleanup *cleanup;
   char *name;
   enum print_values print_values;
+  int optind = 0;
+  char *optarg;
+  struct varobj *var;
+  char *pv = re_comp ("[012]");
+
+  enum opt
+    {
+      NO_VALUES, SIMPLE_VALUES, ALL_VALUES
+    };
 
-  if (argc != 1 && argc != 2)
-    error (_("mi_cmd_var_update: Usage: [PRINT_VALUES] NAME."));
+  static struct mi_opt opts[] =
+  {
+      { "-no-values", NO_VALUES, 0 },
+      { "-simple-values", SIMPLE_VALUES, 0 },
+      { "-all-values", ALL_VALUES, 0 },
+      { 0, 0, 0 }
+    };
 
   if (argc == 1)
-    name = argv[0];
+      print_values = PRINT_NO_VALUES;
+  else if (argc == 2 && re_exec(argv[0]))
+    {
+      optind = 1;
+      print_values = mi_parse_values_option (argv[0]);
+    }
   else
-    name = (argv[1]);
+    {
+      while (1)
+	{
+	  int opt = mi_getopt ("mi_cmd_var_update",
+			       argc, argv, opts, &optind, &optarg);
+	  if (opt < 0)
+	    break;
+	  switch ((enum opt) opt)
+	    {
+	    case NO_VALUES:
+	      print_values = PRINT_NO_VALUES;
+	      break;
+	    case SIMPLE_VALUES:
+	      print_values = PRINT_SIMPLE_VALUES;
+	      break;
+	    case ALL_VALUES:
+	      print_values = PRINT_ALL_VALUES;
+	      break;
+	    }
+	}
 
-  if (argc == 2)
-    print_values = mi_parse_values_option (argv[0]);
-  else
-    print_values = PRINT_NO_VALUES;
+      if (optind >= argc)
+	error (_("mi_cmd_var_update: Usage: [PRINT_VALUES] NAME1 NAME2 ..."));
+    }
+
+  name = argv[optind];
 
   if (mi_version (uiout) <= 1)
     cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, "changelist");
@@ -697,10 +737,15 @@ mi_cmd_var_update (char *command, char *
     }
   else
     {
-      /* Get varobj handle, if a valid var obj name was specified */
-      struct varobj *var = varobj_get_handle (name);
+      int index = optind;
+      while (argv[index])
+	{
+	  /* Get varobj handle, if a valid var obj name was specified */
+	  var = varobj_get_handle (argv[index]);
 
-      varobj_update_one (var, print_values, 1 /* explicit */);
+	  varobj_update_one (var, print_values, 1 /* explicit */);
+	  index++;
+	}
     }
 
   do_cleanups (cleanup);

Re: [PATCH:gdb/mi] Update a specified list of variable objects

[Nick Roberts]

 > I've tried to address this point below.  It's even in unified diff format!

Actually it's probably better to use isdigit like below


-- 
Nick                                           http://www.inet.net.nz/~nickrob


--- mi-cmd-var.c.~1.61~	2009-09-16 18:30:24.000000000 +1200
+++ mi-cmd-var.c	2009-09-22 11:59:40.000000000 +1200
@@ -660,19 +660,57 @@ mi_cmd_var_update (char *command, char *
   struct cleanup *cleanup;
   char *name;
   enum print_values print_values;
+  int optind = 0;
+  char *optarg;
+  struct varobj *var;
+
+  enum opt
+    {
+      NO_VALUES, SIMPLE_VALUES, ALL_VALUES
+    };
 
-  if (argc != 1 && argc != 2)
-    error (_("mi_cmd_var_update: Usage: [PRINT_VALUES] NAME."));
+  static struct mi_opt opts[] =
+  {
+      { "-no-values", NO_VALUES, 0 },
+      { "-simple-values", SIMPLE_VALUES, 0 },
+      { "-all-values", ALL_VALUES, 0 },
+      { 0, 0, 0 }
+    };
 
   if (argc == 1)
-    name = argv[0];
+      print_values = PRINT_NO_VALUES;
+  else if (argc == 2 && isdigit (*argv[0]))
+    {
+      optind = 1;
+      print_values = mi_parse_values_option (argv[0]);
+    }
   else
-    name = (argv[1]);
+    {
+      while (1)
+	{
+	  int opt = mi_getopt ("mi_cmd_var_update",
+			       argc, argv, opts, &optind, &optarg);
+	  if (opt < 0)
+	    break;
+	  switch ((enum opt) opt)
+	    {
+	    case NO_VALUES:
+	      print_values = PRINT_NO_VALUES;
+	      break;
+	    case SIMPLE_VALUES:
+	      print_values = PRINT_SIMPLE_VALUES;
+	      break;
+	    case ALL_VALUES:
+	      print_values = PRINT_ALL_VALUES;
+	      break;
+	    }
+	}
 
-  if (argc == 2)
-    print_values = mi_parse_values_option (argv[0]);
-  else
-    print_values = PRINT_NO_VALUES;
+      if (optind >= argc)
+	error (_("mi_cmd_var_update: Usage: [PRINT_VALUES] NAME1 NAME2 ..."));
+    }
+
+  name = argv[optind];
 
   if (mi_version (uiout) <= 1)
     cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, "changelist");
@@ -697,10 +735,15 @@ mi_cmd_var_update (char *command, char *
     }
   else
     {
-      /* Get varobj handle, if a valid var obj name was specified */
-      struct varobj *var = varobj_get_handle (name);
+      int index = optind;
+      while (argv[index])
+	{
+	  /* Get varobj handle, if a valid var obj name was specified */
+	  var = varobj_get_handle (argv[index]);
 
-      varobj_update_one (var, print_values, 1 /* explicit */);
+	  varobj_update_one (var, print_values, 1 /* explicit */);
+	  index++;
+	}
     }
 
   do_cleanups (cleanup);

Re: [PATCH:gdb/mi] Update a specified list of variable objects

[Vladimir Prus]

On Tuesday 22 September 2009 Nick Roberts wrote:

>  > I've tried to address this point below.  It's even in unified diff format!

Thanks!

> 
> Actually it's probably better to use isdigit like below

In fact, it's probably even easier than than. Note this code from
mi_cmd_var_create:

  if (strcmp (name, "-") == 0)
    {
      xfree (name);
      name = varobj_gen_name ();
    }
  else if (!isalpha (*name))
    error (_("mi_cmd_var_create: name of object must begin with a letter"));

with this in mind,


> +      print_values = PRINT_NO_VALUES;
> +  else if (argc == 2 && isdigit (*argv[0]))
> +    {
> +      optind = 1;
> +      print_values = mi_parse_values_option (argv[0]);
> +    }

can be written like this:

  else if (argc == 2 && (isdigit (*argv[0]) || *argv[0] == '-'))
    {
      optind = 1;
      print_values = mi_parse_values_option (argv[0]);
    }

>    else
> -    name = (argv[1]);
> +    {
> +      while (1)
> +       {
> +         int opt = mi_getopt ("mi_cmd_var_update",
> +                              argc, argv, opts, &optind, &optarg);

And this code can be dropped. What do you think?

- Volodya

Re: [PATCH:gdb/mi] Update a specified list of variable objects

[Nick Roberts]

 > > Actually it's probably better to use isdigit like below
 > 
 > In fact, it's probably even easier than than. Note this code from
 > mi_cmd_var_create:
 > 
 >   if (strcmp (name, "-") == 0)
 >     {
 >       xfree (name);
 >       name = varobj_gen_name ();
 >     }
 >   else if (!isalpha (*name))
 >     error (_("mi_cmd_var_create: name of object must begin with a letter"));
 > 
 > with this in mind,
 > 
 > 
 > > +      print_values = PRINT_NO_VALUES;
 > > +  else if (argc == 2 && isdigit (*argv[0]))
 > > +    {
 > > +      optind = 1;
 > > +      print_values = mi_parse_values_option (argv[0]);
 > > +    }
 > 
 > can be written like this:
 > 
 >   else if (argc == 2 && (isdigit (*argv[0]) || *argv[0] == '-'))
 >     {
 >       optind = 1;
 >       print_values = mi_parse_values_option (argv[0]);
 >     }

"-var-update --all-values var1 var2" doesn't work in this case.  I see
a couple of shortcomings with my patch too now:  "-var-update 0 var1 var2" doesn't work
and "-var-update" with no argument givese a segmentation fault.

 > >    else
 > > -    name = (argv[1]);
 > > +    {
 > > +      while (1)
 > > +       {
 > > +         int opt = mi_getopt ("mi_cmd_var_update",
 > > +                              argc, argv, opts, &optind, &optarg);
 > 
 > And this code can be dropped. What do you think?

This is a few lines shorter but what if someone wants to add another option?
mi_getopt provides an extensible solution.

I'm not really worried which approach is used.  Since the general idea seems
to be acceptable, I'll write some tests and documentation.  Then you can code
it a different way if you wish and the only requirement will be that the tests
pass.

-- 
Nick                                           http://users.snap.net.nz/~nickrob

Re: [PATCH:gdb/mi] Update a specified list of variable objects

[Tom Tromey]

>>>>> "Nick" == Nick Roberts <nickrob@snap.net.nz> writes:

Nick>           -var-update [PRINT_VALUES] NAME1 NAME2 ...

Two notes on this patch.

First, it needs a documentation change as well.

Nick> !       int index = optind;
Nick> !       while (argv[index]) {

Second, a nit: wrong brace placement.

Tom

Re: [PATCH:gdb/mi] Update a specified list of variable objects

[Nick Roberts]

Tom Tromey writes:
 > >>>>> "Nick" == Nick Roberts <nickrob@snap.net.nz> writes:
 > 
 > Nick>           -var-update [PRINT_VALUES] NAME1 NAME2 ...
 > 
 > Two notes on this patch.
 > 
 > First, it needs a documentation change as well.

I should have posted it as a RFC.  I've not run the testsuite or written any
tests either.  I wanted to test the water first but these will be forthcoming
if there's general agreement on the change.

 > Nick> !       int index = optind;
 > Nick> !       while (argv[index]) {
 > 
 > Second, a nit: wrong brace placement.

Hmm.  Too much Java.

-- 
Nick                                           http://www.inet.net.nz/~nickrob