[RFA] C++-ify parser_state

  This mildly C++-ifies parser_state and stap_parse_info -- just enough
to remove some cleanups.

Regression tested by the buildbot.

ChangeLog
2017-11-26  Tom Tromey  <tom@tromey.com>

	* stap-probe.h (struct stap_parse_info): Add constructor,
	destructor.
	* stap-probe.c (stap_parse_argument): Update.
	* rust-exp.y (rust_lex_tests): Update.
	* parser-defs.h (struct parser_state): Add constructor,
	destructor, release method.
	(initialize_expout, reallocate_expout): Remove.
	* parse.c (parser_state::parser_state): Rename from
	initialize_expout.
	(parser_state::~parser_state): New.
	(parser_state::release): Rename from reallocate_expout.
	(parse_exp_in_context_1): Update.
	* dtrace-probe.c (dtrace_probe::build_arg_exprs): Update.
---
 gdb/ChangeLog      | 16 ++++++++++++++
 gdb/dtrace-probe.c | 11 ++--------
 gdb/parse.c        | 63 ++++++++++++++++++++++++++++++------------------------
 gdb/parser-defs.h  | 32 +++++++++++++--------------
 gdb/rust-exp.y     |  3 +--
 gdb/stap-probe.c   | 15 ++-----------
 gdb/stap-probe.h   | 17 +++++++++++++++
 7 files changed, 88 insertions(+), 69 deletions(-)
  

On Sunday, November 26 2017, Tom Tromey wrote:

> This mildly C++-ifies parser_state and stap_parse_info -- just enough
> to remove some cleanups.

Thanks for the patch, Tom.

This was on my TODO list for a while now.  I looked it over and it seems
good to me.

