[PATCH] Introduce expression::first_opcode

[PATCH] Introduce expression::first_opcode

[Tom Tromey]

This adds a new helper method, expression::first_opcode, that extracts
the outermost opcode of an expression.  This simplifies some patches
in the expression rewrite series.

Note that this patch requires the earlier patch to avoid manual
dissection of OP_TYPE operations.

gdb/ChangeLog
2020-12-13  Tom Tromey  <tom@tromey.com>

	* varobj.c (varobj_create): Use first_opcode.
	* value.c (init_if_undefined_command): Use first_opcode.
	* typeprint.c (whatis_exp): Use first_opcode.
	* tracepoint.c (validate_actionline): Use first_opcode.
	(encode_actions_1): Use first_opcode.
	* stack.c (return_command): Use first_opcode.
	* expression.h (struct expression) <first_opcode>: New method.
	* eval.c (parse_and_eval_type): Use first_opcode.
	* dtrace-probe.c (dtrace_process_dof_probe): Use first_opcode.
---
 gdb/ChangeLog      | 12 ++++++++++++
 gdb/dtrace-probe.c |  2 +-
 gdb/eval.c         |  2 +-
 gdb/expression.h   |  7 +++++++
 gdb/stack.c        |  4 ++--
 gdb/tracepoint.c   |  4 ++--
 gdb/typeprint.c    |  2 +-
 gdb/value.c        |  2 +-
 gdb/varobj.c       |  7 ++++---
 9 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
index 3ea047fce80..b709390039f 100644
--- a/gdb/dtrace-probe.c
+++ b/gdb/dtrace-probe.c
@@ -492,7 +492,7 @@ dtrace_process_dof_probe (struct objfile *objfile,
 	    {
 	    }
 
-	  if (expr != NULL && expr.get ()->elts[0].opcode == OP_TYPE)
+	  if (expr != NULL && expr->first_opcode () == OP_TYPE)
 	    type = value_type (evaluate_type (expr.get ()));
 
 	  args.emplace_back (type, std::move (type_str), std::move (expr));
diff --git a/gdb/eval.c b/gdb/eval.c
index 3f587694776..5bc3c32deb9 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -3106,7 +3106,7 @@ parse_and_eval_type (const char *p, int length)
   tmp[length + 2] = '0';
   tmp[length + 3] = '\0';
   expression_up expr = parse_expression (tmp);
-  if (expr->elts[0].opcode != UNOP_CAST)
+  if (expr->first_opcode () != UNOP_CAST)
     error (_("Internal error in eval_type."));
   return expr->elts[1].type;
 }
diff --git a/gdb/expression.h b/gdb/expression.h
index 9ac940fa4d0..591ae28a0ed 100644
--- a/gdb/expression.h
+++ b/gdb/expression.h
@@ -91,6 +91,13 @@ struct expression
 
   void resize (size_t);
 
+  /* Return the opcode for the outermost sub-expression of this
+     expression.  */
+  enum exp_opcode first_opcode () const
+  {
+      return elts[0].opcode;
+  }
+
   /* Language it was entered in.  */
   const struct language_defn *language_defn;
   /* Architecture it was parsed in.  */
