[2/6] Mention which return values need to be freed in lang_varobj_ops
Commit Message
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(-)
Comments
> 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?
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?
> 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.
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.
@@ -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);