RFC: add pick and roll agent expression operations

RFC: add pick and roll agent expression operations

[Tom Tromey]

I'd appreciate comments on this.
It at least needs a documentation review.

While looking into agent expressions today, I decided to fix a few bugs
in DWARF -> AX compilation.  This patch adds two new agent expressions,
pick and roll.

I wanted comments since I was not sure if there is some approved method
for rolling out AX changes.  Do we need some kind of version test in
remote.c or in the AX translators?  If not, should we somehow add this?

I haven't run the test suite for this yet.

Tom

2011-02-16  Tom Tromey  <tromey@redhat.com>

	* ax-general.c (aop_map): Add pick and roll.
	* dwarf2loc.c (compile_dwarf_to_ax) <DW_OP_over>: Reimplement.
	<DW_OP_rot>: Implement.
	* ax.h (enum agent_op) <aop_pick, aop_roll>: New constants.
	(ax_pick): Declare.
	* ax-general.c (ax_pick): New function.

2011-02-16  Tom Tromey  <tromey@redhat.com>

	* agentexpr.texi (Bytecode Descriptions): Document pick and roll.

2011-02-16  Tom Tromey  <tromey@redhat.com>

	* tracepoint.c (enum gdb_agent_op) <gdb_agent_op_pick,
	gdb_agent_op_roll>: New constants.
	(gdb_agent_op_names): Add pick and roll.
	(eval_agent_expr) <gdb_agent_op_pick, gdb_agent_op_roll>: New
	cases.

diff --git a/gdb/ax-general.c b/gdb/ax-general.c
index 71d23f1..06ff958 100644
--- a/gdb/ax-general.c
+++ b/gdb/ax-general.c
@@ -143,6 +143,17 @@ ax_simple (struct agent_expr *x, enum agent_op op)
   x->buf[x->len++] = op;
 }
 
+/* Append a pick operator to EXPR.  DEPTH is the stack item to pick,
+   with 0 being top of stack.  */
+void
+ax_pick (struct agent_expr *x, int depth)
+{
+  if (depth < 0 || depth > 255)
+    error (_("GDB bug: ax-general.c (ax_pick): stack depth out of range"));
+  ax_simple (x, aop_pick);
+  append_const (x, 1, depth);
+}
+
 
 /* Append a sign-extension or zero-extension instruction to EXPR, to
    extend an N-bit value.  */
@@ -376,6 +387,8 @@ struct aop_map aop_map[] =
   {"tracev", 2, 0, 0, 1},	/* 0x2e */
   {0, 0, 0, 0, 0},		/* 0x2f */
   {"trace16", 2, 0, 1, 1},	/* 0x30 */
+  {"pick", 1, 0, 0, 1},		/* 0x31 */
+  {"roll", 0, 0, 3, 3},		/* 0x32 */
 };
 
 
diff --git a/gdb/ax.h b/gdb/ax.h
index cd1088d..e34aa07 100644
--- a/gdb/ax.h
+++ b/gdb/ax.h
@@ -204,6 +204,8 @@ enum agent_op
     aop_setv = 0x2d,
     aop_tracev = 0x2e,
     aop_trace16 = 0x30,
+    aop_pick = 0x31,
+    aop_rot = 0x32,
     aop_last
   };
 \f
@@ -221,6 +223,10 @@ extern struct cleanup *make_cleanup_free_agent_expr (struct agent_expr *);
 /* Append a simple operator OP to EXPR.  */
 extern void ax_simple (struct agent_expr *EXPR, enum agent_op OP);
 
+/* Append a pick operator to EXPR.  DEPTH is the stack item to pick,
+   with 0 being top of stack.  */
+extern void ax_pick (struct agent_expr *EXPR, int DEPTH);
+
 /* Append the floating-point prefix, for the next bytecode.  */
 #define ax_float(EXPR) (ax_simple ((EXPR), aop_float))
 
diff --git a/gdb/doc/agentexpr.texi b/gdb/doc/agentexpr.texi
index 7b3fe5a..b33ccaa 100644
--- a/gdb/doc/agentexpr.texi
+++ b/gdb/doc/agentexpr.texi
@@ -391,6 +391,16 @@ Exchange the top two items on the stack.
 @item @code{pop} (0x29): @var{a} =>
 Discard the top value on the stack.
 
