This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA v2 1/4] C++-ify break-catch-sig
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: Tom Tromey <tom at tromey dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 20 Jul 2017 14:13:20 -0400
- Subject: Re: [RFA v2 1/4] C++-ify break-catch-sig
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=sergiodj at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 74D38267C6
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 74D38267C6
- References: <20170719203225.6714-1-tom@tromey.com> <20170719203225.6714-2-tom@tromey.com>
On Wednesday, July 19 2017, Tom Tromey wrote:
> This changes signal_catchpoint to be more of a C++ class, using
> std::vector and updating the users.
Thanks for the patch, Tom.
> 2017-06-04 Tom Tromey <tom@tromey.com>
>
> * break-catch-sig.c (gdb_signal_type): Remove typedef.
> (struct signal_catchpoint) <signals_to_be_caught>: Now a
> std::vector.
> <catch_all>: Now a bool.
> (~signal_catchpoint): Remove.
> (signal_catchpoint_insert_location)
> (signal_catchpoint_remove_location)
> (signal_catchpoint_breakpoint_hit, signal_catchpoint_print_one)
> (signal_catchpoint_print_mention)
> (signal_catchpoint_print_recreate)
> (signal_catchpoint_explains_signal): Update.
> (create_signal_catchpoint): Change type of "filter" and
> "catch_all".
> (catch_signal_split_args): Return a std::vector. Change type of
> "catch_all".
> (catch_signal_command): Update.
> ---
> gdb/ChangeLog | 19 +++++++
> gdb/break-catch-sig.c | 143 +++++++++++++++++---------------------------------
> 2 files changed, 66 insertions(+), 96 deletions(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index bc6e55b..42bde4b 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,22 @@
> +2017-06-04 Tom Tromey <tom@tromey.com>
> +
> + * break-catch-sig.c (gdb_signal_type): Remove typedef.
> + (struct signal_catchpoint) <signals_to_be_caught>: Now a
> + std::vector.
> + <catch_all>: Now a bool.
> + (~signal_catchpoint): Remove.
> + (signal_catchpoint_insert_location)
> + (signal_catchpoint_remove_location)
> + (signal_catchpoint_breakpoint_hit, signal_catchpoint_print_one)
> + (signal_catchpoint_print_mention)
> + (signal_catchpoint_print_recreate)
> + (signal_catchpoint_explains_signal): Update.
> + (create_signal_catchpoint): Change type of "filter" and
> + "catch_all".
> + (catch_signal_split_args): Return a std::vector. Change type of
> + "catch_all".
> + (catch_signal_command): Update.
> +
> 2017-07-18 David Blaikie <dblaikie@gmail.com>
>
> * dwarf2read.c (create_cus_hash_table): Re-add lost initialization
> diff --git a/gdb/break-catch-sig.c b/gdb/break-catch-sig.c
> index 3eede93..afac016 100644
> --- a/gdb/break-catch-sig.c
> +++ b/gdb/break-catch-sig.c
> @@ -33,30 +33,24 @@
>
> #define INTERNAL_SIGNAL(x) ((x) == GDB_SIGNAL_TRAP || (x) == GDB_SIGNAL_INT)
>
> -typedef enum gdb_signal gdb_signal_type;
> -
> -DEF_VEC_I (gdb_signal_type);
> -
> /* An instance of this type is used to represent a signal catchpoint.
> A breakpoint is really of this type iff its ops pointer points to
> SIGNAL_CATCHPOINT_OPS. */
>
> struct signal_catchpoint : public breakpoint
> {
> - ~signal_catchpoint () override;
> -
> /* Signal numbers used for the 'catch signal' feature. If no signal
> - has been specified for filtering, its value is NULL. Otherwise,
> + has been specified for filtering, it is empty. Otherwise,
> it holds a list of all signals to be caught. */
>
> - VEC (gdb_signal_type) *signals_to_be_caught;
> + std::vector<gdb_signal> signals_to_be_caught;
>
> - /* If SIGNALS_TO_BE_CAUGHT is NULL, then all "ordinary" signals are
> + /* If SIGNALS_TO_BE_CAUGHT is empty, then all "ordinary" signals are
> caught. If CATCH_ALL is non-zero, then internal signals are
"If CATCH_ALL is true"
> - caught as well. If SIGNALS_TO_BE_CAUGHT is non-NULL, then this
> + caught as well. If SIGNALS_TO_BE_CAUGHT is not empty, then this
> field is ignored. */
>
> - int catch_all;
> + bool catch_all;
> };
>
> /* The breakpoint_ops structure to be used in signal catchpoints. */
> @@ -85,13 +79,6 @@ signal_to_name_or_int (enum gdb_signal sig)
>
>
>
> -/* signal_catchpoint destructor. */
> -
> -signal_catchpoint::~signal_catchpoint ()
> -{
> - VEC_free (gdb_signal_type, this->signals_to_be_caught);
> -}
> -
> /* Implement the "insert_location" breakpoint_ops method for signal
> catchpoints. */
>
> @@ -99,20 +86,15 @@ static int
> signal_catchpoint_insert_location (struct bp_location *bl)
> {
> struct signal_catchpoint *c = (struct signal_catchpoint *) bl->owner;
> - int i;
>
> - if (c->signals_to_be_caught != NULL)
> + if (!c->signals_to_be_caught.empty ())
> {
> - gdb_signal_type iter;
> -
> - for (i = 0;
> - VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter);
> - i++)
> + for (gdb_signal iter : c->signals_to_be_caught)
> ++signal_catch_counts[iter];
> }
> else
> {
> - for (i = 0; i < GDB_SIGNAL_LAST; ++i)
> + for (int i = 0; i < GDB_SIGNAL_LAST; ++i)
> {
> if (c->catch_all || !INTERNAL_SIGNAL (i))
> ++signal_catch_counts[i];
> @@ -132,15 +114,10 @@ signal_catchpoint_remove_location (struct bp_location *bl,
> enum remove_bp_reason reason)
> {
> struct signal_catchpoint *c = (struct signal_catchpoint *) bl->owner;
> - int i;
>
> - if (c->signals_to_be_caught != NULL)
> + if (!c->signals_to_be_caught.empty ())
> {
> - gdb_signal_type iter;
> -
> - for (i = 0;
> - VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter);
> - i++)
> + for (gdb_signal iter : c->signals_to_be_caught)
> {
> gdb_assert (signal_catch_counts[iter] > 0);
> --signal_catch_counts[iter];
> @@ -148,7 +125,7 @@ signal_catchpoint_remove_location (struct bp_location *bl,
> }
> else
> {
> - for (i = 0; i < GDB_SIGNAL_LAST; ++i)
> + for (int i = 0; i < GDB_SIGNAL_LAST; ++i)
> {
> if (c->catch_all || !INTERNAL_SIGNAL (i))
> {
> @@ -174,7 +151,7 @@ signal_catchpoint_breakpoint_hit (const struct bp_location *bl,
> {
> const struct signal_catchpoint *c
> = (const struct signal_catchpoint *) bl->owner;
> - gdb_signal_type signal_number;
> + gdb_signal signal_number;
>
> if (ws->kind != TARGET_WAITKIND_STOPPED)
> return 0;
> @@ -184,18 +161,12 @@ signal_catchpoint_breakpoint_hit (const struct bp_location *bl,
> /* If we are catching specific signals in this breakpoint, then we
> must guarantee that the called signal is the same signal we are
> catching. */
> - if (c->signals_to_be_caught)
> + if (!c->signals_to_be_caught.empty ())
> {
> - int i;
> - gdb_signal_type iter;
> -
> - for (i = 0;
> - VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter);
> - i++)
> + for (gdb_signal iter : c->signals_to_be_caught)
> if (signal_number == iter)
> return 1;
> /* Not the same. */
> - gdb_assert (!iter);
I don't really understand what this assert was checking. In what
situation would iter == 0?
> return 0;
> }
> else
> @@ -246,26 +217,24 @@ signal_catchpoint_print_one (struct breakpoint *b,
> uiout->field_skip ("addr");
> annotate_field (5);
>
> - if (c->signals_to_be_caught
> - && VEC_length (gdb_signal_type, c->signals_to_be_caught) > 1)
> + if (!c->signals_to_be_caught.empty ())
Should be:
if (c->signals_to_be_caught.size () > 1)
> uiout->text ("signals \"");
> else
> uiout->text ("signal \"");
>
> - if (c->signals_to_be_caught)
> + if (c->signals_to_be_caught.size () > 1)
And here:
if (!c->signals_to_be_caught.empty ())
> {
> - int i;
> - gdb_signal_type iter;
> std::string text;
>
> - for (i = 0;
> - VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter);
> - i++)
> + bool first = true;
> + for (gdb_signal iter : c->signals_to_be_caught)
> {
> const char *name = signal_to_name_or_int (iter);
>
> - if (i > 0)
> + if (!first)
> text += " ";
> + first = false;
> +
> text += name;
> }
> uiout->field_string ("what", text.c_str ());
> @@ -287,19 +256,14 @@ signal_catchpoint_print_mention (struct breakpoint *b)
> {
> struct signal_catchpoint *c = (struct signal_catchpoint *) b;
>
> - if (c->signals_to_be_caught)
> + if (!c->signals_to_be_caught.empty ())
> {
> - int i;
> - gdb_signal_type iter;
> -
> - if (VEC_length (gdb_signal_type, c->signals_to_be_caught) > 1)
> + if (c->signals_to_be_caught.size () > 1)
> printf_filtered (_("Catchpoint %d (signals"), b->number);
> else
> printf_filtered (_("Catchpoint %d (signal"), b->number);
>
> - for (i = 0;
> - VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter);
> - i++)
> + for (gdb_signal iter : c->signals_to_be_caught)
> {
> const char *name = signal_to_name_or_int (iter);
>
> @@ -323,14 +287,9 @@ signal_catchpoint_print_recreate (struct breakpoint *b, struct ui_file *fp)
>
> fprintf_unfiltered (fp, "catch signal");
>
> - if (c->signals_to_be_caught)
> + if (!c->signals_to_be_caught.empty ())
> {
> - int i;
> - gdb_signal_type iter;
> -
> - for (i = 0;
> - VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter);
> - i++)
> + for (gdb_signal iter : c->signals_to_be_caught)
> fprintf_unfiltered (fp, " %s", signal_to_name_or_int (iter));
> }
> else if (c->catch_all)
> @@ -355,8 +314,8 @@ signal_catchpoint_explains_signal (struct breakpoint *b, enum gdb_signal sig)
> then internal signals like SIGTRAP are not caught. */
>
> static void
> -create_signal_catchpoint (int tempflag, VEC (gdb_signal_type) *filter,
> - int catch_all)
> +create_signal_catchpoint (int tempflag, std::vector<gdb_signal> &&filter,
> + bool catch_all)
> {
> struct signal_catchpoint *c;
> struct gdbarch *gdbarch = get_current_arch ();
> @@ -373,57 +332,50 @@ create_signal_catchpoint (int tempflag, VEC (gdb_signal_type) *filter,
> /* Splits the argument using space as delimiter. Returns an xmalloc'd
> filter list, or NULL if no filtering is required. */
>
> -static VEC (gdb_signal_type) *
> -catch_signal_split_args (char *arg, int *catch_all)
> +static std::vector<gdb_signal>
> +catch_signal_split_args (char *arg, bool *catch_all)
> {
> - VEC (gdb_signal_type) *result = NULL;
> - struct cleanup *cleanup = make_cleanup (VEC_cleanup (gdb_signal_type),
> - &result);
> + std::vector<gdb_signal> result;
> int first = 1;
Nit: "first" can be a bool.
>
> while (*arg != '\0')
> {
> int num;
> - gdb_signal_type signal_number;
> - char *one_arg, *endptr;
> - struct cleanup *inner_cleanup;
> + gdb_signal signal_number;
> + char *endptr;
>
> - one_arg = extract_arg (&arg);
> + gdb::unique_xmalloc_ptr<char> one_arg (extract_arg (&arg));
> if (one_arg == NULL)
> break;
> - inner_cleanup = make_cleanup (xfree, one_arg);
>
> /* Check for the special flag "all". */
> - if (strcmp (one_arg, "all") == 0)
> + if (strcmp (one_arg.get (), "all") == 0)
> {
> arg = skip_spaces (arg);
> if (*arg != '\0' || !first)
> error (_("'all' cannot be caught with other signals"));
> - *catch_all = 1;
> - gdb_assert (result == NULL);
> - do_cleanups (inner_cleanup);
> - discard_cleanups (cleanup);
> - return NULL;
> + *catch_all = true;
> + gdb_assert (result.empty ());
> + return result;
> }
>
> first = 0;
>
> /* Check if the user provided a signal name or a number. */
> - num = (int) strtol (one_arg, &endptr, 0);
> + num = (int) strtol (one_arg.get (), &endptr, 0);
> if (*endptr == '\0')
> signal_number = gdb_signal_from_command (num);
> else
> {
> - signal_number = gdb_signal_from_name (one_arg);
> + signal_number = gdb_signal_from_name (one_arg.get ());
> if (signal_number == GDB_SIGNAL_UNKNOWN)
> - error (_("Unknown signal name '%s'."), one_arg);
> + error (_("Unknown signal name '%s'."), one_arg.get ());
> }
>
> - VEC_safe_push (gdb_signal_type, result, signal_number);
> - do_cleanups (inner_cleanup);
> + result.push_back (signal_number);
> }
>
> - discard_cleanups (cleanup);
> + result.shrink_to_fit ();
Do you really need this call here? You're using safe_push above, so I
don't think the vector will end up with a bigger capacity than its size.
> return result;
> }
>
> @@ -433,8 +385,9 @@ static void
> catch_signal_command (char *arg, int from_tty,
> struct cmd_list_element *command)
> {
> - int tempflag, catch_all = 0;
> - VEC (gdb_signal_type) *filter;
> + int tempflag;
> + bool catch_all = false;
> + std::vector<gdb_signal> filter;
>
> tempflag = get_cmd_context (command) == CATCH_TEMPORARY;
>
> @@ -448,10 +401,8 @@ catch_signal_command (char *arg, int from_tty,
>
> if (arg != NULL)
> filter = catch_signal_split_args (arg, &catch_all);
> - else
> - filter = NULL;
>
> - create_signal_catchpoint (tempflag, filter, catch_all);
> + create_signal_catchpoint (tempflag, std::move (filter), catch_all);
> }
>
> static void
> --
> 2.9.3
Otherwise, LGTM.
Thanks,
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/