RFC: MI - Detecting change of string contents with variable objects

RFC: MI - Detecting change of string contents with variable objects

[Nick Roberts]

This post follows on from a thread earlier this month on the GDB mailing list
called "memory address ranges (-var-create)"

Currently variable objects treat strings as pointers so -var-update only
detects a change of address or, if the child is created, when the first
character changes.  The patch below detects when the contents change which is
more useful.  I've only tested it for C, but I guess it could work for other
languages that variable objects handle (C++, Java).  The function
value_get_value gets both the address and string value but it's probably better
to just get the string value directly.

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


*** varobj.c	11 Dec 2006 08:13:03 +1300	1.65
--- varobj.c	18 Dec 2006 15:06:23 +1300	
*************** struct varobj
*** 101,106 ****
--- 101,107 ----
    /* The type of this variable. This may NEVER be NULL. */
    struct type *type;
  
+ 
    /* 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 varobj
*** 126,131 ****
--- 127,135 ----
  
    /* Was this variable updated via a varobj_set_value operation */
    int updated;
+ 
+   /* Last string value, if appropriate */
+   char *string_value;
  };
  
  /* Every variable keeps a linked list of its children, described
*************** static int variable_editable (struct var
*** 233,238 ****
--- 237,244 ----
  
  static char *my_value_of_variable (struct varobj *var);
  
+ static char *value_get_value (struct value* value);
+ 
  static int type_changeable (struct varobj *var);
  
  /* C implementation */
*************** install_new_value (struct varobj *var, s
*** 983,1003 ****
  	      
  	      if (!value_contents_equal (var->value, value))
  		changed = 1;
  	    }
  	}
      }
!     
    /* We must always keep the new value, since children depend on it.  */
    if (var->value != NULL)
      value_free (var->value);
    var->value = value;
    var->updated = 0;
!   
    gdb_assert (!var->value || value_type (var->value));
  
    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
--- 989,1025 ----
  	      
  	      if (!value_contents_equal (var->value, value))
  		changed = 1;
+ 
+ 	      if (variable_language (var) == vlang_c &&
+ 		  !strcmp (varobj_get_type (var), "char *"))
+ 		{
+ 		  if (var->string_value)
+ 		    {
+ 		      if (strcmp (var->string_value, value_get_value (value)))
+ 			{
+ 			  free (var->string_value);
+ 			  var->string_value =
+ 			    xstrdup (value_get_value (value));
+ 			  changed = 1;
+ 			}
+ 		    }
+ 		  else
+ 		    var->string_value = xstrdup (value_get_value (value));
+ 		}
  	    }
  	}
      }
! 
    /* We must always keep the new value, since children depend on it.  */
    if (var->value != NULL)
      value_free (var->value);
    var->value = value;
    var->updated = 0;
! 
    gdb_assert (!var->value || value_type (var->value));
  
    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
*************** new_variable (void)
*** 1470,1475 ****
--- 1492,1498 ----
    var->format = 0;
    var->root = NULL;
    var->updated = 0;
+   var->string_value = NULL;
  
    return var;
  }
*************** my_value_of_variable (struct varobj *var
*** 1785,1790 ****
--- 1808,1827 ----
    return (*var->root->lang->value_of_variable) (var);
  }
  
+ static char *
+ value_get_value (struct value* value)
+ {
+   long dummy;
+   struct ui_file *stb = mem_fileopen ();
+   struct cleanup *old_chain = make_cleanup_ui_file_delete (stb);
+   char *thevalue;
+ 	    
+   common_val_print (value, stb, 0, 1, 0, 0);
+   thevalue = ui_file_xstrdup (stb, &dummy);
+   do_cleanups (old_chain);
+   return thevalue;
+ }
+ 
  /* Return non-zero if changes in value of VAR
     must be detected and reported by -var-update.
     Return zero is -var-update should never report

Re: RFC: MI - Detecting change of string contents with variable objects

[Vladimir Prus]

Nick Roberts wrote:

> 
> This post follows on from a thread earlier this month on the GDB mailing
> list called "memory address ranges (-var-create)"

It looks like that thread did not reach a conclusion, though....

> 
> Currently variable objects treat strings as pointers so -var-update only
> detects a change of address or, if the child is created, when the first
> character changes.  The patch below detects when the contents change which
> is
> more useful.  I've only tested it for C, but I guess it could work for
> other
> languages that variable objects handle (C++, Java).  The function
> value_get_value gets both the address and string value but it's probably
> better to just get the string value directly.

I think this is probably a wrong thing to do in MI. Yes, this helps with
char*, but char* happens to be not so important in C++ -- modern code
mostly uses std::string (or QString, or gtkmm::ustring, or whatever). This
patch does not help with those, for the frontend is required to contain
special code to handle string classes. As as soon as it has such special
code, handling char* can be done in frontend as well. 

In fact, it looks like your patch only changes the behaviour for C --
you have:

          if (variable_language (var) == vlang_c &&

but I think we need to avoid special-casing C while not solving any problems
with C++. You mentioned that Insight handles char* just fine -- using
current MI code. What approach is take there?

On technical points:

1. Your value_get_value has no comments at all.
2. I don't see 'string_value' being freed in 'free_variable'


- Volodya

Re: RFC: MI - Detecting change of string contents with variable objects

[Nick Roberts]

Vladimir Prus writes:
 > Nick Roberts wrote:
 > 
 > > 
 > > This post follows on from a thread earlier this month on the GDB mailing
 > > list called "memory address ranges (-var-create)"
 > 
 > It looks like that thread did not reach a conclusion, though....

That's why I've continued it here.

 > > Currently variable objects treat strings as pointers so -var-update only
 > > detects a change of address or, if the child is created, when the first
 > > character changes.  The patch below detects when the contents change which
 > > is
 > > more useful.  I've only tested it for C, but I guess it could work for
 > > other
 > > languages that variable objects handle (C++, Java).  The function
 > > value_get_value gets both the address and string value but it's probably
 > > better to just get the string value directly.
 > 
 > I think this is probably a wrong thing to do in MI. Yes, this helps with
 > char*, but char* happens to be not so important in C++ -- modern code
 > mostly uses std::string (or QString, or gtkmm::ustring, or whatever). This
 > patch does not help with those, for the frontend is required to contain
 > special code to handle string classes. As as soon as it has such special
 > code, handling char* can be done in frontend as well. 

You seem to be saying that because it won't work generally for C++ it should
not be made to work for C.

 > In fact, it looks like your patch only changes the behaviour for C --
 > you have:
 > 
 >           if (variable_language (var) == vlang_c &&

Yes, sorry I was trying to say it only currently works for C.

 > but I think we need to avoid special-casing C while not solving any problems
 > with C++. 

I think it's better than nothing.  If you can think of a more general approach
that would be even better.

 >            You mentioned that Insight handles char* just fine -- using
 > current MI code. What approach is take there?

GDB is built into Insight as a single executable, it doesn't rely on
interprocess communication with the frontend.  It compares the displayed string
in the watch expression window with the current value.

 > On technical points:
 > 
 > 1. Your value_get_value has no comments at all.

Its based on c_value_of_variable but this is just a rough sketch.

 > 2. I don't see 'string_value' being freed in 'free_variable'

OK thanks, I hadn't noticed that.

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

Re: RFC: MI - Detecting change of string contents with variable objects

[Vladimir Prus]

On Monday 18 December 2006 11:10, Nick Roberts wrote:

>  > > Currently variable objects treat strings as pointers so -var-update only
>  > > detects a change of address or, if the child is created, when the first
>  > > character changes.  The patch below detects when the contents change which
>  > > is
>  > > more useful.  I've only tested it for C, but I guess it could work for
>  > > other
>  > > languages that variable objects handle (C++, Java).  The function
>  > > value_get_value gets both the address and string value but it's probably
>  > > better to just get the string value directly.
>  > 
>  > I think this is probably a wrong thing to do in MI. Yes, this helps with
>  > char*, but char* happens to be not so important in C++ -- modern code
>  > mostly uses std::string (or QString, or gtkmm::ustring, or whatever). This
>  > patch does not help with those, for the frontend is required to contain
>  > special code to handle string classes. As as soon as it has such special
>  > code, handling char* can be done in frontend as well. 
> 
> You seem to be saying that because it won't work generally for C++ it should
> not be made to work for C.

Right, because it would be bad to have, in any given frontend, two different solutions
for C and C++. 

>  > but I think we need to avoid special-casing C while not solving any problems
>  > with C++. 
> 
> I think it's better than nothing.  If you can think of a more general approach
> that would be even better.

First of all, what's the problem? The problem as I see is that for some types,
default comparison rules used by MI is not appropriate. This problem
can be solved either by:

	1. Having frontend grab the value on each step and do the comparison itself.
	2. Adding some 'comparison customization' to MI.

(2) might work like this:

	-var-set-comparator V "strcmp($a, $b) == 0"

then MI can set "$a" and "$b" to old and new value, and evaluate this
expression.

I'm not sure if (1) or (2) is better. (2) is slightly easier for frontend and it *might* reduce
the traffic between gdb and frontend.

But (2) has a serious problem -- for std::wstring and QString, frontend has to read
the data itself to present it to the user, since 

	-var-evaluate-expression 

returns nothing interesting for std::wstring and QString. This suggests that we need:

	-var-set-format-expression "................."

and the ellipsis part is a big problem. For QString, KDevelop does the following:

	$kdev_d=%1.d
	$kdev_s=$kdev_d.size
	$kdev_s= ($kdev_s > 0)? ($kdev_s > 100 ? 200 : 2*$kdev_s) : 0
        ($kdev_s>0) ? (*((char*)&$kdev_d.unicode[0])@$kdev_s) : \"\""

and for complex data structures things can get out of control -- I don't fancy writing
programs in gdb script language.

Imagine the most complex case: std::map. Should variable object detect changes
in objects of that kind by looking at all contained elements and comparing them?
Should formatting of std::map be done in gdb, or in frontend? 

If it's better be done in gdb, then I think we'd need Python binding, so that you can do:

	define_python_function kdevelop_format_std_map ...........

	-var-set-format-expression V "kdevelop_format_std_map($a)"

or something like that. But as I say, I don't yet sure such formatting should happen in gdb.
 
>  >            You mentioned that Insight handles char* just fine -- using
>  > current MI code. What approach is take there?
> 
> GDB is built into Insight as a single executable, it doesn't rely on
> interprocess communication with the frontend.  It compares the displayed string
> in the watch expression window with the current value.

Ah, ok.

- Volodya

Re: RFC: MI - Detecting change of string contents with variable objects

[Daniel Jacobowitz]

On Mon, Dec 18, 2006 at 11:36:02AM +0300, Vladimir Prus wrote:
> If it's better be done in gdb, then I think we'd need Python binding, so that you can do:
> 
> 	define_python_function kdevelop_format_std_map ...........
> 
> 	-var-set-format-expression V "kdevelop_format_std_map($a)"
> 
> or something like that. But as I say, I don't yet sure such formatting should happen in gdb.

I think it will end up being a Guile binding, and I was working on it
over the weekend.  Should a varobj always report changed if the way GDB
would display the value has changed, in which case the special casing
isn't necessary?

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: MI - Detecting change of string contents with variable objects

[Nick Roberts]

 >  Should a varobj always report changed if the way GDB
 > would display the value has changed, in which case the special casing
 > isn't necessary?

That's a vary good idea.  Here's a revised patch that isn't language dependent.
In addition to detecting string contents changes it detects when the output
format has changed with -var-set-format.  I don't see this as a bad thing
and it means that my patch earlier in the year for including the value
in the output of -var-set-format probably isn't needed.

It also means that value_contents_equal is not needed any longer, although I
guess there's no harm in keeping it.


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


2006-12-19  Nick Roberts  <nickrob@snap.net.nz>

	* varobj.c (struct varobj): New member print_value.
	(install_new_value): Compare last printed value with current one
	instead of contents.
	(new_variable): Initialize var->print_value to NULL.
	(free_variable): Free var->print_value.
	(value_get_print_value): New function derived from
	c_value_of_variable.


*** varobj.c	11 Dec 2006 08:13:03 +1300	1.65
--- varobj.c	19 Dec 2006 10:27:50 +1300	
*************** struct varobj
*** 101,106 ****
--- 101,107 ----
    /* The type of this variable. This may NEVER be NULL. */
    struct type *type;
  
