Re: [RFA] C++-ify skip.c

Tom Tromey
2017-08-08T19:52:32Z

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

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

Alright, here's a new version.

Tom

commit 2d47b197465b640fde68194b3ffe0c5dfa57c134
Author: Tom Tromey <tom@tromey.com>
Date: Sat Aug 5 16:40:56 2017 -0600

C++-ify skip.c

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-08 Tom Tromey <tom@tromey.com>

* skip.c (skiplist_entry): New constructor.
(skiplist_entry::enabled, skiplist_entry::function_is_regexp)
(skiplist_entry::file_is_glob): Now bool.
(skiplist_entry::file, skiplist_entry::function): Now
std::string.
(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)
(skip_form_bytes, compile_skip_regexp, skip_command, skip_info)
(skip_file_p, skip_gfile_p, skip_function_p, skip_rfunction_p)
(function_name_is_marked_for_skip): Update.
(skip_delete_command): Update. Use delete.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 722fade..643e407 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,19 @@
+2017-08-08 Tom Tromey <tom@tromey.com>
+
+ * skip.c (skiplist_entry): New constructor.
+ (skiplist_entry::enabled, skiplist_entry::function_is_regexp)
+ (skiplist_entry::file_is_glob): Now bool.
+ (skiplist_entry::file, skiplist_entry::function): Now
+ std::string.
+ (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)
+ (skip_form_bytes, compile_skip_regexp, skip_command, skip_info)
+ (skip_file_p, skip_gfile_p, skip_function_p, skip_rfunction_p)
+ (function_name_is_marked_for_skip): 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..66e9282 100644
--- a/gdb/skip.c
+++ b/gdb/skip.c
@@ -38,34 +38,44 @@

struct skiplist_entry
{
+ skiplist_entry (bool file_is_glob_, std::string &&file_,
+ bool function_is_regexp_, std::string &&function_)
+ : number (-1),
+ file_is_glob (file_is_glob_),
+ file (file_),
+ function_is_regexp (function_is_regexp_),
+ function (function_),
+ enabled (true),
+ next (NULL)
+ {
+ }
+
int number;

- /* Non-zero if FILE is a glob-style pattern.
- Otherewise it is the plain file name (possibly with directories). */
- int file_is_glob;
+ /* True if FILE is a glob-style pattern.
+ Otherwise it is the plain file name (possibly with directories). */
+ bool file_is_glob;

- /* The name of the file or NULL.
- The skiplist entry owns this pointer. */
- char *file;
+ /* The name of the file or empty if no name. */
+ std::string file;

- /* Non-zero if FUNCTION is a regexp.
+ /* True if FUNCTION is a regexp.
Otherwise it is a plain function name (possibly with arguments,
for C++). */
- int function_is_regexp;
+ bool function_is_regexp;

- /* The name of the function or NULL.
- The skiplist entry owns this pointer. */
- char *function;
+ /* The name of the function or empty if no name. */
+ std::string function;

/* 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 +90,19 @@ static int skiplist_entry_count;

/* Create a skip object. */

-static struct skiplist_entry *
-make_skip_entry (int file_is_glob, const char *file,
- int function_is_regexp, const char *function)
+static std::unique_ptr<skiplist_entry>
+make_skip_entry (bool file_is_glob, std::string &&file,
+ bool function_is_regexp, std::string &&function)
{
- struct skiplist_entry *e = XCNEW (struct skiplist_entry);
-
- gdb_assert (file != NULL || function != NULL);
+ gdb_assert (!file.empty () || !function.empty ());
if (file_is_glob)
- gdb_assert (file != NULL);
+ gdb_assert (!file.empty ());
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. */
+ gdb_assert (!function.empty ());

-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, std::move (file),
+ function_is_regexp, std::move (function)));
}

static void
@@ -150,7 +126,8 @@ skip_file_command (char *arg, int from_tty)
else
filename = arg;

- add_skiplist_entry (make_skip_entry (0, filename, 0, NULL));
+ add_skiplist_entry (make_skip_entry (false, std::string (filename), false,
+ std::string ()));

printf_filtered (_("File %s will be skipped when stepping.\n"), filename);
}
@@ -161,7 +138,8 @@ skip_file_command (char *arg, int from_tty)
static void
skip_function (const char *name)
{
- add_skiplist_entry (make_skip_entry (0, NULL, 0, name));
+ add_skiplist_entry (make_skip_entry (false, std::string (), false,
+ std::string (name)));

printf_filtered (_("Function %s will be skipped when stepping.\n"), name);
}
@@ -204,8 +182,8 @@ compile_skip_regexp (struct skiplist_entry *e, const char *message)
flags |= REG_EXTENDED;
#endif

- gdb_assert (e->function_is_regexp && e->function != NULL);
- e->compiled_function_regexp.emplace (e->function, flags, message);
+ gdb_assert (e->function_is_regexp && !e->function.empty ());
+ e->compiled_function_regexp.emplace (e->function.c_str (), flags, message);
}

/* Process "skip ..." that does not match "skip file" or "skip function". */
@@ -217,7 +195,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 +268,23 @@ 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::string entry_file;
+ if (file != NULL)
+ entry_file = file;
+ else if (gfile != NULL)
+ entry_file = gfile;
+ std::string entry_function;
+ if (function != NULL)
+ entry_function = function;
+ else if (rfunction != NULL)
+ entry_function = rfunction;
+ std::unique_ptr<skiplist_entry> e
+ = make_skip_entry (gfile != NULL, std::move (entry_file),
+ rfunction != NULL, std::move (entry_function));
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
@@ -392,14 +376,16 @@ skip_info (char *arg, int from_tty)
current_uiout->field_string ("regexp", "n"); /* 3 */

