On 2017-08-08 21:47, Tom Tromey wrote:
>>>>>> "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_),
I think you have to use std::move here to avoid a copy being made. I
wasn't sure so I made a small test if you want to try for yourself (or
maybe find that I got it wrong): http://paste.ubuntu.com/25275614/
Otherwise, a simpler way would be to leave the constructor (and calling
functions) accepting const char* and just change the fields themselves
to std::string. The string objects will be constructed using the const
char* constructor when the skiplist_entry object itself will be
constructed, so you know you for sure you won't have any unnecessary
copies. That would avoid complicating the calling functions as well.
Either way (adding std::moves or this) are fine for me, it's as you
wish.
Thanks,
Simon
@@ -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
@@ -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;