diff --git a/gdb/stack.c b/gdb/stack.c
index 7618f72319a..943b3db0875 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -2770,8 +2770,8 @@ return_command (const char *retval_exp, int from_tty)
 	return_type = TYPE_TARGET_TYPE (SYMBOL_TYPE (thisfun));
       if (return_type == NULL)
       	{
-	  if (retval_expr->elts[0].opcode != UNOP_CAST
-	      && retval_expr->elts[0].opcode != UNOP_CAST_TYPE)
+	  if (retval_expr->first_opcode () != UNOP_CAST
+	      && retval_expr->first_opcode () != UNOP_CAST_TYPE)
 	    error (_("Return value type not available for selected "
 		     "stack frame.\n"
 		     "Please use an explicit cast of the value to return."));
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index c2f923b8fa2..cb2b3b65c33 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -687,7 +687,7 @@ validate_actionline (const char *line, struct breakpoint *b)
 	      expression_up exp = parse_exp_1 (&p, loc->address,
 					       block_for_pc (loc->address), 1);
 
-	      if (exp->elts[0].opcode == OP_VAR_VALUE)
+	      if (exp->first_opcode () == OP_VAR_VALUE)
 		{
 		  if (SYMBOL_CLASS (exp->elts[2].symbol) == LOC_CONST)
 		    {
@@ -1383,7 +1383,7 @@ encode_actions_1 (struct command_line *action,
 						   block_for_pc (tloc->address),
 						   1);
 
-		  switch (exp->elts[0].opcode)
+		  switch (exp->first_opcode ())
 		    {
 		    case OP_REGISTER:
 		      {
diff --git a/gdb/typeprint.c b/gdb/typeprint.c
index 2f671d9c80f..49877710fa4 100644
--- a/gdb/typeprint.c
+++ b/gdb/typeprint.c
@@ -493,7 +493,7 @@ whatis_exp (const char *exp, int show)
       val = evaluate_type (expr.get ());
       type = value_type (val);
 
-      if (show == -1 && expr->elts[0].opcode == OP_TYPE)
+      if (show == -1 && expr->first_opcode () == OP_TYPE)
 	{
 	  /* The user expression names a type directly.  */
 
diff --git a/gdb/value.c b/gdb/value.c
index eba5bce6d2e..9584fc13c27 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1981,7 +1981,7 @@ init_if_undefined_command (const char* args, int from_tty)
   /* Validate the expression.
      Was the expression an assignment?
      Or even an expression at all?  */
-  if (expr->nelts == 0 || expr->elts[0].opcode != BINOP_ASSIGN)
+  if (expr->nelts == 0 || expr->first_opcode () != BINOP_ASSIGN)
     error (_("Init-if-undefined requires an assignment expression."));
 
   /* Extract the variable from the parsed expression.
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 19a90c71196..6a95e1ec0c2 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -311,9 +311,10 @@ varobj_create (const char *objname,
 	}
 
       /* Don't allow variables to be created for types.  */
-      if (var->root->exp->elts[0].opcode == OP_TYPE
-	  || var->root->exp->elts[0].opcode == OP_TYPEOF
-	  || var->root->exp->elts[0].opcode == OP_DECLTYPE)
+      enum exp_opcode opcode = var->root->exp->first_opcode ();
+      if (opcode == OP_TYPE
+	  || opcode == OP_TYPEOF
+	  || opcode == OP_DECLTYPE)
 	{
 	  fprintf_unfiltered (gdb_stderr, "Attempt to use a type name"
 			      " as an expression.\n");
-- 
2.17.2

Re: [PATCH] Introduce expression::first_opcode

[Simon Marchi]

On 2020-12-13 10:45 p.m., Tom Tromey wrote:
> This adds a new helper method, expression::first_opcode, that extracts
> the outermost opcode of an expression.  This simplifies some patches
> in the expression rewrite series.
>
> Note that this patch requires the earlier patch to avoid manual
> dissection of OP_TYPE operations.

It's not immediately clear how this will help (why is the first opcode
special?).  But this looks good to me regardless, if you say it helps I
trust you :).

Simon

Re: [PATCH] Introduce expression::first_opcode

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> On 2020-12-13 10:45 p.m., Tom Tromey wrote:
>> This adds a new helper method, expression::first_opcode, that extracts
>> the outermost opcode of an expression.  This simplifies some patches
>> in the expression rewrite series.
>> 
>> Note that this patch requires the earlier patch to avoid manual
>> dissection of OP_TYPE operations.

Simon> It's not immediately clear how this will help (why is the first
Simon> opcode special?).

Yeah, sorry about that.

There's a reasonable number of places where code examines the opcode of
a subexpression.  Some of these spots are simply removed in the new
approach.  Others are handled by the new implementation of each
individual opcode.

The top-most case is different because, during the switchover, code
referring to it must decide to take the new route or the old route.
It's simpler to make this decision at a single point; and having the old
and new code co-exist for some of the series means that many patches can
remain separate for easier review.

There will be a few more preparatory patches along these lines to handle
the places where code tries to examine the operands of the top-most
opcode.  For examine, consider init_if_undefined_command:

  if (expr->nelts == 0 || expr->elts[0].opcode != BINOP_ASSIGN)
    error (_("Init-if-undefined requires an assignment expression."));

  /* Extract the variable from the parsed expression.
     In the case of an assign the lvalue will be in elts[1] and elts[2].  */
  if (expr->elts[1].opcode != OP_INTERNALVAR)
    error (_("The first parameter to init-if-undefined "
	     "should be a GDB variable."));
  intvar = expr->elts[2].internalvar;

I'll probably move this into some kind of method on struct expression.

FWIW there's actually somewhat of a latent bug here.  A language can
override any opcode with its own implementation -- and change the layout
of the operands.  If this is done for BINOP_ASSIGN, this code will
break.  (The new approach won't have this problem; if it fails it will
fail safely.)  No language currently does this, so nothing is wrong
here; but I can't imagine thinking of this problem if such a patch did
arrive.

Tom