Re: [PATCH v2] Fix leak with internal functions

[Guinevere Larsen <guinevere@redhat.com>]

On 4/26/24 12:56 PM, Tom Tromey wrote:
> Valgrind reported a memory leak on exit with internal functions.
>
> To fix this, I added a new case to clear_internalvar, and then
> arranged to call clear_internalvar when an internalvar is deleted.
> Also, because an internalvar can reference a value, it seemed prudent
> to clear 'internalvars' from the final cleanup in value.c.
>
> Regression tested on x86-64 Fedora 38.  I also did a bit of testing by
> hand with valgrind and ASAN.
>
> This version includes the DISABLE_COPY_AND_ASSIGN requested in review.
> The move constructor and assignment operator are needed by the
> std::map.
> ---

Hi!

I was looking at the "long standing unreviewed patches" in my backlog 
and noticed this. I just tried it and it applies cleanly to upstream, so 
I guess it was never fixed.

This needs a tiny adjustement to compile, but other than that this patch 
looks good.

>   gdb/testsuite/gdb.base/gdbvars.exp |  8 ++++++++
>   gdb/value.c                        | 31 ++++++++++++++++++++++++++++++
>   2 files changed, 39 insertions(+)
>
> diff --git a/gdb/testsuite/gdb.base/gdbvars.exp b/gdb/testsuite/gdb.base/gdbvars.exp
> index 7ec7df36b9e..32fd4c4da4e 100644
> --- a/gdb/testsuite/gdb.base/gdbvars.exp
> +++ b/gdb/testsuite/gdb.base/gdbvars.exp
> @@ -102,6 +102,14 @@ proc test_convenience_functions {} {
>   
>       gdb_test "print \$_isvoid (foo_int ())" " = 0" \
>   	"Check whether non-void function is void"
> +
> +    gdb_test "print \$isvoid_copy = \$_isvoid" \
> +	" = <internal function _isvoid>"
> +    gdb_test "print \$isvoid_copy = 0" " = 0"
> +
> +    # Can't reset the canonical name.
> +    gdb_test "print \$_isvoid = 0" \
> +	"Cannot overwrite convenience function _isvoid"
>   }
>   
>   proc test_value_history {} {
> diff --git a/gdb/value.c b/gdb/value.c
> index e71f38b0ce4..47e7a616e48 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -1883,6 +1883,28 @@ struct internalvar
>       : name (std::move (name))
>     {}
>   
> +  ~internalvar ()
> +  {
> +    clear_internalvar (this);
> +  }
> +
> +  internalvar (internalvar &&other)
> +    : kind (other.kind),
> +      u (other.u)
> +  {
> +    other.kind = INTERNALVAR_VOID;
> +  }
> +
> +  internalvar &operator= (internalvar &&other)
> +  {
> +    kind = other.kind;
> +    u = other.u;
> +    other.kind = INTERNALVAR_VOID;
> +    return *this;
> +  }
> +
> +  DISABLE_COPY_AND_ASSIGN (internalvar);
> +
>     std::string name;
>   
>     /* We support various different kinds of content of an internal variable.
> @@ -2320,6 +2342,14 @@ clear_internalvar (struct internalvar *var)
>         xfree (var->u.string);
>         break;
>   
> +    case INTERNALVAR_FUNCTION:
> +      if (var->u.fn.canonical)
> +	{
> +	  xfree (var->u.fn.function->name);
> +	  xfree (var->u.fn.function);

this needs to be changed to `delete var->u.fn.function;`

with this change, it looks good

Reviewed-By: Guinevere Larsen <guinevere@redhat.com>

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

> +	}
> +      break;
> +
>       default:
>         break;
>       }
> @@ -4514,5 +4544,6 @@ and exceeds this limit will cause an error."),
>     add_final_cleanup ([] ()
>       {
>         all_values.clear ();
> +      internalvars.clear ();
>       });
>   }