current_uiout->field_string ("file",
- e->file ? e->file : "<none>"); /* 4 */
+ e->file.empty () ? "<none>"
+ : e->file.c_str ()); /* 4 */
if (e->function_is_regexp)
current_uiout->field_string ("regexp", "y"); /* 5 */
else
current_uiout->field_string ("regexp", "n"); /* 5 */

- current_uiout->field_string (
- "function", e->function ? e->function : "<none>"); /* 6 */
+ current_uiout->field_string ("function",
+ e->function.empty () ? "<none>"
+ : e->function.c_str ()); /* 6 */

current_uiout->text ("\n");
}
@@ -414,7 +400,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 +417,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 +440,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 +455,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 +466,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 ();
}
}

@@ -495,28 +481,29 @@ static int
skip_file_p (struct skiplist_entry *e,
const struct symtab_and_line *function_sal)
{
- gdb_assert (e->file != NULL && !e->file_is_glob);
+ gdb_assert (!e->file.empty () && !e->file_is_glob);

if (function_sal->symtab == NULL)
return 0;

/* Check first sole SYMTAB->FILENAME. It may not be a substring of
symtab_to_fullname as it may contain "./" etc. */
- if (compare_filenames_for_search (function_sal->symtab->filename, e->file))
+ if (compare_filenames_for_search (function_sal->symtab->filename,
+ e->file.c_str ()))
return 1;

/* Before we invoke realpath, which can get expensive when many
files are involved, do a quick comparison of the basenames. */
if (!basenames_may_differ
&& filename_cmp (lbasename (function_sal->symtab->filename),
- lbasename (e->file)) != 0)
+ lbasename (e->file.c_str ())) != 0)
return 0;

/* Note: symtab_to_fullname caches its result, thus we don't have to. */
{
const char *fullname = symtab_to_fullname (function_sal->symtab);

- if (compare_filenames_for_search (fullname, e->file))
+ if (compare_filenames_for_search (fullname, e->file.c_str ()))
return 1;
}

@@ -529,14 +516,14 @@ static int
skip_gfile_p (struct skiplist_entry *e,
const struct symtab_and_line *function_sal)
{
- gdb_assert (e->file != NULL && e->file_is_glob);
+ gdb_assert (!e->file.empty () && e->file_is_glob);

if (function_sal->symtab == NULL)
return 0;

/* Check first sole SYMTAB->FILENAME. It may not be a substring of
symtab_to_fullname as it may contain "./" etc. */
- if (gdb_filename_fnmatch (e->file, function_sal->symtab->filename,
+ if (gdb_filename_fnmatch (e->file.c_str (), function_sal->symtab->filename,
FNM_FILE_NAME | FNM_NOESCAPE) == 0)
return 1;

@@ -546,7 +533,7 @@ skip_gfile_p (struct skiplist_entry *e,
If the basename of the glob pattern is something like "*.c" then this
isn't much of a win. Oh well. */
if (!basenames_may_differ
- && gdb_filename_fnmatch (lbasename (e->file),
+ && gdb_filename_fnmatch (lbasename (e->file.c_str ()),
lbasename (function_sal->symtab->filename),
FNM_FILE_NAME | FNM_NOESCAPE) != 0)
return 0;
@@ -555,7 +542,7 @@ skip_gfile_p (struct skiplist_entry *e,
{
const char *fullname = symtab_to_fullname (function_sal->symtab);

- if (compare_glob_filenames_for_search (fullname, e->file))
+ if (compare_glob_filenames_for_search (fullname, e->file.c_str ()))
return 1;
}

@@ -567,8 +554,8 @@ skip_gfile_p (struct skiplist_entry *e,
static int
skip_function_p (struct skiplist_entry *e, const char *function_name)
{
- gdb_assert (e->function != NULL && !e->function_is_regexp);
- return strcmp_iw (function_name, e->function) == 0;
+ gdb_assert (!e->function.empty () && !e->function_is_regexp);
+ return strcmp_iw (function_name, e->function.c_str ()) == 0;
}

/* Return non-zero if we're stopped at a function regexp to be skipped. */
@@ -576,7 +563,7 @@ skip_function_p (struct skiplist_entry *e, const char *function_name)
static int
skip_rfunction_p (struct skiplist_entry *e, const char *function_name)
{
- gdb_assert (e->function != NULL && e->function_is_regexp
+ gdb_assert (!e->function.empty () && e->function_is_regexp
&& e->compiled_function_regexp);
return (e->compiled_function_regexp->exec (function_name, 0, NULL, 0)
== 0);
@@ -601,7 +588,7 @@ function_name_is_marked_for_skip (const char *function_name,
if (!e->enabled)
continue;

- if (e->file != NULL)
+ if (!e->file.empty ())
{
if (e->file_is_glob)
{
@@ -614,7 +601,7 @@ function_name_is_marked_for_skip (const char *function_name,
skip_by_file = 1;
}
}
- if (e->function != NULL)
+ if (!e->function.empty ())
{
if (e->function_is_regexp)
{
@@ -630,7 +617,7 @@ function_name_is_marked_for_skip (const char *function_name,

/* If both file and function must match, make sure we don't errantly
exit if only one of them match. */
- if (e->file != NULL && e->function != NULL)
+ if (!e->file.empty () && !e->function.empty ())
{
if (skip_by_file && skip_by_function)
return 1;