Commit Message
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(-)
Comments
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
>>>>> "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
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
@@ -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
@@ -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 ();
}
}