+ 
    /* 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 varobj
*** 126,131 ****
--- 127,135 ----
  
    /* Was this variable updated via a varobj_set_value operation */
    int updated;
+ 
+   /* Last print value */
+   char *print_value;
  };
  
  /* Every variable keeps a linked list of its children, described
*************** static int variable_editable (struct var
*** 233,238 ****
--- 237,245 ----
  
  static char *my_value_of_variable (struct varobj *var);
  
+ static char *value_get_print_value (struct value *value,
+ 				    enum varobj_display_formats format);
+ 
  static int type_changeable (struct varobj *var);
  
  /* C implementation */
*************** install_new_value (struct varobj *var, s
*** 979,1003 ****
  	  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 the new value, since children depend on it.  */
    if (var->value != NULL)
      value_free (var->value);
    var->value = value;
    var->updated = 0;
!   
    gdb_assert (!var->value || value_type (var->value));
  
    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
--- 986,1020 ----
  	  else
  	    {
  	      gdb_assert (!value_lazy (var->value));
  	      
! 	      if (var->print_value)
! 		{
! 		  if (strcmp (var->print_value,
! 			      value_get_print_value (value, var->format)))
! 		    {
! 		      xfree (var->print_value);
! 		      var->print_value =
! 			xstrdup (value_get_print_value (value, var->format));
! 		      changed = 1;
! 		    }
! 		}
! 	      else
! 		var->print_value =
! 		  xstrdup (value_get_print_value (value, var->format));
  	    }
  	}
      }
! 
    /* We must always keep the new value, since children depend on it.  */
    if (var->value != NULL)
      value_free (var->value);
    var->value = value;
    var->updated = 0;
! 
    gdb_assert (!var->value || value_type (var->value));
  
    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
*************** new_variable (void)
*** 1470,1475 ****
--- 1487,1493 ----
    var->format = 0;
    var->root = NULL;
    var->updated = 0;
+   var->print_value = NULL;
  
    return var;
  }
*************** free_variable (struct varobj *var)
*** 1503,1508 ****
--- 1521,1527 ----
  
    xfree (var->name);
    xfree (var->obj_name);
+   xfree (var->print_value);
    xfree (var);
  }
  
*************** my_value_of_variable (struct varobj *var
*** 1785,1790 ****
--- 1804,1823 ----
    return (*var->root->lang->value_of_variable) (var);
  }
  