+@item @code{pick} (0x31) @var{n}: @var{a} @dots{} @var{b} => @var{a} @dots{} @var{b} @var{a}
+Duplicate an item from the stack and push it on the top of the stack.
+@var{n}, a single byte, indicates the stack item to copy.  If @var{n}
+is zero, this is the same as @code{dup}; if @var{n} is one, it copies
+the item under the top item, etc.  If @var{n} exceeds the number of
+items on the stack, terminate with an error.
+
+@item @code{rot} (0x32): @var{a} @var{b} @var{c} => @var{c} @var{b} @var{a}
+Rotate the top three items on the stack.
+
 @item @code{if_goto} (0x20) @var{offset}: @var{a} @result{}
 Pop an integer off the stack; if it is non-zero, branch to the given
 offset in the bytecode string.  Otherwise, continue to the next
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index f90335d..7ae5513 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1739,7 +1739,7 @@ compile_dwarf_to_ax (struct agent_expr *expr, struct axs_value *loc,
 
 	case DW_OP_pick:
 	  offset = *op_ptr++;
-	  unimplemented (op);
+	  ax_pick (expr, offset);
 	  break;
 	  
 	case DW_OP_swap:
@@ -1747,31 +1747,11 @@ compile_dwarf_to_ax (struct agent_expr *expr, struct axs_value *loc,
 	  break;
 
 	case DW_OP_over:
-	  /* We can't directly support DW_OP_over, but GCC emits it as
-	     part of a sequence to implement signed modulus.  As a
-	     hack, we recognize this sequence.  Note that if GCC ever
-	     generates a branch to the middle of this sequence, then
-	     we will die somehow.  */
-	  if (op_end - op_ptr >= 4
-	      && op_ptr[0] == DW_OP_over
-	      && op_ptr[1] == DW_OP_div
-	      && op_ptr[2] == DW_OP_mul
-	      && op_ptr[3] == DW_OP_minus)
-	    {
-	      /* Sign extend the operands.  */
-	      ax_ext (expr, addr_size_bits);
-	      ax_simple (expr, aop_swap);
-	      ax_ext (expr, addr_size_bits);
-	      ax_simple (expr, aop_swap);
-	      ax_simple (expr, aop_rem_signed);
-	      op_ptr += 4;
-	    }
-	  else
-	    unimplemented (op);
+	  ax_pick (expr, 1);
 	  break;
 
 	case DW_OP_rot:
-	  unimplemented (op);
+	  ax_simple (expr, aop_rot);
 	  break;
 
 	case DW_OP_deref:
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index fb5f522..660cf3c 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -517,6 +517,8 @@ enum gdb_agent_op
     gdb_agent_op_setv = 0x2d,
     gdb_agent_op_tracev = 0x2e,
     gdb_agent_op_trace16 = 0x30,
+    gdb_agent_op_pick = 0x31,
+    gdb_agent_op_rot = 0x32,
     gdb_agent_op_last
   };
 
@@ -571,6 +573,8 @@ static const char *gdb_agent_op_names [gdb_agent_op_last] =
     "tracev",
     "?undef?",
     "trace16",
+    "pick",
+    "rot"
   };
 
 struct agent_expr
