[v2,1/5] gdb: Do not create variables when parsing expressions

  We do this by introducing a new type of expression for internalvars
that had not been given a value at parse time.

This is not particularly useful on its own, but allows us to parse
expressions without having side-effects on the set of internalvars.

This will then allow us to tab-complete internalvars in expressions
without creating new internalvars, which would otherwise defeat the
purpose of tab completion (completing only variables that have been
set).
---
 gdb/ada-exp.h       | 12 ++++++++--
 gdb/ax-gdb.c        | 53 ++++++++++++++++++++++++++++++++++++-------
 gdb/cli/cli-utils.c |  2 +-
 gdb/expop.h         | 55 ++++++++++++++++++++++++++++++++++++++++++++-
 gdb/parse.c         |  7 +++---
 5 files changed, 113 insertions(+), 16 deletions(-)
  

Hi,

Thank you for resubmitting this patch and (especially) including
new tests. This is going to be so much easier to review.

On 8/27/24 6:49 PM, Antonio Rische wrote:
> We do this by introducing a new type of expression for internalvars
> that had not been given a value at parse time.
> 
> This is not particularly useful on its own, but allows us to parse
> expressions without having side-effects on the set of internalvars.

This is a nice bonus side-effect of this series. I've always found
it annoying that convenience variables are created by default like this.
This is, however, a slight change in established behavior which I
think should be explicitly called out in your commit message. A
NEWS entry is justified for this new feature. Users should definitely
be aware of this!

A trivial update of "Command Completion" mentioning convenience
variables/functions in the user manual would be a welcome addition.

In general, this looks very good, but there are a handful of GDB-
specific style issues that I'd like to point out. I encourage you
to peruse the Contribution Checklist:

https://sourceware.org/gdb/wiki/ContributionChecklist

That has a link to our (sometimes quirky) coding standards and other
useful information.

> ---
>   gdb/ada-exp.h       | 12 ++++++++--
>   gdb/ax-gdb.c        | 53 ++++++++++++++++++++++++++++++++++++-------
>   gdb/cli/cli-utils.c |  2 +-
>   gdb/expop.h         | 55 ++++++++++++++++++++++++++++++++++++++++++++-
>   gdb/parse.c         |  7 +++---
>   5 files changed, 113 insertions(+), 16 deletions(-)
> 
> diff --git a/gdb/ada-exp.h b/gdb/ada-exp.h
> index 94e4ea0f4..3d267b61c 100644
> --- a/gdb/ada-exp.h
> +++ b/gdb/ada-exp.h
> @@ -534,8 +534,16 @@ class ada_assign_operation
>        assignment.  */
>     value *eval_for_resolution (struct expression *exp)
>     {
> -    return std::get<0> (m_storage)->evaluate (nullptr, exp,
> -					      EVAL_AVOID_SIDE_EFFECTS);
> +    operation *lhs_op = std::get<0> (m_storage).get();

Space after function name: "get ()".

> +
> +    /* If the operation is an internalvar that was not initialized at
> +     parse time, ensure that it exists. */
> +    auto *uninit_var_op = dynamic_cast<uninit_internalvar_operation *>
> +			  (lhs_op);
> +    if (uninit_var_op)

Since uninit_var_op is a pointer, we stylistically use explicit 
comparison with nullptr.

> +      lookup_internalvar (uninit_var_op->get_name());

Space after function name again.

> +
> +    return lhs_op->evaluate (nullptr, exp, EVAL_AVOID_SIDE_EFFECTS);
>     }
>   
>     /* The parser must construct the assignment node before parsing the
> diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
> index 3d619012b..c70aef023 100644
> --- a/gdb/ax-gdb.c
> +++ b/gdb/ax-gdb.c
> @@ -1668,6 +1668,31 @@ register_operation::do_generate_ax (struct expression *exp,
>     value->type = register_type (ax->gdbarch, reg);
>   }
>   
> +void
> +uninit_internalvar_operation::do_generate_ax (struct expression *exp,

All functions should have a comment, even if that is simply "See foo.h."

> +				       struct agent_expr *ax,
> +				       struct axs_value *value,
> +				       struct type *cast_type)
> +{
> +  const char *name = get_name ();
> +  struct internalvar *var = lookup_only_internalvar(name);

Space after function name.

> +  struct trace_state_variable *tsv;
> +
> +  tsv = find_trace_state_variable (name);
> +  if (tsv)

Use explicit comparison with nullptr.

> +    {
> +      ax_tsv (ax, aop_getv, tsv->number);
> +      if (ax->tracing)

Likewise.

> +	ax_tsv (ax, aop_tracev, tsv->number);
> +      /* Trace state variables are always 64-bit integers.  */
> +      value->kind = axs_rvalue;
> +      value->type = builtin_type (ax->gdbarch)->builtin_long_long;
> +    }
> +  else if (! compile_internalvar_to_ax (var, ax, value))