> Regression tested by the buildbot.
>
> ChangeLog
> 2017-11-26  Tom Tromey  <tom@tromey.com>
>
> 	* stap-probe.h (struct stap_parse_info): Add constructor,
> 	destructor.
> 	* stap-probe.c (stap_parse_argument): Update.
> 	* rust-exp.y (rust_lex_tests): Update.
> 	* parser-defs.h (struct parser_state): Add constructor,
> 	destructor, release method.
> 	(initialize_expout, reallocate_expout): Remove.
> 	* parse.c (parser_state::parser_state): Rename from
> 	initialize_expout.
> 	(parser_state::~parser_state): New.
> 	(parser_state::release): Rename from reallocate_expout.
> 	(parse_exp_in_context_1): Update.
> 	* dtrace-probe.c (dtrace_probe::build_arg_exprs): Update.
> ---
>  gdb/ChangeLog      | 16 ++++++++++++++
>  gdb/dtrace-probe.c | 11 ++--------
>  gdb/parse.c        | 63 ++++++++++++++++++++++++++++++------------------------
>  gdb/parser-defs.h  | 32 +++++++++++++--------------
>  gdb/rust-exp.y     |  3 +--
>  gdb/stap-probe.c   | 15 ++-----------
>  gdb/stap-probe.h   | 17 +++++++++++++++
>  7 files changed, 88 insertions(+), 69 deletions(-)
>
> diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
> index 1c88f89054..c0134dcac5 100644
> --- a/gdb/dtrace-probe.c
> +++ b/gdb/dtrace-probe.c
> @@ -626,21 +626,15 @@ dtrace_probe::build_arg_exprs (struct gdbarch *gdbarch)
>       value of the argument when executed at the PC of the probe.  */
>    for (dtrace_probe_arg &arg : m_args)
>      {
> -      struct cleanup *back_to;
> -      struct parser_state pstate;
> -
>        /* Initialize the expression buffer in the parser state.  The
>  	 language does not matter, since we are using our own
>  	 parser.  */
> -      initialize_expout (&pstate, 10, current_language, gdbarch);
> -      back_to = make_cleanup (free_current_contents, &pstate.expout);
> +      parser_state pstate (10, current_language, gdbarch);
>  
>        /* The argument value, which is ABI dependent and casted to
>  	 `long int'.  */
>        gdbarch_dtrace_parse_probe_argument (gdbarch, &pstate, argc);
>  
> -      discard_cleanups (back_to);
> -
>        /* Casting to the expected type, but only if the type was
>  	 recognized at probe load time.  Otherwise the argument will
>  	 be evaluated as the long integer passed to the probe.  */
> @@ -651,8 +645,7 @@ dtrace_probe::build_arg_exprs (struct gdbarch *gdbarch)
>  	  write_exp_elt_opcode (&pstate, UNOP_CAST);
>  	}
>  
> -      reallocate_expout (&pstate);
> -      arg.expr = expression_up (pstate.expout);
> +      arg.expr = pstate.release ();
>        prefixify_expression (arg.expr.get ());
>        ++argc;
>      }
> diff --git a/gdb/parse.c b/gdb/parse.c
> index 506efa38fb..3a7b7af4e1 100644
> --- a/gdb/parse.c
> +++ b/gdb/parse.c
> @@ -152,34 +152,43 @@ end_arglist (void)
>  
>  /* See definition in parser-defs.h.  */
>  
> -void
> -initialize_expout (struct parser_state *ps, size_t initial_size,
> -		   const struct language_defn *lang,
> -		   struct gdbarch *gdbarch)
> +parser_state::parser_state (size_t initial_size,
> +			    const struct language_defn *lang,
> +			    struct gdbarch *gdbarch)
> +  : expout_size (initial_size),
> +    expout_ptr (0)
>  {
> -  ps->expout_size = initial_size;
> -  ps->expout_ptr = 0;
> -  ps->expout
> +  expout
>      = (struct expression *) xmalloc (sizeof (struct expression)
> -				     + EXP_ELEM_TO_BYTES (ps->expout_size));
> -  ps->expout->language_defn = lang;
> -  ps->expout->gdbarch = gdbarch;
> +				     + EXP_ELEM_TO_BYTES (expout_size));
> +  expout->language_defn = lang;
> +  expout->gdbarch = gdbarch;
>  }
>  
>  /* See definition in parser-defs.h.  */
>  
> -void
> -reallocate_expout (struct parser_state *ps)
> +parser_state::~parser_state ()
> +{
> +  xfree (expout);
> +}
> +
> +expression_up
> +parser_state::release ()
>  {
>    /* Record the actual number of expression elements, and then
>       reallocate the expression memory so that we free up any
>       excess elements.  */
>  
> -  ps->expout->nelts = ps->expout_ptr;
> -  ps->expout = (struct expression *)
> -     xrealloc (ps->expout,
> +  expout->nelts = expout_ptr;
> +  expout = (struct expression *)
> +     xrealloc (expout,
>  	       sizeof (struct expression)
> -	       + EXP_ELEM_TO_BYTES (ps->expout_ptr));
> +	       + EXP_ELEM_TO_BYTES (expout_ptr));
> +
> +  expression_up result (expout);
> +  /* Ensure that we don't free it in the destructor.  */
> +  expout = nullptr;
> +  return result;
>  }
>  
>  /* This page contains the functions for adding data to the struct expression
> @@ -1118,7 +1127,6 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc,
>  			int comma, int void_context_p, int *out_subexp)
>  {
>    const struct language_defn *lang = NULL;
> -  struct parser_state ps;
>    int subexp;
>  
>    lexptr = *stringptr;
> @@ -1194,7 +1202,7 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc,
>       and others called from *.y) ensure CURRENT_LANGUAGE gets restored
>       to the value matching SELECTED_FRAME as set by get_current_arch.  */
>  
> -  initialize_expout (&ps, 10, lang, get_current_arch ());
> +  parser_state ps (10, lang, get_current_arch ());
>  
>    scoped_restore_current_language lang_saver;
>    set_language (lang->la_language);
> @@ -1207,33 +1215,32 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc,
>    CATCH (except, RETURN_MASK_ALL)
>      {
>        if (! parse_completion)
> -	{
> -	  xfree (ps.expout);
> -	  throw_exception (except);
> -	}
> +	throw_exception (except);
>      }
>    END_CATCH
>  
> -  reallocate_expout (&ps);
> +  /* We have to operate on an "expression *", due to la_post_parser,
> +     which explains this funny-looking double release.  */
> +  struct expression *result = ps.release ().release ();
>  
>    /* Convert expression from postfix form as generated by yacc
>       parser, to a prefix form.  */
>  
>    if (expressiondebug)
> -    dump_raw_expression (ps.expout, gdb_stdlog,
> +    dump_raw_expression (result, gdb_stdlog,
>  			 "before conversion to prefix form");
>  
> -  subexp = prefixify_expression (ps.expout);
> +  subexp = prefixify_expression (result);
>    if (out_subexp)
>      *out_subexp = subexp;
>  
> -  lang->la_post_parser (&ps.expout, void_context_p);
> +  lang->la_post_parser (&result, void_context_p);
>  
>    if (expressiondebug)
> -    dump_prefix_expression (ps.expout, gdb_stdlog);
> +    dump_prefix_expression (result, gdb_stdlog);
>  
>    *stringptr = lexptr;
> -  return expression_up (ps.expout);
> +  return expression_up (result);
>  }
>  
>  /* Parse STRING as an expression, and complain if this fails
> diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
> index f43fb75df2..262b81b056 100644
> --- a/gdb/parser-defs.h
> +++ b/gdb/parser-defs.h
> @@ -37,6 +37,21 @@ extern int parser_debug;
>  
>  struct parser_state
>  {
> +  /* Constructor.  INITIAL_SIZE is the initial size of the expout
> +     array.  LANG is the language used to parse the expression.  And
> +     GDBARCH is the gdbarch to use during parsing.  */
> +
> +  parser_state (size_t initial_size, const struct language_defn *lang,
> +		struct gdbarch *gdbarch);
> +  ~parser_state ();
> +
> +  DISABLE_COPY_AND_ASSIGN (parser_state);
> +
> +  /* Resize the allocated expression to the correct size, and return
> +     it as an expression_up -- passing ownership to the caller.  */
> +  expression_up release ();
> +
> +
>    /* The expression related to this parser state.  */
>  
>    struct expression *expout;
> @@ -156,23 +171,6 @@ struct type_stack
>    int size;
>  };
>  
> -/* Helper function to initialize the expout, expout_size, expout_ptr
> -   trio inside PS before it is used to store expression elements created
> -   during the parsing of an expression.  INITIAL_SIZE is the initial size of
> -   the expout array.  LANG is the language used to parse the expression.
> -   And GDBARCH is the gdbarch to use during parsing.  */
> -
> -extern void initialize_expout (struct parser_state *ps,
> -			       size_t initial_size,
> -			       const struct language_defn *lang,
> -			       struct gdbarch *gdbarch);
> -
> -/* Helper function that reallocates the EXPOUT inside PS in order to
> -   eliminate any unused space.  It is generally used when the expression
> -   has just been parsed and created.  */
> -
> -extern void reallocate_expout (struct parser_state *ps);
> -
>  /* Reverse an expression from suffix form (in which it is constructed)
>     to prefix form (in which we can conveniently print or execute it).
>     Ordinarily this always returns -1.  However, if EXPOUT_LAST_STRUCT
> diff --git a/gdb/rust-exp.y b/gdb/rust-exp.y
> index e372a6ea1e..8f1455bbf0 100644
> --- a/gdb/rust-exp.y
> +++ b/gdb/rust-exp.y
> @@ -2670,8 +2670,7 @@ rust_lex_tests (void)
>  						       &test_obstack);
>  
>    // Set up dummy "parser", so that rust_type works.
> -  struct parser_state ps;
> -  initialize_expout (&ps, 0, &rust_language_defn, target_gdbarch ());
> +  struct parser_state ps (0, &rust_language_defn, target_gdbarch ());
>    rust_parser parser (&ps);
>  
>    rust_lex_test_one ("", 0);
> diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
> index 9f4e00845a..d8a67c750a 100644
> --- a/gdb/stap-probe.c
> +++ b/gdb/stap-probe.c
> @@ -1146,25 +1146,17 @@ static expression_up
>  stap_parse_argument (const char **arg, struct type *atype,
>  		     struct gdbarch *gdbarch)
>  {
> -  struct stap_parse_info p;
> -  struct cleanup *back_to;
> -
>    /* We need to initialize the expression buffer, in order to begin
>       our parsing efforts.  We use language_c here because we may need
>       to do pointer arithmetics.  */
> -  initialize_expout (&p.pstate, 10, language_def (language_c), gdbarch);
> -  back_to = make_cleanup (free_current_contents, &p.pstate.expout);
> +  struct stap_parse_info p (10, language_def (language_c), gdbarch);
>  
>    p.saved_arg = *arg;
>    p.arg = *arg;
>    p.arg_type = atype;
> -  p.gdbarch = gdbarch;
> -  p.inside_paren_p = 0;
>  
>    stap_parse_argument_1 (&p, 0, STAP_OPERAND_PREC_NONE);
>  
> -  discard_cleanups (back_to);
> -
>    gdb_assert (p.inside_paren_p == 0);
>  
>    /* Casting the final expression to the appropriate type.  */
> @@ -1172,13 +1164,10 @@ stap_parse_argument (const char **arg, struct type *atype,
>    write_exp_elt_type (&p.pstate, atype);
>    write_exp_elt_opcode (&p.pstate, UNOP_CAST);
>  
> -  reallocate_expout (&p.pstate);
> -
>    p.arg = skip_spaces (p.arg);
>    *arg = p.arg;
>  
> -  /* We can safely return EXPOUT here.  */
> -  return expression_up (p.pstate.expout);
> +  return p.pstate.release ();
>  }
>  
>  /* Implementation of 'parse_arguments' method.  */
> diff --git a/gdb/stap-probe.h b/gdb/stap-probe.h
> index c3327e6999..af24a380e0 100644
> --- a/gdb/stap-probe.h
> +++ b/gdb/stap-probe.h
> @@ -28,6 +28,23 @@
>  
>  struct stap_parse_info
>  {
> +  /* Constructor.  Arguments are passed to the parser_state
> +     constructor.  */
> +  stap_parse_info (size_t initial_size, const struct language_defn *lang,
> +		   struct gdbarch *gdbarch)
> +    : arg (nullptr),
> +      pstate (initial_size, lang, gdbarch),
> +      saved_arg (nullptr),
> +      arg_type (nullptr),
> +      gdbarch (gdbarch),
> +      inside_paren_p (0)
> +  {
> +  }
> +
> +  ~stap_parse_info () = default;
> +
> +  DISABLE_COPY_AND_ASSIGN (stap_parse_info);
> +
>    /* The probe's argument in a string format.  */
>    const char *arg;
>  
> -- 
> 2.13.6
  

Hi Tom,

I think this looks good.  I have some suggestions to clean things a further
bit, you are free to take them or leave them

On 2017-11-26 12:40 PM, Tom Tromey wrote:

> +expression_up
> +parser_state::release ()
>  {
>    /* Record the actual number of expression elements, and then
>       reallocate the expression memory so that we free up any
>       excess elements.  */
>  
> -  ps->expout->nelts = ps->expout_ptr;
> -  ps->expout = (struct expression *)
> -     xrealloc (ps->expout,
> +  expout->nelts = expout_ptr;
> +  expout = (struct expression *)
> +     xrealloc (expout,
>  	       sizeof (struct expression)
> -	       + EXP_ELEM_TO_BYTES (ps->expout_ptr));
> +	       + EXP_ELEM_TO_BYTES (expout_ptr));
> +
> +  expression_up result (expout);
> +  /* Ensure that we don't free it in the destructor.  */
> +  expout = nullptr;
> +  return result;

If expout was an expression_up, we could just std::move it here, and
wouldn't need an explicit destructor.

>  }
>  
>  /* This page contains the functions for adding data to the struct expression
> @@ -1118,7 +1127,6 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc,
>  			int comma, int void_context_p, int *out_subexp)
>  {
>    const struct language_defn *lang = NULL;
> -  struct parser_state ps;
>    int subexp;
>  
>    lexptr = *stringptr;
> @@ -1194,7 +1202,7 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc,
>       and others called from *.y) ensure CURRENT_LANGUAGE gets restored
>       to the value matching SELECTED_FRAME as set by get_current_arch.  */
>  
> -  initialize_expout (&ps, 10, lang, get_current_arch ());
> +  parser_state ps (10, lang, get_current_arch ());
>  
>    scoped_restore_current_language lang_saver;
>    set_language (lang->la_language);
> @@ -1207,33 +1215,32 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc,
>    CATCH (except, RETURN_MASK_ALL)
>      {
>        if (! parse_completion)
> -	{
> -	  xfree (ps.expout);
> -	  throw_exception (except);
> -	}
> +	throw_exception (except);
>      }
>    END_CATCH
>  
> -  reallocate_expout (&ps);
> +  /* We have to operate on an "expression *", due to la_post_parser,
> +     which explains this funny-looking double release.  */
> +  struct expression *result = ps.release ().release ();
>  
>    /* Convert expression from postfix form as generated by yacc
>       parser, to a prefix form.  */
>  
>    if (expressiondebug)
> -    dump_raw_expression (ps.expout, gdb_stdlog,
> +    dump_raw_expression (result, gdb_stdlog,
>  			 "before conversion to prefix form");
>  
> -  subexp = prefixify_expression (ps.expout);
> +  subexp = prefixify_expression (result);
>    if (out_subexp)
>      *out_subexp = subexp;
>  
> -  lang->la_post_parser (&ps.expout, void_context_p);
> +  lang->la_post_parser (&result, void_context_p);

Passing a pointer or reference to the unique_ptr would allow
the implementations of la_post_parser to modify it directly,
and avoid the .release ().release ().

> --- a/gdb/stap-probe.c
> +++ b/gdb/stap-probe.c
> @@ -1146,25 +1146,17 @@ static expression_up
>  stap_parse_argument (const char **arg, struct type *atype,
>  		     struct gdbarch *gdbarch)
>  {
> -  struct stap_parse_info p;
> -  struct cleanup *back_to;
> -
>    /* We need to initialize the expression buffer, in order to begin
>       our parsing efforts.  We use language_c here because we may need
>       to do pointer arithmetics.  */
> -  initialize_expout (&p.pstate, 10, language_def (language_c), gdbarch);
> -  back_to = make_cleanup (free_current_contents, &p.pstate.expout);
> +  struct stap_parse_info p (10, language_def (language_c), gdbarch);
>  
>    p.saved_arg = *arg;
>    p.arg = *arg;
>    p.arg_type = atype;
> -  p.gdbarch = gdbarch;
> -  p.inside_paren_p = 0;

Why not pass the other arguments to the constructor (*arg and atype)?

Simon
  

>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:

>> +  /* Ensure that we don't free it in the destructor.  */
>> +  expout = nullptr;
>> +  return result;

Simon> If expout was an expression_up, we could just std::move it here, and
Simon> wouldn't need an explicit destructor.

I thought that looked somewhat difficult due to the use of xrealloc when
growing the expression.

Really, of course, the whole expression structure needs to be redone.
That's a big task though.

>> -  lang->la_post_parser (&ps.expout, void_context_p);
>> +  lang->la_post_parser (&result, void_context_p);

Simon> Passing a pointer or reference to the unique_ptr would allow
Simon> the implementations of la_post_parser to modify it directly,
Simon> and avoid the .release ().release ().

I'll look into it.
The realloc thing may be an issue here as well.

>> p.saved_arg = *arg;
>> p.arg = *arg;
>> p.arg_type = atype;
>> -  p.gdbarch = gdbarch;
>> -  p.inside_paren_p = 0;

Simon> Why not pass the other arguments to the constructor (*arg and atype)?

Yeah, this one I'll do for sure.

Tom
  

On 2017-11-29 22:29, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
> 
>>> +  /* Ensure that we don't free it in the destructor.  */
>>> +  expout = nullptr;
>>> +  return result;
> 
> Simon> If expout was an expression_up, we could just std::move it here, 
> and
> Simon> wouldn't need an explicit destructor.
> 
> I thought that looked somewhat difficult due to the use of xrealloc 
> when
> growing the expression.
> 
> Really, of course, the whole expression structure needs to be redone.
> That's a big task though.

Agreed.

>>> -  lang->la_post_parser (&ps.expout, void_context_p);
>>> +  lang->la_post_parser (&result, void_context_p);
> 
> Simon> Passing a pointer or reference to the unique_ptr would allow
> Simon> the implementations of la_post_parser to modify it directly,
> Simon> and avoid the .release ().release ().
> 
> I'll look into it.
> The realloc thing may be an issue here as well.

I think this should be enough (the commit on top):

https://github.com/simark/binutils-gdb/commits/parser_state

Simon
  

On 2017-11-29 22:54, Simon Marchi wrote:
> On 2017-11-29 22:29, Tom Tromey wrote:
>> Simon> Passing a pointer or reference to the unique_ptr would allow
>> Simon> the implementations of la_post_parser to modify it directly,
>> Simon> and avoid the .release ().release ().
>> 
>> I'll look into it.
>> The realloc thing may be an issue here as well.
> 
> I think this should be enough (the commit on top):
> 
> https://github.com/simark/binutils-gdb/commits/parser_state
> 
> Simon

Oops, I realized I only answered half the question.  I added a second 
patch on top.

Simon