[PATCH v2] Fix leak with internal functions

[PATCH v2] Fix leak with internal functions

[Tom Tromey]

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

Re: [PATCH v2] Fix leak with internal functions

[Guinevere Larsen]

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

Re: [PATCH v2] Fix leak with internal functions

[Tom Tromey]

>>>>> "Guinevere" == Guinevere Larsen <guinevere@redhat.com> writes:

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


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

Thanks, I've updated this & I'm checking it in.

Tom

Re: [PATCH v2] Fix leak with internal functions

[Tom Tromey]

Tom> Thanks, I've updated this & I'm checking it in.

Spoke too soon, there's a regression.
I'll fix it up sometime later.

Tom