[RFA] C++-ify skip.c

Tom Tromey
2017-08-06T19:42:45Z

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

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

--
2.9.4