+ static char *
+ value_get_print_value (struct value *value, enum varobj_display_formats format)
+ {
+   long dummy;
+   struct ui_file *stb = mem_fileopen ();
+   struct cleanup *old_chain = make_cleanup_ui_file_delete (stb);
+   char *thevalue;
+ 	    
+   common_val_print (value, stb, format_code[(int) format], 1, 0, 0);
+   thevalue = ui_file_xstrdup (stb, &dummy);
+   do_cleanups (old_chain);
+   return thevalue;
+ }
+ 
  /* Return non-zero if changes in value of VAR
     must be detected and reported by -var-update.
     Return zero is -var-update should never report

Re: RFC: MI - Detecting change of string contents with variable objects

[Vladimir Prus]

Nick Roberts wrote:

>  >  Should a varobj always report changed if the way GDB
>  > would display the value has changed, in which case the special casing
>  > isn't necessary?
> 
> That's a vary good idea.  Here's a revised patch that isn't language
> dependent. In addition to detecting string contents changes it detects
> when the output
> format has changed with -var-set-format.  I don't see this as a bad thing
> and it means that my patch earlier in the year for including the value
> in the output of -var-set-format probably isn't needed.
> 
> It also means that value_contents_equal is not needed any longer, although
> I guess there's no harm in keeping it.

Why do you need the value_get_print_value function? I think that the right
semantic of -var-update is that it returns all such variable objects for
which the -var-evaluate-expression will return different value before and
after -var-update. -var-evaluate-expression is the only way to get a value
of varobj, so we should be using that, not some similar but different
function.

In order words, why can't you just call varobj_get_value instead of
introducing and calling a new function? 

- Volodya

Re: RFC: MI - Detecting change of string contents with variable objects

[Nick Roberts]

 > Why do you need the value_get_print_value function? I think that the right
 > semantic of -var-update is that it returns all such variable objects for
 > which the -var-evaluate-expression will return different value before and
 > after -var-update. -var-evaluate-expression is the only way to get a value
 > of varobj, so we should be using that, not some similar but different
 > function.
 > 
 > In order words, why can't you just call varobj_get_value instead of
 > introducing and calling a new function? 

Because -var-evaluate-expression uses varobj_get_value so they will always
return the same value?

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

Re: RFC: MI - Detecting change of string contents with variable objects

[Vladimir Prus]

On Friday 22 December 2006 01:23, Nick Roberts wrote:
>  > Why do you need the value_get_print_value function? I think that the right
>  > semantic of -var-update is that it returns all such variable objects for
>  > which the -var-evaluate-expression will return different value before and
>  > after -var-update. -var-evaluate-expression is the only way to get a value
>  > of varobj, so we should be using that, not some similar but different
>  > function.
>  > 
>  > In order words, why can't you just call varobj_get_value instead of
>  > introducing and calling a new function? 
> 
> Because -var-evaluate-expression uses varobj_get_value so they will always
> return the same value?

Then, there are two solutions:

1. Make c_value_of_variable and friends accept struct value as opposed to taking
struct varobj.
2. Extra the part of c_value_of_variable that you've based your function on
into a separate function. Make both c_value_of_variable and install_new_value
call the new function.

I'm not sure which approach you find better, but I don't think copy-pasting is a solution
to anything.

- Volodya

Re: RFC: MI - Detecting change of string contents with variable objects

[Nick Roberts]

 > > Because -var-evaluate-expression uses varobj_get_value so they will always
 > > return the same value?
 > 
 > Then, there are two solutions:

We seem to be agree that the patch does the right thing and are just talking
about implementation details.

 > 1. Make c_value_of_variable and friends accept struct value as opposed to
 > taking struct varobj.

c_value_of_variable, as it's name implies, requires a struct varobj as it's
argument.

 > 2. Extra the part of c_value_of_variable that you've based your function on
 > into a separate function. Make both c_value_of_variable and install_new_value
 > call the new function.

value_get_print_value is quite a small function.  After you've wrapped
some statements in if clauses and worked out how to call them with a
common argument, I can't see that you would gain much.

 > I'm not sure which approach you find better, but I don't think copy-pasting
 > is a solution to anything.

Clearly it is *a* solution, it's just a question of whether it's the *best*
solution in this case.  Anyway, it's not a straight copy-paste (the asserts are
removed, for example).  I think if you look through the code you'll find
numerous examples where one section of code is a slight variation of another.

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

Re: RFC: MI - Detecting change of string contents with variable objects

[Vladimir Prus]

On Friday 22 December 2006 10:12, Nick Roberts wrote:
>  > > Because -var-evaluate-expression uses varobj_get_value so they will always
>  > > return the same value?
>  > 
>  > Then, there are two solutions:
> 
> We seem to be agree that the patch does the right thing and are just talking
> about implementation details.

Yes.

> 
>  > 1. Make c_value_of_variable and friends accept struct value as opposed to
>  > taking struct varobj.
> 
> c_value_of_variable, as it's name implies, requires a struct varobj as it's
> argument.

You can renamed it to anything you want, say "c_format_value", or whatever.

>  > 2. Extra the part of c_value_of_variable that you've based your function on
>  > into a separate function. Make both c_value_of_variable and install_new_value
>  > call the new function.
> 
> value_get_print_value is quite a small function.  After you've wrapped
> some statements in if clauses and worked out how to call them with a
> common argument, I can't see that you would gain much.
> 
>  > I'm not sure which approach you find better, but I don't think copy-pasting
>  > is a solution to anything.
> 
> Clearly it is *a* solution, it's just a question of whether it's the *best*
> solution in this case.  Anyway, it's not a straight copy-paste (the asserts are
> removed, for example).  

Probably it's perfectionism, but if we want -var-update to return a varobj if the
result of -var-evaluate-expression would be different, then we must execute 
the same code as for -var-evaluate-expression when deciding if the value has changed.
If you run some other code, however similar *now*, it's very likely that soon things
will break.

> I think if you look through the code you'll find 
> numerous examples where one section of code is a slight variation of another.

Yes. I'd like to remove all such cases.

- Volodya

Re: RFC: MI - Detecting change of string contents with variable objects

[Daniel Jacobowitz]

On Tue, Dec 19, 2006 at 10:52:41AM +1300, Nick Roberts wrote:
> That's a vary good idea.  Here's a revised patch that isn't language dependent.
> In addition to detecting string contents changes it detects when the output
> format has changed with -var-set-format.  I don't see this as a bad thing
> and it means that my patch earlier in the year for including the value
> in the output of -var-set-format probably isn't needed.

Sounds good.  I generally agree with Vladimir about reusing
c_value_of_variable, however.

I believe it is as simple as:
  - Passing a struct value *.
  - Always returning NULL if there is no value, instead of doing
    the struct and array special cases first.
  - Using value_type to get the type code and the number of children.
  - Probably calling value_fetch_lazy first when passing it the new
    value, to avoid the assertions.

Let me know if you want me to try that myself.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: MI - Detecting change of string contents with variable objects

[Nick Roberts]

 > > That's a vary good idea.  Here's a revised patch that isn't language
 > > dependent.  In addition to detecting string contents changes it detects
 > > when the output format has changed with -var-set-format.  I don't see this
 > > as a bad thing and it means that my patch earlier in the year for
 > > including the value in the output of -var-set-format probably isn't
 > > needed.
 > 
 > Sounds good.  I generally agree with Vladimir about reusing
 > c_value_of_variable, however.
 > 
 > I believe it is as simple as:
 >   - Passing a struct value *.
 >   - Always returning NULL if there is no value, instead of doing
 >     the struct and array special cases first.
 >   - Using value_type to get the type code and the number of children.
 >   - Probably calling value_fetch_lazy first when passing it the new
 >     value, to avoid the assertions.

I've probably done it the other way round to how you're asking (used
value_get_print_value in c_value_of_variable) but, likewise, it involves
re-use. 

 > Let me know if you want me to try that myself.

If you approve what I've done that's fine, if not, perhaps you could show me
what you want.

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


2007-01-03  Nick Roberts  <nickrob@snap.net.nz>

	* varobj.c (struct varobj): New member print_value.
	(install_new_value): Compare last printed value with current one
	instead of contents.
	(new_variable): Initialize var->print_value to NULL.
	(free_variable): Free var->print_value.
	(value_get_print_value): New function derived from
	c_value_of_variable. 
	(c_value_of_variable): Use value_get_print_value.


*** varobj.c	04 Jan 2007 16:22:25 +1300	1.68
--- varobj.c	04 Jan 2007 17:00:45 +1300	
*************** struct varobj
*** 102,107 ****
--- 102,108 ----
    /* The type of this variable. This may NEVER be NULL. */
    struct type *type;
  
+ 
    /* 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 varobj
*** 127,132 ****
--- 128,136 ----
  
    /* Was this variable updated via a varobj_set_value operation */
    int updated;