@@ -4598,6 +4602,23 @@ eval_agent_expr (struct tracepoint_hit_ctx *ctx,
 	    top = stack[sp];
 	  break;
 
+	case gdb_agent_op_pick:
+	  arg = aexpr->bytes[pc++];
+	  stack[sp] = top;
+	  top = stack[sp - arg];
+	  ++sp;
+	  break;
+
+	case gdb_agent_op_rot:
+	  {
+	    int tem = stack[sp - 1];
+
+	    stack[sp - 1] = stack[sp - 2];
+	    stack[sp - 2] = top;
+	    top = tem;
+	  }
+	  break;
+
 	case gdb_agent_op_zero_ext:
 	  arg = aexpr->bytes[pc++];
 	  if (arg < (sizeof (LONGEST) * 8))

Re: RFC: add pick and roll agent expression operations

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Date: Wed, 16 Feb 2011 14:59:04 -0700
> 
> It at least needs a documentation review.

The documentation part is okay.  Thanks.

Re: RFC: add pick and roll agent expression operations

[Pedro Alves]

On Wednesday 16 February 2011 21:59:04, Tom Tromey wrote:
> I'd appreciate comments on this.
> It at least needs a documentation review.
> 
> While looking into agent expressions today, I decided to fix a few bugs
> in DWARF -> AX compilation.  This patch adds two new agent expressions,
> pick and roll.
> 
> I wanted comments since I was not sure if there is some approved method
> for rolling out AX changes.  Do we need some kind of version test in
> remote.c or in the AX translators?  If not, should we somehow add this?

I think we can get by without it in this case.  Without the
patch you can't even compile the agent expressions.  After
the patch, against an older server/stub, we risk the user
only noticing that the expression didn't work
until late in the trace cycle.  But then again, in
a super controlled environment where that matters, you should
only use a client/server pair that's been verified to work.

Here's an idea for posterity: what if gdbserver made
a quick validating pass over the expressions at tracepoint
download time, and errored out if it found opcodes it didn't
grok?

>  /* Append a sign-extension or zero-extension instruction to EXPR, to
>     extend an N-bit value.  */
> @@ -376,6 +387,8 @@ struct aop_map aop_map[] =
>    {"tracev", 2, 0, 0, 1},	/* 0x2e */
>    {0, 0, 0, 0, 0},		/* 0x2f */
>    {"trace16", 2, 0, 1, 1},	/* 0x30 */
> +  {"pick", 1, 0, 0, 1},		/* 0x31 */
> +  {"roll", 0, 0, 3, 3},		/* 0x32 */

Hmm, you used rot everywhere else?  Looks like
you end up fixing it in the next patch.

> --- a/gdb/ax.h
> +++ b/gdb/ax.h
> @@ -204,6 +204,8 @@ enum agent_op
>      aop_setv = 0x2d,
>      aop_tracev = 0x2e,
>      aop_trace16 = 0x30,
> +    aop_pick = 0x31,
> +    aop_rot = 0x32,
>      aop_last
>    };

