Re: RFC: add pick and roll agent expression operations

[Pedro Alves <pedro at codesourcery dot com>]

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