+ 
+   /* Last print value */
+   char *print_value;
  };
  
  /* Every variable keeps a linked list of its children, described
*************** static int variable_editable (struct var
*** 234,239 ****
--- 238,246 ----
  
  static char *my_value_of_variable (struct varobj *var);
  
+ static char *value_get_print_value (struct value *value,
+ 				    enum varobj_display_formats format);
+ 
  static int type_changeable (struct varobj *var);
  
  /* C implementation */
*************** install_new_value (struct varobj *var, s
*** 979,1003 ****
  	  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 the new value, since children depend on it.  */
    if (var->value != NULL)
      value_free (var->value);
    var->value = value;
    var->updated = 0;
!   
    gdb_assert (!var->value || value_type (var->value));
  
    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
--- 986,1020 ----
  	  else
  	    {
  	      gdb_assert (!value_lazy (var->value));
  	      
! 	      if (var->print_value)
! 		{
! 		  if (strcmp (var->print_value,
! 			      value_get_print_value (value, var->format)))
! 		    {
! 		      xfree (var->print_value);
! 		      var->print_value =
! 			xstrdup (value_get_print_value (value, var->format));
! 		      changed = 1;
! 		    }
! 		}
! 	      else
! 		var->print_value =
! 		  xstrdup (value_get_print_value (value, var->format));
  	    }
  	}
      }
! 
    /* We must always keep the new value, since children depend on it.  */
    if (var->value != NULL)
      value_free (var->value);
    var->value = value;
    var->updated = 0;
! 
    gdb_assert (!var->value || value_type (var->value));
  
    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
*************** new_variable (void)
*** 1470,1475 ****
--- 1487,1493 ----
    var->format = 0;
    var->root = NULL;
    var->updated = 0;
+   var->print_value = NULL;
  
    return var;
  }
*************** free_variable (struct varobj *var)
*** 1503,1508 ****
--- 1521,1527 ----
  
    xfree (var->name);
    xfree (var->obj_name);
+   xfree (var->print_value);
    xfree (var);
  }
  
*************** my_value_of_variable (struct varobj *var
*** 1785,1790 ****
--- 1804,1823 ----
    return (*var->root->lang->value_of_variable) (var);
  }
  
+ static char *
+ value_get_print_value (struct value *value, enum varobj_display_formats format)
+ {
+   long dummy;
+   struct ui_file *stb = mem_fileopen ();
+   struct cleanup *old_chain = make_cleanup_ui_file_delete (stb);
+   char *thevalue;
+ 	    
+   common_val_print (value, stb, format_code[(int) format], 1, 0, 0);
+   thevalue = ui_file_xstrdup (stb, &dummy);
+   do_cleanups (old_chain);
+   return thevalue;
+ }
+ 
  /* Return non-zero if changes in value of VAR
     must be detected and reported by -var-update.
     Return zero is -var-update should never report
*************** c_value_of_variable (struct varobj *var)
*** 2153,2171 ****
  	  }
  	else
  	  {
- 	    long dummy;
- 	    struct ui_file *stb = mem_fileopen ();
- 	    struct cleanup *old_chain = make_cleanup_ui_file_delete (stb);
- 	    char *thevalue;
- 
  	    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);
! 	    do_cleanups (old_chain);
! 	return thevalue;
!       }
        }
      }
  }
--- 2186,2195 ----
  	  }
  	else
  	  {
  	    gdb_assert (type_changeable (var));
  	    gdb_assert (!value_lazy (var->value));
! 	    return value_get_print_value (var->value, var->format);
! 	  }
        }
      }
  }

Re: RFC: MI - Detecting change of string contents with variable objects

[Daniel Jacobowitz]

On Thu, Jan 04, 2007 at 05:13:36PM +1300, Nick Roberts wrote:
> I've probably done it the other way round to how you're asking (used
> value_get_print_value in c_value_of_variable) but, likewise, it involves
> re-use. 

Works out the same.  It looks reasonable to me.

>     struct type *type;
>   
> + 
>     /* The value of this expression or subexpression.  This may be NULL. 

Please try not to gratuitously move whitespace around (you
did this in a bunch of places).  I really appreciate that
when reviewing.

> ! 	      if (var->print_value)
> ! 		{
> ! 		  if (strcmp (var->print_value,
> ! 			      value_get_print_value (value, var->format)))
> ! 		    {
> ! 		      xfree (var->print_value);
> ! 		      var->print_value =
> ! 			xstrdup (value_get_print_value (value, var->format));
> ! 		      changed = 1;
> ! 		    }
> ! 		}
> ! 	      else
> ! 		var->print_value =
> ! 		  xstrdup (value_get_print_value (value, var->format));

value_get_print_value does a bunch of work, and returns a malloced
string.  So you should avoid calling it twice, and avoid leaking the
return value.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: MI - Detecting change of string contents with variable objects

[Nick Roberts]

 > >     struct type *type;
 > >   
 > > + 
 > >     /* The value of this expression or subexpression.  This may be NULL. 
 > 
 > Please try not to gratuitously move whitespace around (you
 > did this in a bunch of places).  I really appreciate that
 > when reviewing.

Well I try not to do it gratuitously but the above must have been a case of
sticky fingers.  The others seem sensible and hopefully not too distracting.

 > > ! 	      if (var->print_value)
 > > ! 		{
 > > ! 		  if (strcmp (var->print_value,
 > > ! 			      value_get_print_value (value, var->format)))
 > > ! 		    {
 > > ! 		      xfree (var->print_value);
 > > ! 		      var->print_value =
 > > ! 			xstrdup (value_get_print_value (value, var->format));
 > > ! 		      changed = 1;
 > > ! 		    }
 > > ! 		}
 > > ! 	      else
 > > ! 		var->print_value =
 > > ! 		  xstrdup (value_get_print_value (value, var->format));
 > 
 > value_get_print_value does a bunch of work, and returns a malloced
 > string.  So you should avoid calling it twice, and avoid leaking the
 > return value.

OK, I had thought the return value of value_get_print_value was placed
somewhere on the stack.  New attempt below.

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



*** varobj.c	04 Jan 2007 16:22:25 +1300	1.68
--- varobj.c	04 Jan 2007 19:04:25 +1300	
*************** struct varobj
*** 127,132 ****
--- 127,135 ----
  
    /* Was this variable updated via a varobj_set_value operation */
    int updated;
+ 
+   /* Last print value */
+   char *print_value;
  };
  
  /* Every variable keeps a linked list of its children, described
*************** static int variable_editable (struct var
*** 234,239 ****
--- 237,245 ----
  
  static char *my_value_of_variable (struct varobj *var);
  
+ static char *value_get_print_value (struct value *value,
+ 				    enum varobj_display_formats format);
+ 
  static int type_changeable (struct varobj *var);
  
  /* C implementation */
*************** install_new_value (struct varobj *var, s
*** 978,1003 ****
  	    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 the new value, since children depend on it.  */
    if (var->value != NULL)
      value_free (var->value);
    var->value = value;
    var->updated = 0;
!   
    gdb_assert (!var->value || value_type (var->value));
  
    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
--- 984,1017 ----
  	    changed = 1;
  	  else
  	    {
+ 	      char* print_value = value_get_print_value (value, var->format);
  	      gdb_assert (!value_lazy (var->value));
  	      
! 	      if (var->print_value)
! 		{
! 		  if (strcmp (var->print_value, print_value))
! 		    {
! 		      xfree (var->print_value);
! 		      var->print_value = print_value;
! 		      changed = 1;
! 		    }
! 		}
! 	      else
! 		var->print_value = print_value;
  	    }
  	}
      }
! 
    /* We must always keep the new value, since children depend on it.  */
    if (var->value != NULL)
      value_free (var->value);
    var->value = value;
    var->updated = 0;
! 
    gdb_assert (!var->value || value_type (var->value));
  
    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
*************** new_variable (void)
*** 1470,1475 ****
--- 1484,1490 ----
    var->format = 0;
    var->root = NULL;
    var->updated = 0;