>  
>  struct agent_expr
> @@ -4598,6 +4602,23 @@ eval_agent_expr (struct tracepoint_hit_ctx *ctx,

> +	case gdb_agent_op_rot:
> +	  {
> +	    int tem = stack[sp - 1];
> +
> +	    stack[sp - 1] = stack[sp - 2];
> +	    stack[sp - 2] = top;
> +	    top = tem;

tem should be ULONGEST.

Otherwise looks okay to me.  Thanks.

-- 
Pedro Alves

Re: RFC: add pick and roll agent expression operations

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> Here's an idea for posterity: what if gdbserver made
Pedro> a quick validating pass over the expressions at tracepoint
Pedro> download time, and errored out if it found opcodes it didn't
Pedro> grok?

That seems like a good idea.

>> +  {"roll", 0, 0, 3, 3},		/* 0x32 */

Pedro> Hmm, you used rot everywhere else?  Looks like
Pedro> you end up fixing it in the next patch.

Yeah, fixed.  Also, perplexingly, I used "roll" in the ChangeLogs.

Pedro> tem should be ULONGEST.

Fixed.


Also, I did not update the tracepoint JIT.  I did look and notice that
it is already incomplete, and that the one use I know of for DW_OP_over
in GCC's output won't be handled anyway, since it involves division.

Pedro> Otherwise looks okay to me.  Thanks.

Thanks, here is what I am checking in.
I updated the opcode numbers to avoid clashing with the printf patch.

I ran the gdb.trace tests against this locally.

Tom

2011-02-18  Tom Tromey  <tromey@redhat.com>

	* ax-general.c (aop_map): Add pick and rot.
	* dwarf2loc.c (compile_dwarf_to_ax) <DW_OP_over>: Reimplement.
	<DW_OP_rot>: Implement.
	* ax.h (enum agent_op) <aop_pick, aop_rot>: New constants.
	(ax_pick): Declare.
	* ax-general.c (ax_pick): New function.

2011-02-18  Tom Tromey  <tromey@redhat.com>

	* agentexpr.texi (Bytecode Descriptions): Document pick and rot.

2011-02-18  Tom Tromey  <tromey@redhat.com>

	* tracepoint.c (enum gdb_agent_op) <gdb_agent_op_pick,
	gdb_agent_op_rot>: New constants.
	(gdb_agent_op_names): Add pick and roll.
	(eval_agent_expr) <gdb_agent_op_pick, gdb_agent_op_rot>: New
	cases.

Index: ax-general.c
===================================================================
RCS file: /cvs/src/src/gdb/ax-general.c,v
retrieving revision 1.23
diff -u -r1.23 ax-general.c
--- ax-general.c	5 Jan 2011 22:22:47 -0000	1.23
+++ ax-general.c	18 Feb 2011 20:51:07 -0000
@@ -143,6 +143,17 @@
   x->buf[x->len++] = op;
 }
 
+/* Append a pick operator to EXPR.  DEPTH is the stack item to pick,
+   with 0 being top of stack.  */
+void
+ax_pick (struct agent_expr *x, int depth)
+{
+  if (depth < 0 || depth > 255)
+    error (_("GDB bug: ax-general.c (ax_pick): stack depth out of range"));
+  ax_simple (x, aop_pick);
+  append_const (x, 1, depth);
+}
+
 
 /* Append a sign-extension or zero-extension instruction to EXPR, to
    extend an N-bit value.  */
@@ -376,6 +387,9 @@
   {"tracev", 2, 0, 0, 1},	/* 0x2e */
   {0, 0, 0, 0, 0},		/* 0x2f */
   {"trace16", 2, 0, 1, 1},	/* 0x30 */
+  {0, 0, 0, 0, 0},		/* 0x31 */
+  {"pick", 1, 0, 0, 1},		/* 0x32 */
+  {"rot", 0, 0, 3, 3},		/* 0x33 */
 };
 
 
Index: ax.h
===================================================================
RCS file: /cvs/src/src/gdb/ax.h,v
retrieving revision 1.15
diff -u -r1.15 ax.h
--- ax.h	16 Feb 2011 21:02:29 -0000	1.15
+++ ax.h	18 Feb 2011 20:51:07 -0000
@@ -204,6 +204,8 @@
     aop_setv = 0x2d,
     aop_tracev = 0x2e,
     aop_trace16 = 0x30,
+    aop_pick = 0x32,
+    aop_rot = 0x33,
     aop_last
   };
 \f
@@ -221,6 +223,10 @@
 /* Append a simple operator OP to EXPR.  */
 extern void ax_simple (struct agent_expr *EXPR, enum agent_op OP);
 
+/* Append a pick operator to EXPR.  DEPTH is the stack item to pick,
+   with 0 being top of stack.  */
+extern void ax_pick (struct agent_expr *EXPR, int DEPTH);
+
 /* Append the floating-point prefix, for the next bytecode.  */
 #define ax_float(EXPR) (ax_simple ((EXPR), aop_float))
 
Index: dwarf2loc.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2loc.c,v
retrieving revision 1.107
diff -u -r1.107 dwarf2loc.c
--- dwarf2loc.c	17 Feb 2011 16:20:44 -0000	1.107
+++ dwarf2loc.c	18 Feb 2011 20:51:07 -0000
@@ -1740,7 +1740,7 @@
 
 	case DW_OP_pick:
 	  offset = *op_ptr++;
-	  unimplemented (op);
+	  ax_pick (expr, offset);
 	  break;
 	  
 	case DW_OP_swap:
@@ -1748,31 +1748,11 @@
 	  break;
 
 	case DW_OP_over:
