Patchwork [RFA] C++-ify skip.c

login
register
mail settings
Submitter Tom Tromey
Date Aug. 6, 2017, 7:42 p.m.
Message ID <20170806194231.26627-1-tom@tromey.com>
Download mbox | patch
Permalink /patch/21939/
State New
Headers show

Comments

Tom Tromey - Aug. 6, 2017, 7:42 p.m.
I happened to notice that skiplist_entry, in skip.c, contains a
gdb::optional<compiled_regex> -- but that this object's destructor is
never run.  This can result in a memory leak.

This patch fixes the bug by applying a bit more C++: changing this
code to use new and delete, and std::unique_ptr; and removing cleanups
in the process.

Built and regression tested on x86-64 Fedora 25.

ChangeLog
2017-08-06  Tom Tromey  <tom@tromey.com>

	* skip.c (skiplist_entry): New constructor.
	(~skiplist_entry): New destructor.
	(skiplist_entry::enabled): Now bool.
	(make_skip_entry): Return a unique_ptr.  Use new.
	(free_skiplist_entry, free_skiplist_entry_cleanup)
	(make_free_skiplist_entry_cleanup): Remove.
	(skip_command, skip_disable_command, add_skiplist_entry): Update.
	(skip_delete_command): Update.  Use delete.
---
 gdb/ChangeLog | 11 ++++++++
 gdb/skip.c    | 89 +++++++++++++++++++++++------------------------------------
 2 files changed, 45 insertions(+), 55 deletions(-)
Simon Marchi - Aug. 7, 2017, 11:46 a.m.
On 2017-08-06 21:42, Tom Tromey wrote:
> I happened to notice that skiplist_entry, in skip.c, contains a
> gdb::optional<compiled_regex> -- but that this object's destructor is
> never run.  This can result in a memory leak.
> 
> This patch fixes the bug by applying a bit more C++: changing this
> code to use new and delete, and std::unique_ptr; and removing cleanups
> in the process.
> 
> Built and regression tested on x86-64 Fedora 25.

Ah thanks, I tripped on this with me poison-XNEW patch.  I didn't have 
the time to fix and post it yet.

> ChangeLog
> 2017-08-06  Tom Tromey  <tom@tromey.com>
> 
> 	* skip.c (skiplist_entry): New constructor.
> 	(~skiplist_entry): New destructor.
> 	(skiplist_entry::enabled): Now bool.
> 	(make_skip_entry): Return a unique_ptr.  Use new.
> 	(free_skiplist_entry, free_skiplist_entry_cleanup)
> 	(make_free_skiplist_entry_cleanup): Remove.
> 	(skip_command, skip_disable_command, add_skiplist_entry): Update.
> 	(skip_delete_command): Update.  Use delete.
> ---
>  gdb/ChangeLog | 11 ++++++++
>  gdb/skip.c    | 89 
> +++++++++++++++++++++++------------------------------------
>  2 files changed, 45 insertions(+), 55 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 722fade..12e0d02 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,14 @@
> +2017-08-06  Tom Tromey  <tom@tromey.com>
> +
> +	* skip.c (skiplist_entry): New constructor.
> +	(~skiplist_entry): New destructor.
> +	(skiplist_entry::enabled): Now bool.
> +	(make_skip_entry): Return a unique_ptr.  Use new.
> +	(free_skiplist_entry, free_skiplist_entry_cleanup)
> +	(make_free_skiplist_entry_cleanup): Remove.
> +	(skip_command, skip_disable_command, add_skiplist_entry): Update.
> +	(skip_delete_command): Update.  Use delete.
> +
>  2017-08-05  Tom Tromey  <tom@tromey.com>
> 
>  	* compile/compile-object-load.c (compile_object_load): Use
> diff --git a/gdb/skip.c b/gdb/skip.c
> index bf44913..f3291f3 100644
> --- a/gdb/skip.c
> +++ b/gdb/skip.c
> @@ -38,6 +38,24 @@
> 
>  struct skiplist_entry
>  {
> +  skiplist_entry (int file_is_glob_, const char *file_,
> +		  int function_is_regexp_, const char *function_)
> +    : number (-1),
> +      file_is_glob (file_is_glob_),

file_is_glob and function_is_regexp can probably be changed to bools 
too.

> +      file (file_ == NULL ? NULL : xstrdup (file_)),
> +      function_is_regexp (function_is_regexp_),
> +      function (function_ == NULL ? NULL : xstrdup (function_)),

If we are strdup'ing (making a copy) file and function, we might as well 
store them in std::strings, so we don't need to explicitly xfree them.  
And the implicitly defined copy constructor/assignment operator will 
also work (which is not the case now).  An empty string is probably 
enough to mean the field is not used (which is expressed by a NULL value 
currently).

Thanks,

Simon
Tom Tromey - Aug. 7, 2017, 1:57 p.m.
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> Ah thanks, I tripped on this with me poison-XNEW patch.  I didn't have
Simon> the time to fix and post it yet.

Ah, I didn't know about this.
I think I will wait for that before working in this area any more.

Tom
Simon Marchi - Aug. 7, 2017, 2:32 p.m.
On 2017-08-07 15:57, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> Ah thanks, I tripped on this with me poison-XNEW patch.  I 
> didn't have
> Simon> the time to fix and post it yet.
> 
> Ah, I didn't know about this.
> I think I will wait for that before working in this area any more.
> 
> Tom

There are many things to fix before the poison-xnew patch can go in, but 
I think the issue you found is worth fixing sooner than later.

Among the things that need to change is the opaque types with multiple 
definitions, like arch_lwp_info and private_thread_info.  These need to 
be made into actual class hierarchies.  If anybody is looking for things 
to do, feel free to pick up some patches and post them :)