+   var->print_value = NULL;
  
    return var;
  }
*************** free_variable (struct varobj *var)
*** 1503,1508 ****
--- 1518,1524 ----
  
    xfree (var->name);
    xfree (var->obj_name);
+   xfree (var->print_value);
    xfree (var);
  }
  
*************** my_value_of_variable (struct varobj *var
*** 1785,1790 ****
--- 1801,1820 ----
    return (*var->root->lang->value_of_variable) (var);
  }
  
+ static char *
+ value_get_print_value (struct value *value, enum varobj_display_formats format)
+ {
+   long dummy;
+   struct ui_file *stb = mem_fileopen ();
+   struct cleanup *old_chain = make_cleanup_ui_file_delete (stb);
+   char *thevalue;
+ 	    
+   common_val_print (value, stb, format_code[(int) format], 1, 0, 0);
+   thevalue = ui_file_xstrdup (stb, &dummy);
+   do_cleanups (old_chain);
+   return thevalue;
+ }
+ 
  /* Return non-zero if changes in value of VAR
     must be detected and reported by -var-update.
     Return zero is -var-update should never report
*************** c_value_of_variable (struct varobj *var)
*** 2153,2171 ****
  	  }
  	else
  	  {
- 	    long dummy;
- 	    struct ui_file *stb = mem_fileopen ();
- 	    struct cleanup *old_chain = make_cleanup_ui_file_delete (stb);
- 	    char *thevalue;
- 
  	    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);
! 	    do_cleanups (old_chain);
! 	return thevalue;
!       }
        }
      }
  }
--- 2183,2192 ----
  	  }
  	else
  	  {
  	    gdb_assert (type_changeable (var));
  	    gdb_assert (!value_lazy (var->value));
! 	    return value_get_print_value (var->value, var->format);
! 	  }
        }
      }
  }

Re: RFC: MI - Detecting change of string contents with variable objects

[Daniel Jacobowitz]

On Thu, Jan 04, 2007 at 07:10:25PM +1300, Nick Roberts wrote:
> + 	      char* print_value = value_get_print_value (value, var->format);

Use "char *print_value", please.

>   	      gdb_assert (!value_lazy (var->value));
>   	      
> ! 	      if (var->print_value)
> ! 		{
> ! 		  if (strcmp (var->print_value, print_value))
> ! 		    {
> ! 		      xfree (var->print_value);
> ! 		      var->print_value = print_value;
> ! 		      changed = 1;
> ! 		    }
> ! 		}
> ! 	      else
> ! 		var->print_value = print_value;

Should we set changed = 1 in the "else"?

Otherwise the patch seems fine, if it tests OK, but I'm still a little
nervous about it.  For example, you'll call val_print on a struct
or array to see if it's changed.  Depending on things like "set print
elements", that might not print out the whole string.  This is
probably a behavior change.  Is it a harmless one?  If it is, then
should we be sharing the code with c_value_of_variable that avoids
printing structs, unions, and arrays, and never mark them as changed
unless their types change?

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: MI - Detecting change of string contents with variable objects

[Nick Roberts]

 > > + 	      char* print_value = value_get_print_value (value, var->format);
 > 
 > Use "char *print_value", please.

OK.
 
 > >   	      gdb_assert (!value_lazy (var->value));
 > >   	      
 > > ! 	      if (var->print_value)
 > > ! 		{
 > > ! 		  if (strcmp (var->print_value, print_value))
 > > ! 		    {
 > > ! 		      xfree (var->print_value);
 > > ! 		      var->print_value = print_value;
 > > ! 		      changed = 1;
 > > ! 		    }
 > > ! 		}
 > > ! 	      else
 > > ! 		var->print_value = print_value;
 > 
 > Should we set changed = 1 in the "else"?

No.  This is only reached in the initial call to install_new_value i.e
-var-create.

 > Otherwise the patch seems fine, if it tests OK, but I'm still a little
 > nervous about it.  For example, you'll call val_print on a struct
 > or array to see if it's changed.  Depending on things like "set print
 > elements", that might not print out the whole string.  This is
 > probably a behavior change.  Is it a harmless one?  If it is, then
 > should we be sharing the code with c_value_of_variable that avoids
 > printing structs, unions, and arrays, and never mark them as changed
 > unless their types change?

The function val_print is already used for -var-evaluate-expression.  AFAICS
"set print elements" has no effect on variable objects, perhaps because
val_print is only called on the leaves but I can see that using it might
expose MI to the vagaries of CLI.  Currently, however, string changes don't
get reported at all, without the user changing configuration values.


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

Re: RFC: MI - Detecting change of string contents with variable objects

[Daniel Jacobowitz]

On Fri, Jan 05, 2007 at 09:34:53AM +1300, Nick Roberts wrote:
>  > Otherwise the patch seems fine, if it tests OK, but I'm still a little
>  > nervous about it.  For example, you'll call val_print on a struct
>  > or array to see if it's changed.  Depending on things like "set print
>  > elements", that might not print out the whole string.  This is
>  > probably a behavior change.  Is it a harmless one?  If it is, then
>  > should we be sharing the code with c_value_of_variable that avoids
>  > printing structs, unions, and arrays, and never mark them as changed
>  > unless their types change?
> 
> The function val_print is already used for -var-evaluate-expression.

Not in the case I was talking about.  -var-evaluate-expression calls
varobj_get_value, which calls c_value_of_variable, and will return
"{...}" for a struct.  Won't it?

> AFAICS "set print elements" has no effect on variable objects,
> perhaps because val_print is only called on the leaves but I can see
> that using it might expose MI to the vagaries of CLI.  Currently,
> however, string changes don't get reported at all, without the user
> changing configuration values.

"set print elements" will affect the output of character pointers, but
that's not really what I'm worried about - you're now going to print
out most of an array or struct when comparing print_value.

1. Can always set print_value to what c_value_of_variable would return?

2. Currently I believe we will mark an array varobj as updated in
-var-update if any of its children change.  I'm not sure if we'll do
that any more after your patch, e.g. if something beyond the limit of
"set print elements" changes.  So, do you think any front end relies on
the parent being marked updated if any of its children are?  Vlad,
any opinion?

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: MI - Detecting change of string contents with variable objects

[Vladimir Prus]

On Thursday 04 January 2007 23:50, Daniel Jacobowitz wrote:
> On Fri, Jan 05, 2007 at 09:34:53AM +1300, Nick Roberts wrote:
> >  > Otherwise the patch seems fine, if it tests OK, but I'm still a little
> >  > nervous about it.  For example, you'll call val_print on a struct
> >  > or array to see if it's changed.  Depending on things like "set print
> >  > elements", that might not print out the whole string.  This is
> >  > probably a behavior change.  Is it a harmless one?  If it is, then
> >  > should we be sharing the code with c_value_of_variable that avoids
> >  > printing structs, unions, and arrays, and never mark them as changed
> >  > unless their types change?
> > 
> > The function val_print is already used for -var-evaluate-expression.
> 
> Not in the case I was talking about.  -var-evaluate-expression calls
> varobj_get_value, which calls c_value_of_variable, and will return
> "{...}" for a struct.  Won't it?
> 
> > AFAICS "set print elements" has no effect on variable objects,
> > perhaps because val_print is only called on the leaves but I can see
> > that using it might expose MI to the vagaries of CLI.  Currently,
> > however, string changes don't get reported at all, without the user
> > changing configuration values.
> 
> "set print elements" will affect the output of character pointers, but
> that's not really what I'm worried about - you're now going to print
> out most of an array or struct when comparing print_value.
> 
> 1. Can always set print_value to what c_value_of_variable would return?
> 
> 2. Currently I believe we will mark an array varobj as updated in
> -var-update if any of its children change.  

We never mark array varobj as changed, I believe. array varobj is included
in -var-update output only if the type of the array changes, IIRC.

> I'm not sure if we'll do 
> that any more after your patch, e.g. if something beyond the limit of
> "set print elements" changes.  So, do you think any front end relies on
> the parent being marked updated if any of its children are?  Vlad,
> any opinion?

I think the code in question is never executed for structures or arrays -- only
for "changeable" values.

- Volodya

Re: RFC: MI - Detecting change of string contents with variable objects

[Nick Roberts]

 > > I'm not sure if we'll do that any more after your patch, e.g. if something
 > > beyond the limit of "set print elements" changes.  So, do you think any
 > > front end relies on the parent being marked updated if any of its children
 > > are?  Vlad, any opinion?
 > 
 > I think the code in question is never executed for structures or arrays --
 > only for "changeable" values.

I agree.  Below is my latest patch, incorporating some of your suggestions.


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


*** varobj.c	05 Jan 2007 13:16:30 +1300	1.74
--- varobj.c	05 Jan 2007 15:41:23 +1300	
*************** struct varobj
*** 132,137 ****
--- 132,140 ----
  
    /* Was this variable updated via a varobj_set_value operation */
    int updated;
