[RFA,v2,1/4] C++-ify break-catch-sig

  This changes signal_catchpoint to be more of a C++ class, using
std::vector and updating the users.

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(-)
  

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,
  

Hi Tom,

Two nits, in addition to what Sergio already pointed out.

> @@ -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)

The documentation of this function would need updating.

>  {
>    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)

The documentation of this function too.

Thanks,

Simon
  

On 2017-07-20 20:13, Sergio Durigan Junior wrote:
>> 
>>    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.

safe_push is the old implementations, std::vector grows the capacity 
exponentially, so you're likely to be left with unused capacity.

Simon
  

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

>> -  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);

Sergio> I don't really understand what this assert was checking.  In what
Sergio> situation would iter == 0?

Yeah, I don't know either.

Tom