https://github.com/simark/binutils-gdb/commits/poison-xnew

Simon

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 722fade..12e0d02 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@ 
+2017-08-06  Tom Tromey  <tom@tromey.com>
+
+	* skip.c (skiplist_entry): New constructor.
+	(~skiplist_entry): New destructor.
+	(skiplist_entry::enabled): Now bool.
+	(make_skip_entry): Return a unique_ptr.  Use new.
+	(free_skiplist_entry, free_skiplist_entry_cleanup)
+	(make_free_skiplist_entry_cleanup): Remove.
+	(skip_command, skip_disable_command, add_skiplist_entry): Update.
+	(skip_delete_command): Update.  Use delete.
+
 2017-08-05  Tom Tromey  <tom@tromey.com>
 
 	* compile/compile-object-load.c (compile_object_load): Use
diff --git a/gdb/skip.c b/gdb/skip.c
index bf44913..f3291f3 100644
--- a/gdb/skip.c
+++ b/gdb/skip.c
@@ -38,6 +38,24 @@ 
 
 struct skiplist_entry
 {
+  skiplist_entry (int file_is_glob_, const char *file_,
+		  int function_is_regexp_, const char *function_)
+    : number (-1),
+      file_is_glob (file_is_glob_),
+      file (file_ == NULL ? NULL : xstrdup (file_)),
+      function_is_regexp (function_is_regexp_),
+      function (function_ == NULL ? NULL : xstrdup (function_)),
+      enabled (true),
+      next (NULL)
+  {
+  }
+
+  ~skiplist_entry ()
+  {
+    xfree (file);
+    xfree (function);
+  }
+
   int number;
 
   /* Non-zero if FILE is a glob-style pattern.
@@ -60,12 +78,12 @@  struct skiplist_entry
   /* If this is a function regexp, the compiled form.  */
   gdb::optional<compiled_regex> compiled_function_regexp;
 
-  int enabled;
+  bool enabled;
 
   struct skiplist_entry *next;
 };
 
-static void add_skiplist_entry (struct skiplist_entry *e);
+static void add_skiplist_entry (std::unique_ptr<skiplist_entry> &&e);
 
 static struct skiplist_entry *skiplist_entry_chain;
 static int skiplist_entry_count;
@@ -80,53 +98,18 @@  static int skiplist_entry_count;
 
 /* Create a skip object.  */
 