+ 
+   /* Last print value.  */
+   char *print_value;
  };
  
  struct cpstack
*************** static int variable_editable (struct var
*** 206,211 ****
--- 209,217 ----
  
  static char *my_value_of_variable (struct varobj *var);
  
+ static char *value_get_print_value (struct value *value,
+ 				    enum varobj_display_formats format);
+ 
  static int varobj_value_is_changeable_p (struct varobj *var);
  
  static int is_root_p (struct varobj *var);
*************** install_new_value (struct varobj *var, s
*** 953,959 ****
    /* If the type is changeable, compare the old and the new values.
       If this is the initial assignment, we don't have any old value
       to compare with.  */
!   if (!initial && changeable)
      {
        /* If the value of the varobj was changed by -var-set-value, then the 
  	 value in the varobj and in the target is the same.  However, that value
--- 959,967 ----
    /* If the type is changeable, compare the old and the new values.
       If this is the initial assignment, we don't have any old value
       to compare with.  */
!   if (initial)
!     var->print_value = value_get_print_value (value, var->format);
!   else if (changeable)
      {
        /* If the value of the varobj was changed by -var-set-value, then the 
  	 value in the varobj and in the target is the same.  However, that value
*************** install_new_value (struct varobj *var, s
*** 974,999 ****
  	    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 the new value, since children depend on it.  */
    if (var->value != NULL)
      value_free (var->value);
    var->value = value;
    var->updated = 0;
!   
    gdb_assert (!var->value || value_type (var->value));
  
    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
--- 982,1013 ----
  	    changed = 1;
  	  else
  	    {
+ 	      char *print_value;
  	      gdb_assert (!value_lazy (var->value));
! 	      print_value = value_get_print_value (value, var->format);
! 
! 	      if (strcmp (var->print_value, print_value))
! 		{
! 		  xfree (var->print_value);
! 		  var->print_value = print_value;
! 		  changed = 1;
! 		}
! 	      else
! 		xfree (print_value);
  	    }
  	}
      }
! 
    /* We must always keep the new value, since children depend on it.  */
    if (var->value != NULL)
      value_free (var->value);
    var->value = value;
    var->updated = 0;
! 
    gdb_assert (!var->value || value_type (var->value));
  
    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
*************** new_variable (void)
*** 1376,1381 ****
--- 1390,1396 ----
    var->format = 0;
    var->root = NULL;
    var->updated = 0;
+   var->print_value = NULL;
  
    return var;
  }
*************** free_variable (struct varobj *var)
*** 1409,1414 ****
--- 1424,1430 ----
  
    xfree (var->name);
    xfree (var->obj_name);
+   xfree (var->print_value);
    xfree (var);
  }
  
*************** my_value_of_variable (struct varobj *var
*** 1666,1671 ****
--- 1682,1701 ----
    return (*var->root->lang->value_of_variable) (var);
  }
  
