[PATCH] MI: Free values when updating

[PATCH] MI: Free values when updating

[Nick Roberts]

The recent changes to varobj.c have resulted in values computed with
-var-update not being freed automatically.  This makes computation longer and
progressively so as currently free_all_values doesn't always get called.

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


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

	* varobj.c (install_new_value): Don't call release_value when
	updating.


*** varobj.c	16 Jan 2007 18:34:59 +1300	1.79
--- varobj.c	23 Jan 2007 18:26:57 +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,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 (initial && value)
      {
        value = coerce_ref (value);
        release_value (value);

Re: [PATCH] MI: Free values when updating

[Vladimir Prus]

Nick Roberts wrote:

> The recent changes to varobj.c have resulted in values computed with
> -var-update not being freed automatically.  This makes computation longer
> and progressively so as currently free_all_values doesn't always get
> called.
> 
> Nick                                          
> http://www.inet.net.nz/~nickrob
> 
> 
> 2007-01-23  Nick Roberts  <nickrob@snap.net.nz>
> 
> * varobj.c (install_new_value): Don't call release_value when
> updating.
> 
> 
> *** varobj.c    16 Jan 2007 18:34:59 +1300      1.79
> --- varobj.c    23 Jan 2007 18:26:57 +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,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 (initial && value)

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?

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.

- Volodya

Re: [PATCH] MI: Free values when updating

[Nick Roberts]

 > 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);

Re: [PATCH] MI: Free values when updating

[Vladimir Prus]

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

On Tuesday 23 January 2007 11:55, Nick Roberts wrote:
>  > 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; 

Why? If we're about to store a value in varobj->value, we must call release_value
on that value,  otherwise the value is going to be deleted at a random point in time,
and our varobj will be rather useless.

> 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.

Why? Is it because the memory consumption of gdb grows, or because the list
of released values grows without ever being cleared, or for some other reason?

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

Could you perhaps try the attached one, which I think is ready for checkin.
The install_new_value function documents that the incoming value should not be
released yet, in one place that requirement was not met. No regressions on x86.

>  > 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.

As I've said, we need to do it to avoid the value to be deleted at random point in time.
So there must be something else? Does you free_values patch makes any difference here?

- Volodya

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

Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.79
diff -u -p -r1.79 varobj.c
--- varobj.c	16 Jan 2007 02:12:49 -0000	1.79
+++ varobj.c	23 Jan 2007 09:12:07 -0000
@@ -1987,11 +1987,7 @@ c_value_of_root (struct varobj **var_han
       /* We need to catch errors here, because if evaluate
          expression fails we just want to make val->error = 1 and
          go on */
-      if (gdb_evaluate_expression (var->root->exp, &new_val))
-	{
-	  release_value (new_val);
-	}
-
+      gdb_evaluate_expression (var->root->exp, &new_val);
       return new_val;
     }
 

Re: [PATCH] MI: Free values when updating

[Nick Roberts]

 > > OK, it's not the right patch but I do think that release_values shouldn't
 > > be called when updating;
 > 
 > Why? If we're about to store a value in varobj->value, we must call
 > release_value on that value, otherwise the value is going to be deleted at a
 > random point in time, and our varobj will be rather useless.

Because there wasn't a call there before your changes.

 > > 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.
 > 
 > Why? Is it because the memory consumption of gdb grows, or because the list
 > of released values grows without ever being cleared, or for some other
 > reason?

The latter, I think.

 >...
 > As I've said, we need to do it to avoid the value to be deleted at random
 > point in time.  So there must be something else? Does you free_values patch
 > makes any difference here?

I think the permanence of values are already guaranteed through value_of_root
and value_of_child.  Maybe trying to release them the second time takes
longer because GDB has to go all the way through the list to find out they're
not there.


 > - Volodya
 > Index: varobj.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/varobj.c,v
 > retrieving revision 1.79
 > diff -u -p -r1.79 varobj.c
 > --- varobj.c	16 Jan 2007 02:12:49 -0000	1.79
 > +++ varobj.c	23 Jan 2007 09:12:07 -0000
 > @@ -1987,11 +1987,7 @@ c_value_of_root (struct varobj **var_han
 >        /* We need to catch errors here, because if evaluate
 >           expression fails we just want to make val->error = 1 and
 >           go on */

This comment is not applicable anymore.

 > -      if (gdb_evaluate_expression (var->root->exp, &new_val))
 > -	{
 > -	  release_value (new_val);
 > -	}
 > -
 > +      gdb_evaluate_expression (var->root->exp, &new_val);
 >        return new_val;
 >      }

