[RFA,2/2] C++-ify break-catch-throw

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

gdb/ChangeLog
2017-06-04  Tom Tromey  <tom@tromey.com>

	* break-catch-throw.c (struct exception_catchpoint)
	<exception_rx>: Now a std::string.
	<pattern>: Now a unique_ptr.
	(~exception_catchpoint, check_status_exception_catchpoint)
	(print_one_detail_exception_catchpoint): Update.
	(handle_gnu_v3_exceptions): Change type of except_rx.
	(extract_exception_regexp): Return a std::string.
	(catch_exception_command_1): Update.
---
 gdb/ChangeLog           | 11 +++++++++++
 gdb/break-catch-throw.c | 49 +++++++++++++++++++++----------------------------
 2 files changed, 32 insertions(+), 28 deletions(-)
  

> --- a/gdb/break-catch-throw.c
> +++ b/gdb/break-catch-throw.c
> @@ -82,15 +82,15 @@ struct exception_catchpoint : public breakpoint
> 
>    enum exception_event_kind kind;
> 
> -  /* If non-NULL, an xmalloc'd string holding the source form of the
> -     regular expression to match against.  */
> +  /* If not empty, a string holding the source form of the regular
> +     expression to match against.  */
> 
> -  char *exception_rx;
> +  std::string exception_rx;
> 
> -  /* If non-NULL, an xmalloc'd, compiled regular expression which is
> +  /* If non-NULL, a compiled regular expression which is
>       used to determine which exceptions to stop on.  */
> 
> -  regex_t *pattern;
> +  std::unique_ptr<regex_t> pattern;

 From what I understand, we were freeing the regex with regfree, but we 
weren't freeing the regex_t object, which is allocated separately, is 
that right?  Or what is freed in handle_gnu_v3_exceptions by the 
cleanup, and then referenced later?  Either way your code LGTM.

Simon
  

On 06/04/2017 11:53 PM, Tom Tromey wrote:
> @@ -452,17 +450,14 @@ catch_exception_command_1 (enum exception_event_kind ex_event,
>  			   char *arg_entry,
>  			   int tempflag, int from_tty)
>  {
> -  char *except_rx;
>    const char *cond_string = NULL;
> -  struct cleanup *cleanup;
>    const char *arg = arg_entry;
>  
>    if (!arg)
>      arg = "";
>    arg = skip_spaces_const (arg);
>  
> -  except_rx = extract_exception_regexp (&arg);
> -  cleanup = make_cleanup (xfree, except_rx);
> +  std::string except_rx = extract_exception_regexp (&arg);
>  
>    cond_string = ep_parse_optional_if_clause (&arg);
>  
> @@ -474,10 +469,8 @@ catch_exception_command_1 (enum exception_event_kind ex_event,
>        && ex_event != EX_EVENT_RETHROW)
>      error (_("Unsupported or unknown exception event; cannot catch it"));
>  
> -  handle_gnu_v3_exceptions (tempflag, except_rx, cond_string,
> +  handle_gnu_v3_exceptions (tempflag, std::move (except_rx), cond_string,
>  			    ex_event, from_tty);
> -
> -  discard_cleanups (cleanup);
>  }

Something looks suspicious to me -- compile_rx_or_error returns with
an installed cleanup that calls regfree, and handle_gnu_v3_exceptions
leaves it installed too.

Thanks,
Pedro Alves
  

On 06/05/2017 11:21 AM, Pedro Alves wrote:
> On 06/04/2017 11:53 PM, Tom Tromey wrote:
>> @@ -452,17 +450,14 @@ catch_exception_command_1 (enum exception_event_kind ex_event,
>>  			   char *arg_entry,
>>  			   int tempflag, int from_tty)
>>  {
>> -  char *except_rx;
>>    const char *cond_string = NULL;
>> -  struct cleanup *cleanup;
>>    const char *arg = arg_entry;
>>  
>>    if (!arg)
>>      arg = "";
>>    arg = skip_spaces_const (arg);
>>  
>> -  except_rx = extract_exception_regexp (&arg);
>> -  cleanup = make_cleanup (xfree, except_rx);
>> +  std::string except_rx = extract_exception_regexp (&arg);
>>  
>>    cond_string = ep_parse_optional_if_clause (&arg);
>>  
>> @@ -474,10 +469,8 @@ catch_exception_command_1 (enum exception_event_kind ex_event,
>>        && ex_event != EX_EVENT_RETHROW)
>>      error (_("Unsupported or unknown exception event; cannot catch it"));
>>  
>> -  handle_gnu_v3_exceptions (tempflag, except_rx, cond_string,
>> +  handle_gnu_v3_exceptions (tempflag, std::move (except_rx), cond_string,
>>  			    ex_event, from_tty);
>> -
>> -  discard_cleanups (cleanup);
>>  }
> 
> Something looks suspicious to me -- compile_rx_or_error returns with
> an installed cleanup that calls regfree, and handle_gnu_v3_exceptions
> leaves it installed too.

Maybe the cleanest would be to add a wrapper around regex_t,
and replace compile_rx_or_error with a ctor that throws,
like:

class gdb_regex
{
public:
  // replaces old compile_rx_or_error
  gdb_regex (const char *rx, const char *message)
  {
    gdb_assert (rx != NULL);

    int code = regcomp (&m_pattern, rx, REG_NOSUB);
    if (code != 0)
      {
        gdb::unique_xmalloc_ptr<char> err
          = get_regcomp_error (code, &m_pattern);

        error (("%s: %s"), message, err.get ());
      }
  }
  ~gdb_regex ()
  {
    regfree (&m_pattern);
  }

  // add convenience methods as needed.

private:
  regex_t m_pattern;
};

and use it throughout.

Thanks,
Pedro Alves
  

On 06/05/2017 11:33 AM, Pedro Alves wrote:
> class gdb_regex
> {
> public:
>   // replaces old compile_rx_or_error
>   gdb_regex (const char *rx, const char *message)
>   {

Maybe call it "compiled_regex" instead, since you wouldn't
be able to end up with a not-compiled-yet regex with this.

Thanks,
Pedro Alves
  

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> From what I understand, we were freeing the regex with regfree, but we
Simon> weren't freeing the regex_t object, which is allocated separately, is
Simon> that right?

Yes, I think that's a leak in the current code.

Tom