+ static char *
+ value_get_print_value (struct value *value, enum varobj_display_formats format)
+ {
+   long dummy;
+   struct ui_file *stb = mem_fileopen ();
+   struct cleanup *old_chain = make_cleanup_ui_file_delete (stb);
+   char *thevalue;
+ 	    
+   common_val_print (value, stb, format_code[(int) format], 1, 0, 0);
+   thevalue = ui_file_xstrdup (stb, &dummy);
+   do_cleanups (old_chain);
+   return thevalue;
+ }
+ 
  /* Return non-zero if changes in value of VAR
     must be detected and reported by -var-update.
     Return zero is -var-update should never report
*************** c_value_of_variable (struct varobj *var)
*** 2038,2056 ****
  	  }
  	else
  	  {
- 	    long dummy;
- 	    struct ui_file *stb = mem_fileopen ();
- 	    struct cleanup *old_chain = make_cleanup_ui_file_delete (stb);
- 	    char *thevalue;
- 
  	    gdb_assert (varobj_value_is_changeable_p (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);
! 	    do_cleanups (old_chain);
! 	return thevalue;
!       }
        }
      }
  }
--- 2068,2077 ----
  	  }
  	else
  	  {
  	    gdb_assert (varobj_value_is_changeable_p (var));
  	    gdb_assert (!value_lazy (var->value));
! 	    return value_get_print_value (var->value, var->format);
! 	  }
        }
      }
  }

Re: RFC: MI - Detecting change of string contents with variable objects

[Daniel Jacobowitz]

On Fri, Jan 05, 2007 at 05:46:32PM +1300, Nick Roberts wrote:
>  > > I'm not sure if we'll do that any more after your patch, e.g. if something
>  > > beyond the limit of "set print elements" changes.  So, do you think any
>  > > front end relies on the parent being marked updated if any of its children
>  > > are?  Vlad, any opinion?
>  > 
>  > I think the code in question is never executed for structures or arrays --
>  > only for "changeable" values.
> 
> I agree.  Below is my latest patch, incorporating some of your suggestions.

This looks fine to me if it's fine with Vlad.

>   	      gdb_assert (!value_lazy (var->value));
> ! 	      gdb_assert (!value_lazy (value));
> ! 	      
> ! 	      if (!value_contents_equal (var->value, value))
> ! 		changed = 1;

Why are you removing the second assert here?


-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: MI - Detecting change of string contents with variable objects

[Nick Roberts]

 > > I agree.  Below is my latest patch, incorporating some of your
 > > suggestions.
 > 
 > This looks fine to me if it's fine with Vlad.
 > 
 > >   	      gdb_assert (!value_lazy (var->value));
 > > ! 	      gdb_assert (!value_lazy (value));
 > > ! 	      
 > > ! 	      if (!value_contents_equal (var->value, value))
 > > ! 		changed = 1;
 > 
 > Why are you removing the second assert here?

Argh!  Vladimir had the same question, I thought I'd put it back.  I have now.
I've also used:
 
  strcmp (var->print_value, print_value) != 0

since that method seems to be the norm in varobj.c, and GDB generally.

I've committed this change, which you might not like, because I think Vladimir
is agreeable to it.  Of course, I'll make changes, including reversion, if
there are problems.

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

Re: RFC: MI - Detecting change of string contents with variable objects

[Vladimir Prus]

On Saturday 06 January 2007 00:53, Nick Roberts wrote:
>  > > I agree.  Below is my latest patch, incorporating some of your
>  > > suggestions.
>  > 
>  > This looks fine to me if it's fine with Vlad.
>  > 
>  > >   	      gdb_assert (!value_lazy (var->value));
>  > > ! 	      gdb_assert (!value_lazy (value));
>  > > ! 	      
>  > > ! 	      if (!value_contents_equal (var->value, value))
>  > > ! 		changed = 1;
>  > 
>  > Why are you removing the second assert here?
> 
> Argh!  Vladimir had the same question, I thought I'd put it back.  I have now.
> I've also used:
>  
>   strcmp (var->print_value, print_value) != 0
> 
> since that method seems to be the norm in varobj.c, and GDB generally.
> 
> I've committed this change, which you might not like, because I think Vladimir
> is agreeable to it.  Of course, I'll make changes, including reversion, if
> there are problems.

To clarify, I am still not comfortable about the change that's supposedly
needed only for 'char*' variables but yet affects variables of all possible
types -- especially given that we don't any mechanism to customize
display of other types yes. At the same time, we have enough time now to discover
any possible fallout, so we can try.

- Volodya

Re: RFC: MI - Detecting change of string contents with variable objects

[Daniel Jacobowitz]

On Sat, Jan 06, 2007 at 10:53:40AM +1300, Nick Roberts wrote:
> I've also used:
>  
>   strcmp (var->print_value, print_value) != 0
> 
> since that method seems to be the norm in varobj.c, and GDB generally.

Please, when you commit a patch, post it.  That means the final
version, including a ChangeLog entry.  This holds for obvious patches,
patches that you commit with minor changes, whatever.

> I've committed this change, which you might not like, because I think
> Vladimir is agreeable to it.  Of course, I'll make changes, including
> reversion, if there are problems.

I think I was pretty clear that approval was conditional on hearing
from Vlad.  You went ahead and committed it anyway.

You committed a patch without running the testsuite beforehand.  In
general you shouldn't even post patches without doing that.  We
all make this mistake periodically, but please try harder.

You committed a patch to move the select_frame call without, as far as
I can see from my weekend mail, even posting it.

Nick, I'm disappointed in your behavior.  I'm past caring whether
you are frustrated with the pace of progress; especially after I spent
most of an entire work day that I couldn't really afford to catch up
on patch review.  We have rules that contributors are expected to
follow.  In the future, please follow them.

Let's not back out this patch; we don't need more churn, we'll just fix
it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: MI - Detecting change of string contents with variable objects

[Nick Roberts]

 > > I've also used:
 > >  
 > >   strcmp (var->print_value, print_value) != 0
 > > 
 > > since that method seems to be the norm in varobj.c, and GDB generally.
 > 
 > Please, when you commit a patch, post it.  That means the final
 > version, including a ChangeLog entry.

I did this initially, but on re-reading CONTRIBUTE and MAINTAINERS I couldn't
see why (perhaps Andrew instructed me).  I stopped because I thought the posts
were very similar to the previous one and I felt I was creating too much noise
on gdb-patches.  Also the mailing list gdb-cvs, which global maintainers are
presumably subscribed to, provides this very information.

 >                                        This holds for obvious patches,
 > patches that you commit with minor changes, whatever.

I usually still do this.

 > > I've committed this change, which you might not like, because I think
 > > Vladimir is agreeable to it.  Of course, I'll make changes, including
 > > reversion, if there are problems.
 > 
 > I think I was pretty clear that approval was conditional on hearing
 > from Vlad.  You went ahead and committed it anyway.

You said "This looks fine to me if it's fine with Vlad."  He was part of the
thread and I addressed his last reservation.  It's not clear to me, at least,
that he needs to explicitly express his approval.

 > You committed a patch without running the testsuite beforehand.  In
 > general you shouldn't even post patches without doing that.  We
 > all make this mistake periodically, but please try harder.

Yes, this is my mistake - sorry.  I'll have to improve the way I handle
multiple patches.

 > You committed a patch to move the select_frame call without, as far as
 > I can see from my weekend mail, even posting it.

My mistake again - sorry.

 > Nick, I'm disappointed in your behavior.  I'm past caring whether
 > you are frustrated with the pace of progress; especially after I spent
 > most of an entire work day that I couldn't really afford to catch up
 > on patch review.  We have rules that contributors are expected to
 > follow.  In the future, please follow them.

All I can say is that I'm probably trying harder than you think.

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

Re: RFC: MI - Detecting change of string contents with variable objects

[Daniel Jacobowitz]

On Tue, Jan 09, 2007 at 10:30:35AM +1300, Nick Roberts wrote:
> I did this initially, but on re-reading CONTRIBUTE and MAINTAINERS I couldn't
> see why (perhaps Andrew instructed me).  I stopped because I thought the posts
> were very similar to the previous one and I felt I was creating too much noise
> on gdb-patches.  Also the mailing list gdb-cvs, which global maintainers are
> presumably subscribed to, provides this very information.

You're right.  CONTRIBUTE needs updates.  I will put this on my TODO
list for this week.

The current policy, to the best of my knowledge, is supposed to be
"nothing should be checked in that isn't posted to gdb-patches". I
don't repost patches if they are unchanged from the last posted
version, but I do if I've adjusted them.

I'm open to changing it if folks want, but gdb-cvs is much less
convenient - diffs are not included and it isn't a discussion list
so you can't reply.

Anyone have other folklore items they want added to CONTRIBUTE?
Now's the time!  I may adjust the text on testing also.

>  > > I've committed this change, which you might not like, because I think
>  > > Vladimir is agreeable to it.  Of course, I'll make changes, including
>  > > reversion, if there are problems.
>  > 
>  > I think I was pretty clear that approval was conditional on hearing
>  > from Vlad.  You went ahead and committed it anyway.
> 
> You said "This looks fine to me if it's fine with Vlad."  He was part of the
> thread and I addressed his last reservation.  It's not clear to me, at least,
> that he needs to explicitly express his approval.

Sorry if I was unclear.  I would not have said it if Vlad's prior
discussion in the thread had been sufficient - I meant to wait for
him.

> Yes, this is my mistake - sorry.  I'll have to improve the way I handle
> multiple patches.

Thanks.  I realize this is hard, but for this exact reason it is very
important.  And it's usually not a good use of reviewer time to review
patches that cause failures; it is sometimes, so don't take this as an
ironclad rule, but the exceptions should generally be noted in
postings.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: MI - Detecting change of string contents with variable objects

[Vladimir Prus]

On Thursday 04 January 2007 23:34, Nick Roberts wrote:
>  > > + 	      char* print_value = value_get_print_value (value, var->format);
>  > 
>  > Use "char *print_value", please.
> 
> OK.
>  
>  > >   	      gdb_assert (!value_lazy (var->value));
>  > >   	      
>  > > ! 	      if (var->print_value)
>  > > ! 		{
>  > > ! 		  if (strcmp (var->print_value, print_value))
>  > > ! 		    {
>  > > ! 		      xfree (var->print_value);
>  > > ! 		      var->print_value = print_value;
>  > > ! 		      changed = 1;
>  > > ! 		    }
>  > > ! 		}
>  > > ! 	      else
>  > > ! 		var->print_value = print_value;
>  > 
>  > Should we set changed = 1 in the "else"?
> 
> No.  This is only reached in the initial call to install_new_value i.e
> -var-create.

How so? For -var-create the 'initial' parameter to install_new_value should be
1, so we have:

   if (!initial && changeable)
    {
       ................

  	      gdb_assert (!value_lazy (var->value));

              if (var->print_value)
               {
                 if (strcmp (var->print_value, print_value))
                   {
                     xfree (var->print_value);
                     var->print_value = print_value;
                     changed = 1;
                   }
               }
             else
               var->print_value = print_value;      
    }

It seems to be this code is *never* reached during call from -var-create.
In fact, I don't seem to understand how var->print_value will be initialized
by -var-create. 


>  > Otherwise the patch seems fine, if it tests OK, but I'm still a little
>  > nervous about it.  For example, you'll call val_print on a struct
>  > or array to see if it's changed.  Depending on things like "set print
>  > elements", that might not print out the whole string.  This is
>  > probably a behavior change.  Is it a harmless one?  If it is, then
>  > should we be sharing the code with c_value_of_variable that avoids
>  > printing structs, unions, and arrays, and never mark them as changed
>  > unless their types change?
> 
> The function val_print is already used for -var-evaluate-expression.  AFAICS
> "set print elements" has no effect on variable objects, perhaps because
> val_print is only called on the leaves but I can see that using it might
> expose MI to the vagaries of CLI.  Currently, however, string changes don't
> get reported at all, without the user changing configuration values.

The code in question is executed only for changeable values, and so will never
be executed for structures.

- Volodya

Re: RFC: MI - Detecting change of string contents with variable objects

[Nick Roberts]

 > It seems to be this code is *never* reached during call from -var-create.
 > In fact, I don't seem to understand how var->print_value will be initialized
 > by -var-create. 

OK, it was initialised after the first -var-update.  I've changed this.

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

Re: RFC: MI - Detecting change of string contents with variable objects

[Vladimir Prus]

On Thursday 04 January 2007 09:10, Nick Roberts wrote:


> *** varobj.c    04 Jan 2007 16:22:25 +1300      1.68
> --- varobj.c    04 Jan 2007 19:04:25 +1300      
> *************** struct varobj
> *** 127,132 ****
> --- 127,135 ----
>   
>     /* Was this variable updated via a varobj_set_value operation */
>     int updated;
> + 
> +   /* Last print value */