-static struct skiplist_entry *
+static std::unique_ptr<skiplist_entry>
 make_skip_entry (int file_is_glob, const char *file,
 		 int function_is_regexp, const char *function)
 {
-  struct skiplist_entry *e = XCNEW (struct skiplist_entry);
-
   gdb_assert (file != NULL || function != NULL);
   if (file_is_glob)
     gdb_assert (file != NULL);
   if (function_is_regexp)
     gdb_assert (function != NULL);
 
-  if (file != NULL)
-    e->file = xstrdup (file);
-  if (function != NULL)
-    e->function = xstrdup (function);
-  e->file_is_glob = file_is_glob;
-  e->function_is_regexp = function_is_regexp;
-  e->enabled = 1;
-
-  return e;
-}
-
-/* Free a skiplist entry.  */
-
-static void
-free_skiplist_entry (struct skiplist_entry *e)
-{
-  xfree (e->file);
-  xfree (e->function);
-  xfree (e);
-}
-
-/* Wrapper to free_skiplist_entry for use as a cleanup.  */
-
-static void
-free_skiplist_entry_cleanup (void *e)
-{
-  free_skiplist_entry ((struct skiplist_entry *) e);
-}
-
-/* Create a cleanup to free skiplist entry E.  */
-
-static struct cleanup *
-make_free_skiplist_entry_cleanup (struct skiplist_entry *e)
-{
-  return make_cleanup (free_skiplist_entry_cleanup, e);
+  return std::unique_ptr<skiplist_entry>
+    (new skiplist_entry (file_is_glob, file, function_is_regexp, function));
 }
 
 static void
@@ -217,7 +200,6 @@  skip_command (char *arg, int from_tty)
   const char *gfile = NULL;
   const char *function = NULL;
   const char *rfunction = NULL;
-  struct skiplist_entry *e;
   int i;
 
   if (arg == NULL)
@@ -291,16 +273,13 @@  skip_command (char *arg, int from_tty)
   gdb_assert (file != NULL || gfile != NULL
 	      || function != NULL || rfunction != NULL);
 
-  e = make_skip_entry (gfile != NULL, file ? file : gfile,
-		       rfunction != NULL, function ? function : rfunction);
+  std::unique_ptr<skiplist_entry> e
+    = make_skip_entry (gfile != NULL, file ? file : gfile, rfunction != NULL,
+		       function ? function : rfunction);
   if (rfunction != NULL)
-    {
-      struct cleanup *rf_cleanups = make_free_skiplist_entry_cleanup (e);
+    compile_skip_regexp (e.get (), _("regexp"));
 
-      compile_skip_regexp (e, _("regexp"));
-      discard_cleanups (rf_cleanups);
-    }
-  add_skiplist_entry (e);
+  add_skiplist_entry (std::move (e));
 
   /* I18N concerns drive some of the choices here (we can't piece together
      the output too much).  OTOH we want to keep this simple.  Therefore the
@@ -414,7 +393,7 @@  skip_enable_command (char *arg, int from_tty)
   ALL_SKIPLIST_ENTRIES (e)
     if (arg == NULL || number_is_in_list (arg, e->number))
       {
-        e->enabled = 1;
+        e->enabled = true;
         found = 1;
       }
 
@@ -431,7 +410,7 @@  skip_disable_command (char *arg, int from_tty)
   ALL_SKIPLIST_ENTRIES (e)
     if (arg == NULL || number_is_in_list (arg, e->number))
       {
-	e->enabled = 0;
+	e->enabled = false;
         found = 1;
       }
 
@@ -454,7 +433,7 @@  skip_delete_command (char *arg, int from_tty)
 	else
 	  skiplist_entry_chain = e->next;
 
-	free_skiplist_entry (e);
+	delete e;
         found = 1;
       }
     else
@@ -469,7 +448,7 @@  skip_delete_command (char *arg, int from_tty)
 /* Add the given skiplist entry to our list, and set the entry's number.  */
 
 static void
-add_skiplist_entry (struct skiplist_entry *e)
+add_skiplist_entry (std::unique_ptr<skiplist_entry> &&e)
 {
   struct skiplist_entry *e1;
 
@@ -480,12 +459,12 @@  add_skiplist_entry (struct skiplist_entry *e)
 
   e1 = skiplist_entry_chain;
   if (e1 == NULL)
-    skiplist_entry_chain = e;
+    skiplist_entry_chain = e.release ();
   else
     {
       while (e1->next)
 	e1 = e1->next;
-      e1->next = e;
+      e1->next = e.release ();
     }
 }