Re: [PATCH] MI: Free values when updating

[Nick Roberts <nickrob@snap.net.nz>]

 > I don't understand this change at all. It means that if you have 
 > a reference variable then:
 > 
 >         1. For initial value, dereferenced value will be saved
 >         2. For second and subsequent values, the value of reference itself will
 >         be saved. 
 > 
 > This is exactly what the code block in question tries to prevent --
 > it tries to make sure we always store dereferenced value.
 >
 > When I try to run tests with this patch applied, I get:
 > 
 > Running ../.././gdb/testsuite/gdb.mi/mi-var-cmd.exp ...
 > ERROR: Couldn't send -var-create i  * i to GDB.
 > ERROR: Couldn't send -var-create l * l to GDB.
 > ERROR: Couldn't send -var-create linteger * linteger to GDB.
 > 
 > and many more errors like this. Did you run the testsuite after change?

OK, it's not the right patch but I do think that release_values shouldn't be
called when updating; it's just that this patch stops calling it at other times
when it's needed.  Without any change, do enable timings (if you have that
patch), create a variable object of a large array and all its children then
repeatedly do "-var-update *".  It should take longer and longer to execute.

Then try the patch below which does pass all the tests (just intended for
illustration not approval).

 > Can you explain where the memory leak comes from. At the end of the function I see:
 > 
 >     if (var->value != NULL)
 >        value_free (var->value);
 > 
 > and the value passed to function itself does not have release_value called on it,
 > so should be freed automatically.

I've not said it's a memory leak but it's computationally wasteful to
call release_value for each variable object when updating.


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


*** varobj.c	16 Jan 2007 18:34:59 +1300	1.79
--- varobj.c	23 Jan 2007 21:24:17 +1300	
*************** install_new_value (struct varobj *var, s
*** 917,923 ****
    /* We are not interested in the address of references, and given
       that in C++ a reference is not rebindable, it cannot
       meaningfully change.  So, get hold of the real value.  */
!   if (value)
      {
        value = coerce_ref (value);
        release_value (value);
--- 917,927 ----
    /* We are not interested in the address of references, and given
       that in C++ a reference is not rebindable, it cannot
       meaningfully change.  So, get hold of the real value.  */
!   if (initial == 2)
!     {
!       initial = 0;
!     }
!   else if (value)
      {
        value = coerce_ref (value);
        release_value (value);
*************** varobj_update (struct varobj **varp, str
*** 1117,1123 ****
        if (v != *varp)
  	{	  
  	  new = value_of_child (v->parent, v->index);
! 	  if (install_new_value (v, new, 0 /* type not changed */))
  	    {
  	      /* Note that it's changed */
  	      VEC_safe_push (varobj_p, result, v);
--- 1121,1127 ----
        if (v != *varp)
  	{	  
  	  new = value_of_child (v->parent, v->index);
! 	  if (install_new_value (v, new, 2 /* type not changed */))
  	    {
  	      /* Note that it's changed */
  	      VEC_safe_push (varobj_p, result, v);