I think if you also remove the (3) calls to release_value in c_value_of_child
and cplus_value_of_child this is equivalent to my change (and more tidy).


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

Re: [PATCH] MI: Free values when updating

[Daniel Jacobowitz]

On Wed, Jan 24, 2007 at 12:02:12AM +1300, Nick Roberts wrote:
>  > > 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.
>  > 
>  > Why? Is it because the memory consumption of gdb grows, or because the list
>  > of released values grows without ever being cleared, or for some other
>  > reason?
> 
> The latter, I think.

Except that there isn't a list of released values.  So what is GDB
doing that is taking longer and longer?

The call to release_value does a linear walk over all non-released
values.  So if we have a lot of things which aren't being released,
then your patch which calls free_all_values is probably the right thing
to do - that should clean it up.

>  > -      if (gdb_evaluate_expression (var->root->exp, &new_val))
>  > -	{
>  > -	  release_value (new_val);
>  > -	}
>  > -
>  > +      gdb_evaluate_expression (var->root->exp, &new_val);
>  >        return new_val;
>  >      }
> 
> I think if you also remove the (3) calls to release_value in c_value_of_child
> and cplus_value_of_child this is equivalent to my change (and more tidy).

No, those are different.  They come from things like the call to
gdb_value_ind in c_describe_child.  That creates a new value, which is
returned to the caller (the MI front end, to be printed and later
released).  It's the ones in c_value_of_root which matter, because we
save them in the varobj.

You're probably right about the increasing time though - releasing
something already released will be slow.  I wonder if we should make
that an internal error somehow.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] MI: Free values when updating

[Nick Roberts]

 > >  > Why? Is it because the memory consumption of gdb grows, or because the
 > >  > list of released values grows without ever being cleared, or for some
 > >  > other reason?
 > > 
 > > The latter, I think.
 > 
 > Except that there isn't a list of released values.  So what is GDB
 > doing that is taking longer and longer?

The list of *unreleased* values (all_values?) grows.  That should change
now free_all_values gets called from mi_execute_command.

 > > I think if you also remove the (3) calls to release_value in
 > > c_value_of_child and cplus_value_of_child this is equivalent to my change
 > > (and more tidy).
 > 
 > No, those are different.  They come from things like the call to
 > gdb_value_ind in c_describe_child.  That creates a new value, which is
 > returned to the caller (the MI front end, to be printed and later
 > released).  It's the ones in c_value_of_root which matter, because we
 > save them in the varobj.

In varobj_update:

	  new = value_of_child (v->parent, v->index);
	  if (install_new_value (v, new, 0 /* type not changed */))

In create_child:

  value = value_of_child (parent, index);
  ...
  install_new_value (child, value, 1);

Now install_new_value calls release_value on new, it's not neeeded in
*_value_of_child.  I've tried this change on the MI testsuite and see no fails
and it has about the same time improvement that my patch had.  If this is not
the right patch then the testsuite is lacking the appropriate test.  Can you
create a test where this patch fails?

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

Re: [PATCH] MI: Free values when updating

[Vladimir Prus]

On Wednesday 24 January 2007 00:19, Nick Roberts wrote:

>  > > I think if you also remove the (3) calls to release_value in
>  > > c_value_of_child and cplus_value_of_child this is equivalent to my change
>  > > (and more tidy).
>  > 
>  > No, those are different.  They come from things like the call to
>  > gdb_value_ind in c_describe_child.  That creates a new value, which is
>  > returned to the caller (the MI front end, to be printed and later
>  > released).  It's the ones in c_value_of_root which matter, because we
>  > save them in the varobj.
> 
> In varobj_update:
> 
> 	  new = value_of_child (v->parent, v->index);
> 	  if (install_new_value (v, new, 0 /* type not changed */))
> 
> In create_child:
> 
>   value = value_of_child (parent, index);
>   ...
>   install_new_value (child, value, 1);
> 
> Now install_new_value calls release_value on new, it's not neeeded in
> *_value_of_child.  

