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

Simon Marchi
2017-08-07T11:46:50Z

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