-	  /* We can't directly support DW_OP_over, but GCC emits it as
-	     part of a sequence to implement signed modulus.  As a
-	     hack, we recognize this sequence.  Note that if GCC ever
-	     generates a branch to the middle of this sequence, then
-	     we will die somehow.  */
-	  if (op_end - op_ptr >= 4
-	      && op_ptr[0] == DW_OP_over
-	      && op_ptr[1] == DW_OP_div
-	      && op_ptr[2] == DW_OP_mul
-	      && op_ptr[3] == DW_OP_minus)
-	    {
-	      /* Sign extend the operands.  */
-	      ax_ext (expr, addr_size_bits);
-	      ax_simple (expr, aop_swap);
-	      ax_ext (expr, addr_size_bits);
-	      ax_simple (expr, aop_swap);
-	      ax_simple (expr, aop_rem_signed);
-	      op_ptr += 4;
-	    }
-	  else
-	    unimplemented (op);
+	  ax_pick (expr, 1);
 	  break;
 
 	case DW_OP_rot:
-	  unimplemented (op);
+	  ax_simple (expr, aop_rot);
 	  break;
 
 	case DW_OP_deref:
Index: doc/agentexpr.texi
===================================================================
RCS file: /cvs/src/src/gdb/doc/agentexpr.texi,v
retrieving revision 1.12
diff -u -r1.12 agentexpr.texi
--- doc/agentexpr.texi	5 Jan 2011 05:09:52 -0000	1.12
+++ doc/agentexpr.texi	18 Feb 2011 20:51:08 -0000
@@ -391,6 +391,16 @@
 @item @code{pop} (0x29): @var{a} =>
 Discard the top value on the stack.
 
+@item @code{pick} (0x32) @var{n}: @var{a} @dots{} @var{b} => @var{a} @dots{} @var{b} @var{a}
+Duplicate an item from the stack and push it on the top of the stack.
+@var{n}, a single byte, indicates the stack item to copy.  If @var{n}
+is zero, this is the same as @code{dup}; if @var{n} is one, it copies
+the item under the top item, etc.  If @var{n} exceeds the number of
+items on the stack, terminate with an error.
+
+@item @code{rot} (0x33): @var{a} @var{b} @var{c} => @var{c} @var{b} @var{a}
+Rotate the top three items on the stack.
+
 @item @code{if_goto} (0x20) @var{offset}: @var{a} @result{}
 Pop an integer off the stack; if it is non-zero, branch to the given
 offset in the bytecode string.  Otherwise, continue to the next
Index: gdbserver/tracepoint.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/tracepoint.c,v
retrieving revision 1.17
diff -u -r1.17 tracepoint.c
--- gdbserver/tracepoint.c	14 Feb 2011 11:13:12 -0000	1.17
+++ gdbserver/tracepoint.c	18 Feb 2011 20:51:13 -0000
@@ -517,6 +517,8 @@
     gdb_agent_op_setv = 0x2d,
     gdb_agent_op_tracev = 0x2e,
     gdb_agent_op_trace16 = 0x30,
+    gdb_agent_op_pick = 0x32,
+    gdb_agent_op_rot = 0x33,
     gdb_agent_op_last
   };
 
@@ -571,6 +573,9 @@
     "tracev",
     "?undef?",
     "trace16",
+    "?undef?",
+    "pick",
+    "rot"
   };
 
 struct agent_expr
@@ -4598,6 +4603,23 @@
 	    top = stack[sp];
 	  break;
 
+	case gdb_agent_op_pick:
+	  arg = aexpr->bytes[pc++];
+	  stack[sp] = top;
+	  top = stack[sp - arg];
+	  ++sp;
+	  break;
+
+	case gdb_agent_op_rot:
+	  {
+	    ULONGEST tem = stack[sp - 1];
+
+	    stack[sp - 1] = stack[sp - 2];
+	    stack[sp - 2] = top;
+	    top = tem;
+	  }
+	  break;
+
 	case gdb_agent_op_zero_ext:
 	  arg = aexpr->bytes[pc++];
 	  if (arg < (sizeof (LONGEST) * 8))

Re: RFC: add pick and roll agent expression operations

[Pedro Alves]

On Friday 18 February 2011 20:55:05, Tom Tromey wrote:
> Also, I did not update the tracepoint JIT.  I did look and notice that
> it is already incomplete, and that the one use I know of for DW_OP_over
> in GCC's output won't be handled anyway, since it involves division.

Yeah, it's not a correctness issue: we'll just fallback to
bytecode intepreting, and things still work, just not super fast.

-- 
Pedro Alves