Patchwork [2/6] Mention which return values need to be freed in lang_varobj_ops

login
register
mail settings
Submitter Simon Marchi
Date Jan. 29, 2015, 7:28 p.m.
Message ID <1422559716-5480-2-git-send-email-simon.marchi@ericsson.com>
Download mbox | patch
Permalink /patch/4845/
State New
Headers show

Comments

Simon Marchi - Jan. 29, 2015, 7:28 p.m.
This is the result of a little bit of investigation of the C and Ada
languages, as well as some common sense.

gdb/ChangeLog:

	* varobj.h (lang_varobj_ops): Mention which return values need
	to be freed.
---
 gdb/varobj.h | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
Joel Brobecker - Jan. 30, 2015, 3:28 a.m.
> gdb/ChangeLog:
> 
> 	* varobj.h (lang_varobj_ops): Mention which return values need
> 	to be freed.

Thanks for doing that! One question...

> -  /* The ``struct value *'' of the INDEX'th child of PARENT.  */
> +  /* The ``struct value *'' of the INDEX'th child of PARENT.  The returned
> +     value must be freed by the caller.  */
>    struct value *(*value_of_child) (struct varobj *parent, int index);

I'm really surprised by this. For memory management, the struct value
objects are put on a chain. So, you wouldn't delete the value returned,
but you would instead use "value_mark/value_free_to_mark". The top-level
command loop takes a mark at the beginning of the command, and uses it
to free any un-freed value after the command completes.

But maybe you saw something that contradicts my understanding?
Simon Marchi - Jan. 30, 2015, 4:41 p.m.
On 15-01-29 10:28 PM, Joel Brobecker wrote:
>> gdb/ChangeLog:
>>
>> 	* varobj.h (lang_varobj_ops): Mention which return values need
>> 	to be freed.
> 
> Thanks for doing that! One question...
> 
>> -  /* The ``struct value *'' of the INDEX'th child of PARENT.  */
>> +  /* The ``struct value *'' of the INDEX'th child of PARENT.  The returned
>> +     value must be freed by the caller.  */
>>    struct value *(*value_of_child) (struct varobj *parent, int index);
> 
> I'm really surprised by this. For memory management, the struct value
> objects are put on a chain. So, you wouldn't delete the value returned,
> but you would instead use "value_mark/value_free_to_mark". The top-level
> command loop takes a mark at the beginning of the command, and uses it
> to free any un-freed value after the command completes.
> 
> But maybe you saw something that contradicts my understanding?

After looking more closely, I think you are right. Originally, I saw that
install_new_value called value_free on the old value and jumped to the
conclusion. Actually, value_free is more like a "value_decref", which
frees the variable if the reference count drops to 0. The call to
value_free just matches the value_incref that was also done in
install_new_value when we installed the value. So just calling
value_of_child doesn't mean that you have to call value_free.

Thanks for the explanation, I didn't know about the memory management of
values. I'll remove the comment change for value_of_child. Is the rest of
the patch ok?
Joel Brobecker - Jan. 31, 2015, 3:20 a.m.
> After looking more closely, I think you are right. Originally, I saw that
> install_new_value called value_free on the old value and jumped to the
> conclusion. Actually, value_free is more like a "value_decref", which
> frees the variable if the reference count drops to 0. The call to
> value_free just matches the value_incref that was also done in
> install_new_value when we installed the value. So just calling
> value_of_child doesn't mean that you have to call value_free.

Good :).

> Thanks for the explanation, I didn't know about the memory management of
> values. I'll remove the comment change for value_of_child. Is the rest of
> the patch ok?

Yes, it is. Go ahead and push the version without this particular
comment change.
Simon Marchi - Feb. 2, 2015, 6:18 p.m.
On 15-01-30 10:20 PM, Joel Brobecker wrote:
>> After looking more closely, I think you are right. Originally, I saw that
>> install_new_value called value_free on the old value and jumped to the
>> conclusion. Actually, value_free is more like a "value_decref", which
>> frees the variable if the reference count drops to 0. The call to
>> value_free just matches the value_incref that was also done in
>> install_new_value when we installed the value. So just calling
>> value_of_child doesn't mean that you have to call value_free.
> 
> Good :).
> 
>> Thanks for the explanation, I didn't know about the memory management of
>> values. I'll remove the comment change for value_of_child. Is the rest of
>> the patch ok?
> 
> Yes, it is. Go ahead and push the version without this particular
> comment change.
> 

Thanks, pushed.

Patch

diff --git a/gdb/varobj.h b/gdb/varobj.h
index 796b940..145b963 100644
--- a/gdb/varobj.h
+++ b/gdb/varobj.h
@@ -169,23 +169,28 @@  struct lang_varobj_ops
   /* The number of children of PARENT.  */
   int (*number_of_children) (struct varobj *parent);
 
-  /* The name (expression) of a root varobj.  */
+  /* The name (expression) of a root varobj.  The returned value must be freed
+     by the caller.  */
   char *(*name_of_variable) (struct varobj *parent);
 
-  /* The name of the INDEX'th child of PARENT.  */
+  /* The name of the INDEX'th child of PARENT.  The returned value must be
+     freed by the caller.  */
   char *(*name_of_child) (struct varobj *parent, int index);
 
   /* Returns the rooted expression of CHILD, which is a variable
-     obtain that has some parent.  */
+     obtain that has some parent.  The returned value must be freed by the
+     caller.  */
   char *(*path_expr_of_child) (struct varobj *child);
 
-  /* The ``struct value *'' of the INDEX'th child of PARENT.  */
+  /* The ``struct value *'' of the INDEX'th child of PARENT.  The returned
+     value must be freed by the caller.  */
   struct value *(*value_of_child) (struct varobj *parent, int index);
 
   /* The type of the INDEX'th child of PARENT.  */
   struct type *(*type_of_child) (struct varobj *parent, int index);
 
-  /* The current value of VAR.  */
+  /* The current value of VAR.  The returned value must be freed by the
+     caller.  */
   char *(*value_of_variable) (struct varobj *var,
 			      enum varobj_display_formats format);