I think that's right. All calls to release_value except in install_new_value can be
removed I believe, and I'll post a patch to that effect tomorrow.

> I've tried this change on the MI testsuite and see no fails 
> and it has about the same time improvement that my patch had.  If this is not
> the right patch then the testsuite is lacking the appropriate test.  Can you
> create a test where this patch fails?

You cannot. For non-reference types, release_value gets called twice, which is
benign now. I'll try adding assert in release_value, though.

For reference and array types, extra release_value gives you a memleak, and 
there's nothing that will cause a test to fail due to that.

- Volodya

Re: [PATCH] MI: Free values when updating

[Vladimir Prus]

Nick Roberts wrote:

>  > Index: varobj.c
>  > ===================================================================
>  > RCS file: /cvs/src/src/gdb/varobj.c,v
>  > retrieving revision 1.79
>  > diff -u -p -r1.79 varobj.c
>  > --- varobj.c       16 Jan 2007 02:12:49 -0000      1.79
>  > +++ varobj.c       23 Jan 2007 09:12:07 -0000
>  > @@ -1987,11 +1987,7 @@ c_value_of_root (struct varobj **var_han
>  >        /* We need to catch errors here, because if evaluate
>  >           expression fails we just want to make val->error = 1 and
>  >           go on */
> 
> This comment is not applicable anymore.

It actually is -- the comment says why we use gdb_evaluate_expression,
as opposed to evaluate_expression. Only the part about val->error is obsolete,
and I'll fix that.

- Volodya

Re: [PATCH] MI: Free values when updating

[Nick Roberts]

 > >  > @@ -1987,11 +1987,7 @@ c_value_of_root (struct varobj **var_han
 > >  >        /* We need to catch errors here, because if evaluate
 > >  >           expression fails we just want to make val->error = 1 and
 > >  >           go on */
 > > 
 > > This comment is not applicable anymore.
 > 
 > It actually is -- the comment says why we use gdb_evaluate_expression,
 > as opposed to evaluate_expression. Only the part about val->error is obsolete,
 > and I'll fix that.

I'm not even sure that current use of gdb_evaluate_expression with variable
objects is sensible.  Currently GDB accepts:

  -var-create - * 1/0

and if you do:

  -var-create - * n1/n2

and n2 is set to 0,  with "-var-update --all-values", GDB returns:

  ^done,changelist=[{name="var2",in_scope="false"}]


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

Re: [PATCH] MI: Free values when updating

[Vladimir Prus]

On Wednesday 24 January 2007 12:14, Nick Roberts wrote:
>  > >  > @@ -1987,11 +1987,7 @@ c_value_of_root (struct varobj **var_han
>  > >  >        /* We need to catch errors here, because if evaluate
>  > >  >           expression fails we just want to make val->error = 1 and
>  > >  >           go on */
>  > > 
>  > > This comment is not applicable anymore.
>  > 
>  > It actually is -- the comment says why we use gdb_evaluate_expression,
>  > as opposed to evaluate_expression. Only the part about val->error is obsolete,
>  > and I'll fix that.
> 
> I'm not even sure that current use of gdb_evaluate_expression with variable
> objects is sensible.  Currently GDB accepts:
> 
>   -var-create - * 1/0
> 
> and if you do:
> 
>   -var-create - * n1/n2
> 
> and n2 is set to 0,  with "-var-update --all-values", GDB returns:
> 
>   ^done,changelist=[{name="var2",in_scope="false"}]

Yes. in_scope="false" actually means 
"there's being some kind of error evaluating the expression". It's not very
clear but not a big problem either. It might be good to clean this up, 
but I'm nore interested in fixing the (IMO) large scoping problems --
as I've suggested in -var-list --all-locals proposal.

- Volodya