[PATCH] Fix leak with internal functions

[PATCH] 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.
---
 gdb/testsuite/gdb.base/gdbvars.exp |  8 ++++++++
 gdb/value.c                        | 14 ++++++++++++++
 2 files changed, 22 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 1bd180a62cc..58e4a975485 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1882,6 +1882,11 @@ struct internalvar
     : name (std::move (name))
   {}
 
+  ~internalvar ()
+  {
+    clear_internalvar (this);
+  }
+
   std::string name;
 
   /* We support various different kinds of content of an internal variable.
@@ -2319,6 +2324,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;
     }
@@ -4513,5 +4526,6 @@ and exceeds this limit will cause an error."),
   add_final_cleanup ([] ()
     {
       all_values.clear ();
+      internalvars.clear ();
     });
 }
-- 
2.43.0

Re: [PATCH] Fix leak with internal functions

[Simon Marchi]

On 4/22/24 2:05 PM, Tom Tromey wrote:
> diff --git a/gdb/value.c b/gdb/value.c
> index 1bd180a62cc..58e4a975485 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -1882,6 +1882,11 @@ struct internalvar
>      : name (std::move (name))
>    {}
>  
> +  ~internalvar ()
> +  {
> +    clear_internalvar (this);
> +  }

This struct should probably be made non-movable/assignable/copyable/etc
(or the appropriate ones should be implemented, if they are needed).

Simon