Dot and two spaces ;-)

> +   char *print_value;
>   };
>   
>   /* Every variable keeps a linked list of its children, described
> *************** static int variable_editable (struct var
> *** 234,239 ****
> --- 237,245 ----
>   
>   static char *my_value_of_variable (struct varobj *var);
>   
> + static char *value_get_print_value (struct value *value,
> +                                   enum varobj_display_formats format);
> + 

I don't see a comment to this function either here or at the definition point.

>   static int type_changeable (struct varobj *var);
>   
>   /* C implementation */
> *************** install_new_value (struct varobj *var, s
> *** 978,1003 ****
>             changed = 1;
>           else
>             {
>               gdb_assert (!value_lazy (var->value));
> -             gdb_assert (!value_lazy (value));

Did you remove this because it failed? If so, I'd like to understand why.
I think this assert is needed -- if the value is lazy, then even if
printing code will fetch the value, you'll be comparing current value with
current value. That's a definite bug, so must be asserted.

>               
> !             if (!value_contents_equal (var->value, value))
> !               changed = 1;
>             }
>         }
>       }
> !     
>     /* We must always keep the new value, since children depend on it.  */
>     if (var->value != NULL)
>       value_free (var->value);
>     var->value = value;
>     var->updated = 0;
> !   
>     gdb_assert (!var->value || value_type (var->value));

Is that a formatting change above?

>   /* Update the values for a variable and its children.  This is a
>      two-pronged attack.  First, re-parse the value for the root's
> --- 984,1017 ----
>             changed = 1;
>           else
>             {
> +             char* print_value = value_get_print_value (value, var->format);
>               gdb_assert (!value_lazy (var->value));
>               
> !             if (var->print_value)
> !               {
> !                 if (strcmp (var->print_value, print_value))

Can you use 

        strcmp (var->print_value, print_value) != 0

for legibility?

> !                   {
> !                     xfree (var->print_value);
> !                     var->print_value = print_value;
> !                     changed = 1;
> !                   }

So, if values differ you xfree the old one and assign the new one. If the
values are the same -- where is 'print_value' freed?

- Volodya

Re: RFC: MI - Detecting change of string contents with variable objects

[Nick Roberts]

 > > +   /* Last print value */
 > 
 > Dot and two spaces ;-)

OK 

 >...
 > > + static char *value_get_print_value (struct value *value,
 > > +                                   enum varobj_display_formats format);
 > > + 
 > 
 > I don't see a comment to this function either here or at the definition
 > point.

Too many comments get in the way of the code and I think the name is fairly
self explanatory.  Most of the simple functions don't have a comment and AFAIK
a comment for them is not a GNU or GDB coding standard.

 > >   static int type_changeable (struct varobj *var);
 > >   
 > >   /* C implementation */
 > > *************** install_new_value (struct varobj *var, s
 > > *** 978,1003 ****
 > >             changed = 1;
 > >           else
 > >             {
 > >               gdb_assert (!value_lazy (var->value));
 > > -             gdb_assert (!value_lazy (value));
 > 
 > Did you remove this because it failed? If so, I'd like to understand why.
 > I think this assert is needed -- if the value is lazy, then even if
 > printing code will fetch the value, you'll be comparing current value with
 > current value. That's a definite bug, so must be asserted.

I removed it accidentally.  I've put it back.

 > >               
 > > !             if (!value_contents_equal (var->value, value))
 > > !               changed = 1;
 > >             }
 > >         }
 > >       }
 > > !     
 > >     /* We must always keep the new value, since children depend on it.  */
 > >     if (var->value != NULL)
 > >       value_free (var->value);
 > >     var->value = value;
 > >     var->updated = 0;
 > > !   
 > >     gdb_assert (!var->value || value_type (var->value));
 > 
 > Is that a formatting change above?

I've just removed unnecessay spaces.  The real change is replacing
value_contents_equal (but what's with all the underscores!).

 > > !                 if (strcmp (var->print_value, print_value))
 > 
 > Can you use 
 > 
 >         strcmp (var->print_value, print_value) != 0

Is that more legible?  I sometimes see "if (fi != NULL)" but "if (fi)"
seems clearer.  Maybe it comes from programming in Lisp for Emacs.

 > for legibility?
 > 
 > > !                   {
 > > !                     xfree (var->print_value);
 > > !                     var->print_value = print_value;
 > > !                     changed = 1;
 > > !                   }
 > 
 > So, if values differ you xfree the old one and assign the new one. If the
 > values are the same -- where is 'print_value' freed?

It's not; it's a legacy of earlier code.  I'll change it.

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

Re: RFC: MI - Detecting change of string contents with variable objects

[Daniel Jacobowitz]

On Fri, Jan 05, 2007 at 02:09:30PM +1300, Nick Roberts wrote:
>  > Can you use 
>  > 
>  >         strcmp (var->print_value, print_value) != 0
> 
> Is that more legible?  I sometimes see "if (fi != NULL)" but "if (fi)"
> seems clearer.  Maybe it comes from programming in Lisp for Emacs.

Speaking only for myself, I have no preference between "if (fi !=
NULL)" and "if (fi)", but I always use "!= 0" with strcmp; that's just
because strcmp's return value is so frequently misused. The != 0 makes
it clear that someone knew what result they were checking for.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: MI - Detecting change of string contents with variable objects

[Vladimir Prus]

On Friday 05 January 2007 17:44, Daniel Jacobowitz wrote:
> On Fri, Jan 05, 2007 at 02:09:30PM +1300, Nick Roberts wrote:
> >  > Can you use 
> >  > 
> >  >         strcmp (var->print_value, print_value) != 0
> > 
> > Is that more legible?  I sometimes see "if (fi != NULL)" but "if (fi)"
> > seems clearer.  Maybe it comes from programming in Lisp for Emacs.
> 
> Speaking only for myself, I have no preference between "if (fi !=
> NULL)" and "if (fi)", but I always use "!= 0" with strcmp; that's just
> because strcmp's return value is so frequently misused. The != 0 makes
> it clear that someone knew what result they were checking for.

Yeah, I've learn that for strings

          a  [comparison operator] b

is actually written

	strcmp (a, b) [the same comparison operator] 0

and this conveniention works great, while with "!strcmp" I always need
to mentally figure out what's the result.

And "if (fi != NULL)" vs. "if (fi)" is much more a matter of personal taste
than a legibility matter.

- Volodya

Re: RFC: MI - Detecting change of string contents with variable objects

[Jim Blandy]

Nick Roberts <nickrob@snap.net.nz> writes:
> Too many comments get in the way of the code and I think the name is fairly
> self explanatory.  Most of the simple functions don't have a comment and AFAIK
> a comment for them is not a GNU or GDB coding standard.

I see this differently.  I find comments above functions explaining
their interface, as opposed to their strategy for implementing that
interface, to be a real help.  I'm ready to admit I may be chattier
than is helpful within function bodies, but leaving interfaces
unexplained detracts from their utility.