No space after '!'.

> +    error (_("$%s is not a trace state variable; GDB agent "
> +	     "expressions cannot use convenience variables."), name);
> +}
> +
>   void
>   internalvar_operation::do_generate_ax (struct expression *exp,
>   				       struct agent_expr *ax,
> @@ -1911,6 +1936,24 @@ op_this_operation::do_generate_ax (struct expression *exp,
>   	   sym->print_name ());
>   }
>   
> +static const char *

Missing comment describing function.

> +internalvar_op_name (operation *op)
> +{
> +  gdb_assert (op->opcode () == OP_INTERNALVAR);
> +
> +  internalvar_operation *ivar_op
> +    = dynamic_cast<internalvar_operation *> (op);
> +  if (ivar_op != nullptr)
> +    return internalvar_name (ivar_op->get_internalvar ());
> +
> +  uninit_internalvar_operation *uninit_ivar_op
> +    = dynamic_cast<uninit_internalvar_operation *> (op);
> +  if (uninit_ivar_op != nullptr)
> +    return uninit_ivar_op->get_name ();
> +
> +  return nullptr;
> +}
> +
>   void
>   assign_operation::do_generate_ax (struct expression *exp,
>   				  struct agent_expr *ax,
> @@ -1921,10 +1964,7 @@ assign_operation::do_generate_ax (struct expression *exp,
>     if (subop->opcode () != OP_INTERNALVAR)
>       error (_("May only assign to trace state variables"));
>   
> -  internalvar_operation *ivarop
> -    = gdb::checked_static_cast<internalvar_operation *> (subop);
> -
> -  const char *name = internalvar_name (ivarop->get_internalvar ());
> +  const char *name = internalvar_op_name (subop);
>     struct trace_state_variable *tsv;
>   
>     std::get<1> (m_storage)->generate_ax (exp, ax, value);
> @@ -1950,10 +1990,7 @@ assign_modify_operation::do_generate_ax (struct expression *exp,
>     if (subop->opcode () != OP_INTERNALVAR)
>       error (_("May only assign to trace state variables"));
>   
> -  internalvar_operation *ivarop
> -    = gdb::checked_static_cast<internalvar_operation *> (subop);
> -
> -  const char *name = internalvar_name (ivarop->get_internalvar ());
> +  const char *name = internalvar_op_name (subop);
>     struct trace_state_variable *tsv;
>   
>     tsv = find_trace_state_variable (name);
> diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
> index 45b30842e..7dedb3829 100644
> --- a/gdb/cli/cli-utils.c
> +++ b/gdb/cli/cli-utils.c
> @@ -49,7 +49,7 @@ get_ulongest (const char **pp, int trailer)
>   	  while (isalnum (*p) || *p == '_')
>   	    p++;
>   	  std::string varname (start, p - start);
> -	  if (!get_internalvar_integer (lookup_internalvar (varname.c_str ()),
> +	  if (!get_internalvar_integer (lookup_only_internalvar (varname.c_str ()),
>   				       &retval))
>   	    error (_("Convenience variable $%s does not have integer value."),
>   		   varname.c_str ());
> diff --git a/gdb/expop.h b/gdb/expop.h
> index 2d46a9dad..921caf4ce 100644
> --- a/gdb/expop.h
> +++ b/gdb/expop.h
> @@ -878,6 +878,42 @@ class bool_operation
>     { return true; }
>   };
>   
> +/* Reference to a variable that had not been defined at parse time.  */
> +class uninit_internalvar_operation
> +  : public tuple_holding_operation<std::string>
> +{
> +public:
> +
> +  using tuple_holding_operation::tuple_holding_operation;
> +
> +  value *evaluate (struct type *expect_type,
> +		   struct expression *exp,
> +		   enum noside noside) override
> +  {
> +    internalvar* iv = lookup_only_internalvar(std::get<0> (m_storage).c_str ());
> +    if (!iv)
> +      return value::allocate (builtin_type (exp->gdbarch)->builtin_void);

We prefer "internalvar *iv". Explicit comparison w/nullptr.

> +
> +    return value_of_internalvar (exp->gdbarch, iv);
> +  }
> +
> +  const char *get_name () const
> +  {
> +    return std::get<0> (m_storage).c_str ();
> +  }
> +
> +  enum exp_opcode opcode () const override
> +  { return OP_INTERNALVAR; }
> +
> +protected:
> +
> +  void do_generate_ax (struct expression *exp,
> +		       struct agent_expr *ax,
> +		       struct axs_value *value,
> +		       struct type *cast_type)
> +    override;
> +};
> +
>   class internalvar_operation
>     : public tuple_holding_operation<internalvar *>
>   {
> @@ -1891,7 +1927,16 @@ class assign_operation
>   		   struct expression *exp,
>   		   enum noside noside) override
>     {
> -    value *lhs = std::get<0> (m_storage)->evaluate (nullptr, exp, noside);
> +    operation *lhs_op = std::get<0> (m_storage).get();
> +
> +    /* If the operation is an internalvar that was not initialized at
> +       parse time, ensure that it exists. */
> +    auto *uninit_var_op = dynamic_cast<uninit_internalvar_operation *> (lhs_op);
> +    if (uninit_var_op)

Compare w/nullptr.

> +      lookup_internalvar (uninit_var_op->get_name());

Space after function name.

> +
> +    value *lhs = lhs_op->evaluate (nullptr, exp, noside);
> +
>       /* Special-case assignments where the left-hand-side is a
>          convenience variable -- in these, don't bother setting an
>          expected type.  This avoids a weird case where re-assigning a
> @@ -1940,6 +1985,14 @@ class assign_modify_operation
>   		   struct expression *exp,
>   		   enum noside noside) override
>     {
> +    operation *lhs_op = std::get<1> (m_storage).get();

Space after function name.

> +
> +    /* If the operation is an internalvar that was not initialized at
> +       parse time, ensure that it exists. */
> +    auto *uninit_var_op = dynamic_cast<uninit_internalvar_operation *> (lhs_op);
> +    if (uninit_var_op)
> +      lookup_internalvar (uninit_var_op->get_name());

Compare w/nullptr. Space after function name.

> +
>       value *lhs = std::get<1> (m_storage)->evaluate (nullptr, exp, noside);
>       value *rhs = std::get<2> (m_storage)->evaluate (expect_type, exp, noside);
>       return eval_binop_assign_modify (expect_type, exp, noside,
> diff --git a/gdb/parse.c b/gdb/parse.c
> index e0837de7b..b6327956e 100644
> --- a/gdb/parse.c
> +++ b/gdb/parse.c
> @@ -240,10 +240,9 @@ parser_state::push_dollar (struct stoken str)
>         return;
>       }
>   
> -  /* Any other names are assumed to be debugger internal variables.  */
> -
> -  push_new<expr::internalvar_operation>
> -    (create_internalvar (copy.c_str () + 1));
> +  /* Any other names are assumed to be debugger internal variables which
> +     have not yet been initialized.  */
> +  push_new<expr::uninit_internalvar_operation> (copy.c_str () + 1);
>   }
>   
>   /* See parser-defs.h.  */

Thank you for your work on this. I really appreciate it.

Keith
  

I'll make all these changes.

Is there any automated formatter that can help comply with the style rules? I couldn't manage to get astyle, indent, or clang-format to generate output that followed the specified indentation/whitespace rules.

Thanks,
Antonio

On Friday, August 30th, 2024 at 7:27 PM, Keith Seitz <keiths@redhat.com> wrote:

> Hi,
>
> Thank you for resubmitting this patch and (especially) including
> new tests. This is going to be so much easier to review.
>
> On 8/27/24 6:49 PM, Antonio Rische wrote:
>
> > We do this by introducing a new type of expression for internalvars
> > that had not been given a value at parse time.
> >
> > This is not particularly useful on its own, but allows us to parse
> > expressions without having side-effects on the set of internalvars.
>
>
> This is a nice bonus side-effect of this series. I've always found
> it annoying that convenience variables are created by default like this.
> This is, however, a slight change in established behavior which I
> think should be explicitly called out in your commit message. A
> NEWS entry is justified for this new feature. Users should definitely
> be aware of this!
>
> A trivial update of "Command Completion" mentioning convenience
> variables/functions in the user manual would be a welcome addition.
>
> In general, this looks very good, but there are a handful of GDB-
> specific style issues that I'd like to point out. I encourage you
> to peruse the Contribution Checklist:
>
> https://sourceware.org/gdb/wiki/ContributionChecklist
>
> That has a link to our (sometimes quirky) coding standards and other
> useful information.
>
> > ---
> > gdb/ada-exp.h | 12 ++++++++--
> > gdb/ax-gdb.c | 53 ++++++++++++++++++++++++++++++++++++-------
> > gdb/cli/cli-utils.c | 2 +-
> > gdb/expop.h | 55 ++++++++++++++++++++++++++++++++++++++++++++-
> > gdb/parse.c | 7 +++---
> > 5 files changed, 113 insertions(+), 16 deletions(-)
> >
> > diff --git a/gdb/ada-exp.h b/gdb/ada-exp.h
> > index 94e4ea0f4..3d267b61c 100644
> > --- a/gdb/ada-exp.h
> > +++ b/gdb/ada-exp.h
> > @@ -534,8 +534,16 @@ class ada_assign_operation
> > assignment. */
> > value *eval_for_resolution (struct expression *exp)
> > {
> > - return std::get<0> (m_storage)->evaluate (nullptr, exp,
> > - EVAL_AVOID_SIDE_EFFECTS);
> > + operation *lhs_op = std::get<0> (m_storage).get();
>
>
> Space after function name: "get ()".
>
> > +
> > + /* If the operation is an internalvar that was not initialized at
> > + parse time, ensure that it exists. */
> > + auto *uninit_var_op = dynamic_cast<uninit_internalvar_operation *>
> > + (lhs_op);
> > + if (uninit_var_op)
>
>
> Since uninit_var_op is a pointer, we stylistically use explicit
> comparison with nullptr.
>
> > + lookup_internalvar (uninit_var_op->get_name());
>
>
> Space after function name again.
>
> > +
> > + return lhs_op->evaluate (nullptr, exp, EVAL_AVOID_SIDE_EFFECTS);
> > }
> >
> > /* The parser must construct the assignment node before parsing the
> > diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
> > index 3d619012b..c70aef023 100644
> > --- a/gdb/ax-gdb.c
> > +++ b/gdb/ax-gdb.c
> > @@ -1668,6 +1668,31 @@ register_operation::do_generate_ax (struct expression *exp,
> > value->type = register_type (ax->gdbarch, reg);
> > }
> >
> > +void
> > +uninit_internalvar_operation::do_generate_ax (struct expression *exp,
>
>
> All functions should have a comment, even if that is simply "See foo.h."
>
> > + struct agent_expr *ax,
> > + struct axs_value *value,
> > + struct type *cast_type)
> > +{
> > + const char *name = get_name ();
> > + struct internalvar *var = lookup_only_internalvar(name);
>
>
> Space after function name.
>
> > + struct trace_state_variable *tsv;
> > +
> > + tsv = find_trace_state_variable (name);
> > + if (tsv)
>
>
> Use explicit comparison with nullptr.
>
> > + {
> > + ax_tsv (ax, aop_getv, tsv->number);
> > + if (ax->tracing)
>
>
> Likewise.
>
> > + ax_tsv (ax, aop_tracev, tsv->number);
> > + /* Trace state variables are always 64-bit integers. */
> > + value->kind = axs_rvalue;
> > + value->type = builtin_type (ax->gdbarch)->builtin_long_long;
> > + }
> > + else if (! compile_internalvar_to_ax (var, ax, value))
>
>
> No space after '!'.
>
> > + error (_("$%s is not a trace state variable; GDB agent "
> > + "expressions cannot use convenience variables."), name);
> > +}
> > +
> > void
> > internalvar_operation::do_generate_ax (struct expression *exp,
> > struct agent_expr *ax,
> > @@ -1911,6 +1936,24 @@ op_this_operation::do_generate_ax (struct expression *exp,
> > sym->print_name ());
> > }
> >
> > +static const char *
>
>
> Missing comment describing function.
>
> > +internalvar_op_name (operation *op)
> > +{
> > + gdb_assert (op->opcode () == OP_INTERNALVAR);
> > +
> > + internalvar_operation *ivar_op
> > + = dynamic_cast<internalvar_operation *> (op);
> > + if (ivar_op != nullptr)
> > + return internalvar_name (ivar_op->get_internalvar ());
> > +
> > + uninit_internalvar_operation *uninit_ivar_op
> > + = dynamic_cast<uninit_internalvar_operation *> (op);
> > + if (uninit_ivar_op != nullptr)
> > + return uninit_ivar_op->get_name ();
> > +
> > + return nullptr;
> > +}
> > +
> > void
> > assign_operation::do_generate_ax (struct expression *exp,
> > struct agent_expr *ax,
> > @@ -1921,10 +1964,7 @@ assign_operation::do_generate_ax (struct expression *exp,
> > if (subop->opcode () != OP_INTERNALVAR)
> > error (_("May only assign to trace state variables"));
> >
> > - internalvar_operation *ivarop
> > - = gdb::checked_static_cast<internalvar_operation *> (subop);
> > -
> > - const char *name = internalvar_name (ivarop->get_internalvar ());
> > + const char *name = internalvar_op_name (subop);
> > struct trace_state_variable *tsv;
> >
> > std::get<1> (m_storage)->generate_ax (exp, ax, value);
> > @@ -1950,10 +1990,7 @@ assign_modify_operation::do_generate_ax (struct expression *exp,
> > if (subop->opcode () != OP_INTERNALVAR)
> > error (_("May only assign to trace state variables"));
> >
> > - internalvar_operation *ivarop
> > - = gdb::checked_static_cast<internalvar_operation *> (subop);
> > -
> > - const char *name = internalvar_name (ivarop->get_internalvar ());
> > + const char *name = internalvar_op_name (subop);
> > struct trace_state_variable *tsv;
> >
> > tsv = find_trace_state_variable (name);
> > diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
> > index 45b30842e..7dedb3829 100644
> > --- a/gdb/cli/cli-utils.c
> > +++ b/gdb/cli/cli-utils.c
> > @@ -49,7 +49,7 @@ get_ulongest (const char **pp, int trailer)
> > while (isalnum (*p) || *p == '')
> > p++;
> > std::string varname (start, p - start);
> > - if (!get_internalvar_integer (lookup_internalvar (varname.c_str ()),
> > + if (!get_internalvar_integer (lookup_only_internalvar (varname.c_str ()),
> > &retval))
> > error (("Convenience variable $%s does not have integer value."),
> > varname.c_str ());
> > diff --git a/gdb/expop.h b/gdb/expop.h
> > index 2d46a9dad..921caf4ce 100644
> > --- a/gdb/expop.h
> > +++ b/gdb/expop.h
> > @@ -878,6 +878,42 @@ class bool_operation
> > { return true; }
> > };
> >
> > +/* Reference to a variable that had not been defined at parse time. */
> > +class uninit_internalvar_operation
> > + : public tuple_holding_operationstd::string
> > +{
> > +public:
> > +
> > + using tuple_holding_operation::tuple_holding_operation;
> > +
> > + value *evaluate (struct type *expect_type,
> > + struct expression exp,
> > + enum noside noside) override
> > + {
> > + internalvar iv = lookup_only_internalvar(std::get<0> (m_storage).c_str ());
> > + if (!iv)
> > + return value::allocate (builtin_type (exp->gdbarch)->builtin_void);
>
>
> We prefer "internalvar *iv". Explicit comparison w/nullptr.
>
> > +
> > + return value_of_internalvar (exp->gdbarch, iv);
> > + }
> > +
> > + const char *get_name () const
> > + {
> > + return std::get<0> (m_storage).c_str ();
> > + }
> > +
> > + enum exp_opcode opcode () const override
> > + { return OP_INTERNALVAR; }
> > +
> > +protected:
> > +
> > + void do_generate_ax (struct expression *exp,
> > + struct agent_expr *ax,
> > + struct axs_value *value,
> > + struct type *cast_type)
> > + override;
> > +};
> > +
> > class internalvar_operation
> > : public tuple_holding_operation<internalvar *>
> > {
> > @@ -1891,7 +1927,16 @@ class assign_operation
> > struct expression *exp,
> > enum noside noside) override
> > {
> > - value *lhs = std::get<0> (m_storage)->evaluate (nullptr, exp, noside);
> > + operation lhs_op = std::get<0> (m_storage).get();
> > +
> > + / If the operation is an internalvar that was not initialized at
> > + parse time, ensure that it exists. */
> > + auto *uninit_var_op = dynamic_cast<uninit_internalvar_operation *> (lhs_op);
> > + if (uninit_var_op)
>
>
> Compare w/nullptr.
>
> > + lookup_internalvar (uninit_var_op->get_name());
>
>
> Space after function name.
>
> > +
> > + value lhs = lhs_op->evaluate (nullptr, exp, noside);
> > +
> > / Special-case assignments where the left-hand-side is a
> > convenience variable -- in these, don't bother setting an
> > expected type. This avoids a weird case where re-assigning a
> > @@ -1940,6 +1985,14 @@ class assign_modify_operation
> > struct expression *exp,
> > enum noside noside) override
> > {
> > + operation *lhs_op = std::get<1> (m_storage).get();
>
>
> Space after function name.
>
> > +
> > + /* If the operation is an internalvar that was not initialized at
> > + parse time, ensure that it exists. */
> > + auto *uninit_var_op = dynamic_cast<uninit_internalvar_operation *> (lhs_op);
> > + if (uninit_var_op)
> > + lookup_internalvar (uninit_var_op->get_name());
>
>
> Compare w/nullptr. Space after function name.
>
> > +
> > value *lhs = std::get<1> (m_storage)->evaluate (nullptr, exp, noside);
> > value *rhs = std::get<2> (m_storage)->evaluate (expect_type, exp, noside);
> > return eval_binop_assign_modify (expect_type, exp, noside,
> > diff --git a/gdb/parse.c b/gdb/parse.c
> > index e0837de7b..b6327956e 100644
> > --- a/gdb/parse.c
> > +++ b/gdb/parse.c
> > @@ -240,10 +240,9 @@ parser_state::push_dollar (struct stoken str)
> > return;
> > }
> >
> > - /* Any other names are assumed to be debugger internal variables. /
> > -
> > - push_newexpr::internalvar_operation
> > - (create_internalvar (copy.c_str () + 1));
> > + / Any other names are assumed to be debugger internal variables which
> > + have not yet been initialized. */
> > + push_newexpr::uninit_internalvar_operation (copy.c_str () + 1);
> > }
> >
> > /* See parser-defs.h. */
>
>
> Thank you for your work on this. I really appreciate it.
>
> Keith
  

On 9/1/24 6:23 PM, nt8r@protonmail.com wrote:
> I'll make all these changes.
>
> Is there any automated formatter that can help comply with the style rules? I couldn't manage to get astyle, indent, or clang-format to generate output that followed the specified indentation/whitespace rules.

Unfortunately, we could never find a formatter that worked well enough 
with our rules... we have to do everything manually at this point =/

There was talk of changing things and picking a formatter, but that 
never went forward...

>
> Thanks,
> Antonio
>
> On Friday, August 30th, 2024 at 7:27 PM, Keith Seitz <keiths@redhat.com> wrote:
>
>> Hi,
>>
>> Thank you for resubmitting this patch and (especially) including
>> new tests. This is going to be so much easier to review.
>>
>> On 8/27/24 6:49 PM, Antonio Rische wrote:
>>
>>> We do this by introducing a new type of expression for internalvars
>>> that had not been given a value at parse time.
>>>
>>> This is not particularly useful on its own, but allows us to parse
>>> expressions without having side-effects on the set of internalvars.
>>
>> This is a nice bonus side-effect of this series. I've always found
>> it annoying that convenience variables are created by default like this.
>> This is, however, a slight change in established behavior which I
>> think should be explicitly called out in your commit message. A
>> NEWS entry is justified for this new feature. Users should definitely
>> be aware of this!
>>
>> A trivial update of "Command Completion" mentioning convenience
>> variables/functions in the user manual would be a welcome addition.
>>
>> In general, this looks very good, but there are a handful of GDB-
>> specific style issues that I'd like to point out. I encourage you
>> to peruse the Contribution Checklist:
>>
>> https://sourceware.org/gdb/wiki/ContributionChecklist
>>
>> That has a link to our (sometimes quirky) coding standards and other
>> useful information.
>>
>>> ---
>>> gdb/ada-exp.h | 12 ++++++++--
>>> gdb/ax-gdb.c | 53 ++++++++++++++++++++++++++++++++++++-------
>>> gdb/cli/cli-utils.c | 2 +-
>>> gdb/expop.h | 55 ++++++++++++++++++++++++++++++++++++++++++++-
>>> gdb/parse.c | 7 +++---
>>> 5 files changed, 113 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/gdb/ada-exp.h b/gdb/ada-exp.h
>>> index 94e4ea0f4..3d267b61c 100644
>>> --- a/gdb/ada-exp.h
>>> +++ b/gdb/ada-exp.h
>>> @@ -534,8 +534,16 @@ class ada_assign_operation
>>> assignment. */
>>> value *eval_for_resolution (struct expression *exp)
>>> {
>>> - return std::get<0> (m_storage)->evaluate (nullptr, exp,
>>> - EVAL_AVOID_SIDE_EFFECTS);
>>> + operation *lhs_op = std::get<0> (m_storage).get();
>>
>> Space after function name: "get ()".
>>
>>> +
>>> + /* If the operation is an internalvar that was not initialized at
>>> + parse time, ensure that it exists. */
>>> + auto *uninit_var_op = dynamic_cast<uninit_internalvar_operation *>
>>> + (lhs_op);
>>> + if (uninit_var_op)
>>
>> Since uninit_var_op is a pointer, we stylistically use explicit
>> comparison with nullptr.
>>
>>> + lookup_internalvar (uninit_var_op->get_name());
>>
>> Space after function name again.
>>
>>> +
>>> + return lhs_op->evaluate (nullptr, exp, EVAL_AVOID_SIDE_EFFECTS);
>>> }
>>>
>>> /* The parser must construct the assignment node before parsing the
>>> diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
>>> index 3d619012b..c70aef023 100644
>>> --- a/gdb/ax-gdb.c
>>> +++ b/gdb/ax-gdb.c
>>> @@ -1668,6 +1668,31 @@ register_operation::do_generate_ax (struct expression *exp,
>>> value->type = register_type (ax->gdbarch, reg);
>>> }
>>>
>>> +void
>>> +uninit_internalvar_operation::do_generate_ax (struct expression *exp,
>>
>> All functions should have a comment, even if that is simply "See foo.h."
>>
>>> + struct agent_expr *ax,
>>> + struct axs_value *value,
>>> + struct type *cast_type)
>>> +{
>>> + const char *name = get_name ();
>>> + struct internalvar *var = lookup_only_internalvar(name);
>>
>> Space after function name.
>>
>>> + struct trace_state_variable *tsv;
>>> +
>>> + tsv = find_trace_state_variable (name);
>>> + if (tsv)
>>
>> Use explicit comparison with nullptr.
>>
>>> + {
>>> + ax_tsv (ax, aop_getv, tsv->number);
>>> + if (ax->tracing)
>>
>> Likewise.
>>
>>> + ax_tsv (ax, aop_tracev, tsv->number);
>>> + /* Trace state variables are always 64-bit integers. */
>>> + value->kind = axs_rvalue;
>>> + value->type = builtin_type (ax->gdbarch)->builtin_long_long;
>>> + }
>>> + else if (! compile_internalvar_to_ax (var, ax, value))
>>
>> No space after '!'.
>>
>>> + error (_("$%s is not a trace state variable; GDB agent "
>>> + "expressions cannot use convenience variables."), name);
>>> +}
>>> +
>>> void
>>> internalvar_operation::do_generate_ax (struct expression *exp,
>>> struct agent_expr *ax,
>>> @@ -1911,6 +1936,24 @@ op_this_operation::do_generate_ax (struct expression *exp,
>>> sym->print_name ());
>>> }
>>>
>>> +static const char *
>>
>> Missing comment describing function.
>>
>>> +internalvar_op_name (operation *op)
>>> +{
>>> + gdb_assert (op->opcode () == OP_INTERNALVAR);
>>> +
>>> + internalvar_operation *ivar_op
>>> + = dynamic_cast<internalvar_operation *> (op);
>>> + if (ivar_op != nullptr)
>>> + return internalvar_name (ivar_op->get_internalvar ());
>>> +
>>> + uninit_internalvar_operation *uninit_ivar_op
>>> + = dynamic_cast<uninit_internalvar_operation *> (op);
>>> + if (uninit_ivar_op != nullptr)
>>> + return uninit_ivar_op->get_name ();
>>> +
>>> + return nullptr;
>>> +}
>>> +
>>> void
>>> assign_operation::do_generate_ax (struct expression *exp,
>>> struct agent_expr *ax,
>>> @@ -1921,10 +1964,7 @@ assign_operation::do_generate_ax (struct expression *exp,
>>> if (subop->opcode () != OP_INTERNALVAR)
>>> error (_("May only assign to trace state variables"));
>>>
>>> - internalvar_operation *ivarop
>>> - = gdb::checked_static_cast<internalvar_operation *> (subop);
>>> -
>>> - const char *name = internalvar_name (ivarop->get_internalvar ());
>>> + const char *name = internalvar_op_name (subop);
>>> struct trace_state_variable *tsv;
>>>
>>> std::get<1> (m_storage)->generate_ax (exp, ax, value);
>>> @@ -1950,10 +1990,7 @@ assign_modify_operation::do_generate_ax (struct expression *exp,
>>> if (subop->opcode () != OP_INTERNALVAR)
>>> error (_("May only assign to trace state variables"));
>>>
>>> - internalvar_operation *ivarop
>>> - = gdb::checked_static_cast<internalvar_operation *> (subop);
>>> -
>>> - const char *name = internalvar_name (ivarop->get_internalvar ());
>>> + const char *name = internalvar_op_name (subop);
>>> struct trace_state_variable *tsv;
>>>
>>> tsv = find_trace_state_variable (name);
>>> diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
>>> index 45b30842e..7dedb3829 100644
>>> --- a/gdb/cli/cli-utils.c
>>> +++ b/gdb/cli/cli-utils.c
>>> @@ -49,7 +49,7 @@ get_ulongest (const char **pp, int trailer)
>>> while (isalnum (*p) || *p == '')
>>> p++;
>>> std::string varname (start, p - start);
>>> - if (!get_internalvar_integer (lookup_internalvar (varname.c_str ()),
>>> + if (!get_internalvar_integer (lookup_only_internalvar (varname.c_str ()),
>>> &retval))
>>> error (("Convenience variable $%s does not have integer value."),
>>> varname.c_str ());
>>> diff --git a/gdb/expop.h b/gdb/expop.h
>>> index 2d46a9dad..921caf4ce 100644
>>> --- a/gdb/expop.h
>>> +++ b/gdb/expop.h
>>> @@ -878,6 +878,42 @@ class bool_operation
>>> { return true; }
>>> };
>>>
>>> +/* Reference to a variable that had not been defined at parse time. */
>>> +class uninit_internalvar_operation
>>> + : public tuple_holding_operationstd::string
>>> +{
>>> +public:
>>> +
>>> + using tuple_holding_operation::tuple_holding_operation;
>>> +
>>> + value *evaluate (struct type *expect_type,
>>> + struct expression exp,
>>> + enum noside noside) override
>>> + {
>>> + internalvar iv = lookup_only_internalvar(std::get<0> (m_storage).c_str ());
>>> + if (!iv)
>>> + return value::allocate (builtin_type (exp->gdbarch)->builtin_void);
>>
>> We prefer "internalvar *iv". Explicit comparison w/nullptr.
>>
>>> +
>>> + return value_of_internalvar (exp->gdbarch, iv);
>>> + }
>>> +
>>> + const char *get_name () const
>>> + {
>>> + return std::get<0> (m_storage).c_str ();
>>> + }
>>> +
>>> + enum exp_opcode opcode () const override
>>> + { return OP_INTERNALVAR; }
>>> +
>>> +protected:
>>> +
>>> + void do_generate_ax (struct expression *exp,
>>> + struct agent_expr *ax,
>>> + struct axs_value *value,
>>> + struct type *cast_type)
>>> + override;
>>> +};
>>> +
>>> class internalvar_operation
>>> : public tuple_holding_operation<internalvar *>
>>> {
>>> @@ -1891,7 +1927,16 @@ class assign_operation
>>> struct expression *exp,
>>> enum noside noside) override
>>> {
>>> - value *lhs = std::get<0> (m_storage)->evaluate (nullptr, exp, noside);
>>> + operation lhs_op = std::get<0> (m_storage).get();
>>> +
>>> + / If the operation is an internalvar that was not initialized at
>>> + parse time, ensure that it exists. */
>>> + auto *uninit_var_op = dynamic_cast<uninit_internalvar_operation *> (lhs_op);
>>> + if (uninit_var_op)
>>
>> Compare w/nullptr.
>>
>>> + lookup_internalvar (uninit_var_op->get_name());
>>
>> Space after function name.
>>
>>> +
>>> + value lhs = lhs_op->evaluate (nullptr, exp, noside);
>>> +
>>> / Special-case assignments where the left-hand-side is a
>>> convenience variable -- in these, don't bother setting an
>>> expected type. This avoids a weird case where re-assigning a
>>> @@ -1940,6 +1985,14 @@ class assign_modify_operation
>>> struct expression *exp,
>>> enum noside noside) override
>>> {
>>> + operation *lhs_op = std::get<1> (m_storage).get();
>>
>> Space after function name.
>>
>>> +
>>> + /* If the operation is an internalvar that was not initialized at
>>> + parse time, ensure that it exists. */
>>> + auto *uninit_var_op = dynamic_cast<uninit_internalvar_operation *> (lhs_op);
>>> + if (uninit_var_op)
>>> + lookup_internalvar (uninit_var_op->get_name());
>>
>> Compare w/nullptr. Space after function name.
>>
>>> +
>>> value *lhs = std::get<1> (m_storage)->evaluate (nullptr, exp, noside);
>>> value *rhs = std::get<2> (m_storage)->evaluate (expect_type, exp, noside);
>>> return eval_binop_assign_modify (expect_type, exp, noside,
>>> diff --git a/gdb/parse.c b/gdb/parse.c
>>> index e0837de7b..b6327956e 100644
>>> --- a/gdb/parse.c
>>> +++ b/gdb/parse.c
>>> @@ -240,10 +240,9 @@ parser_state::push_dollar (struct stoken str)
>>> return;
>>> }
>>>
>>> - /* Any other names are assumed to be debugger internal variables. /
>>> -
>>> - push_newexpr::internalvar_operation
>>> - (create_internalvar (copy.c_str () + 1));
>>> + / Any other names are assumed to be debugger internal variables which
>>> + have not yet been initialized. */
>>> + push_newexpr::uninit_internalvar_operation (copy.c_str () + 1);
>>> }
>>>
>>> /* See parser-defs.h. */
>>
>> Thank you for your work on this. I really appreciate it.
>>
>> Keith