diff mbox

breakpoints / tracepoints and setting variables

Message ID 14037.1468597161@usendtaylorx2l
State New
Headers show

Commit Message

David Taylor July 15, 2016, 3:39 p.m. UTC
There is a lot of overlap in the functionality of breakpoints and
tracepoints.

For remote targets, the agent expression engine, when supported, greatly
speeds things up.  Once the expressions are downloaded, GDB is not
involved in evaluating the condition and, for tracepoints, is not
involved in the various collections nor in the manipulation of trace
state variables.

Sometimes, for a breakpoint, you want to set one or more variables and
then continue.  A couple of problems here -- there is no agent
expression opcode for 'continue' and there are no agent expression
opcodes for setting variables other than trace state variables.

For the first problem -- no continue -- two solutions present
themselves.  Either use a tracepoint -- they weren't meant for this, but
you use the tools available -- or use the condition, thusly:

    (real condition) && (variable assignment) && 0

The zero makes the condition false and achives the 'continue'.

There's still the problem of the lack of setting memory and registers
within the agent expression byte codes.  This patch addresses that
problem.

gdb/ChangeLog:

	* ax-gdb.c (gen_expr): Add support for assignment to more than
	just trace state variables.
	(gen_expr_binop_rest): Ditto.
	* ax-general.c (ax_setreg): New function.
	* ax.h (ax_setreg): Declare it.
	* common/ax.def (setmem8, setmem16, setmem32, setmem64, setreg):
	New agent bytecodes.

gdb/doc/ChangeLog:

	* agentexpr.texi (Bytecode Descriptions): Add descriptions of
	bytecodes setmem8, setmem16, setmem32, setmem64, and setreg.

gdb/gdbserver/ChangeLog:

	* ax.c (compile_bytecodes): Add cases for new bytecodes (setmem8,
	setmem16, setmem32, setmem64, and setreg).
	* tracepoint.c (write_inferior_memory, agent_mem_write): New
	functions.
	* tracepoint.h (agent_mem_write): Declare.

gdb/testsuite/ChangeLog:

	* gdb.trace/actions.c: (get_int_test): New variable.
	(struct GDB_STRUCT_TEST): New int field.
	(gdb_c_test): Assign values to new variable and struct member.
	* gdb.trace/ax.exp: Add new tests.
---
 gdb/ChangeLog                     |  10 +++
 gdb/ax-gdb.c                      | 128 ++++++++++++++++++++++++++++++++++++--
 gdb/ax-general.c                  |  22 +++++++
 gdb/ax.h                          |   5 ++
 gdb/common/ax.def                 |   6 ++
 gdb/doc/ChangeLog                 |   5 ++
 gdb/doc/agentexpr.texi            |  19 ++++++
 gdb/gdbserver/ChangeLog           |   8 +++
 gdb/gdbserver/ax.c                |  67 ++++++++++++++++++++
 gdb/gdbserver/tracepoint.c        |  15 +++++
 gdb/gdbserver/tracepoint.h        |   4 ++
 gdb/testsuite/ChangeLog           |   7 +++
 gdb/testsuite/gdb.trace/actions.c |   5 +-
 gdb/testsuite/gdb.trace/ax.exp    |  23 +++++++
 14 files changed, 318 insertions(+), 6 deletions(-)

Comments

taylor, david July 15, 2016, 4:55 p.m. UTC | #1
I forgot to add -- built and tested on x86-64 GNU/Linux with no regressions.
Andrew Burgess Aug. 2, 2016, 11:33 a.m. UTC | #2
I'm not a maintainer, so can't approve your patch, however, I made a
few comments inline.

Thanks,
Andrew


* David Taylor <dtaylor@emc.com> [2016-07-15 11:39:21 -0400]:

> There is a lot of overlap in the functionality of breakpoints and
> tracepoints.
> 
> For remote targets, the agent expression engine, when supported, greatly
> speeds things up.  Once the expressions are downloaded, GDB is not
> involved in evaluating the condition and, for tracepoints, is not
> involved in the various collections nor in the manipulation of trace
> state variables.
> 
> Sometimes, for a breakpoint, you want to set one or more variables and
> then continue.  A couple of problems here -- there is no agent
> expression opcode for 'continue' and there are no agent expression
> opcodes for setting variables other than trace state variables.
> 
> For the first problem -- no continue -- two solutions present
> themselves.  Either use a tracepoint -- they weren't meant for this, but
> you use the tools available -- or use the condition, thusly:
> 
>     (real condition) && (variable assignment) && 0
> 
> The zero makes the condition false and achives the 'continue'.
> 
> There's still the problem of the lack of setting memory and registers
> within the agent expression byte codes.  This patch addresses that
> problem.
> 
> gdb/ChangeLog:
> 
> 	* ax-gdb.c (gen_expr): Add support for assignment to more than
> 	just trace state variables.
> 	(gen_expr_binop_rest): Ditto.
> 	* ax-general.c (ax_setreg): New function.
> 	* ax.h (ax_setreg): Declare it.
> 	* common/ax.def (setmem8, setmem16, setmem32, setmem64, setreg):
> 	New agent bytecodes.
> 
> gdb/doc/ChangeLog:
> 
> 	* agentexpr.texi (Bytecode Descriptions): Add descriptions of
> 	bytecodes setmem8, setmem16, setmem32, setmem64, and setreg.
> 
> gdb/gdbserver/ChangeLog:
> 
> 	* ax.c (compile_bytecodes): Add cases for new bytecodes (setmem8,
> 	setmem16, setmem32, setmem64, and setreg).
> 	* tracepoint.c (write_inferior_memory, agent_mem_write): New
> 	functions.
> 	* tracepoint.h (agent_mem_write): Declare.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.trace/actions.c: (get_int_test): New variable.
> 	(struct GDB_STRUCT_TEST): New int field.
> 	(gdb_c_test): Assign values to new variable and struct member.
> 	* gdb.trace/ax.exp: Add new tests.
> ---
>  gdb/ChangeLog                     |  10 +++
>  gdb/ax-gdb.c                      | 128 ++++++++++++++++++++++++++++++++++++--
>  gdb/ax-general.c                  |  22 +++++++
>  gdb/ax.h                          |   5 ++
>  gdb/common/ax.def                 |   6 ++
>  gdb/doc/ChangeLog                 |   5 ++
>  gdb/doc/agentexpr.texi            |  19 ++++++
>  gdb/gdbserver/ChangeLog           |   8 +++
>  gdb/gdbserver/ax.c                |  67 ++++++++++++++++++++
>  gdb/gdbserver/tracepoint.c        |  15 +++++
>  gdb/gdbserver/tracepoint.h        |   4 ++
>  gdb/testsuite/ChangeLog           |   7 +++
>  gdb/testsuite/gdb.trace/actions.c |   5 +-
>  gdb/testsuite/gdb.trace/ax.exp    |  23 +++++++
>  14 files changed, 318 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 4139a29..887551a 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,13 @@
> +2016-07-14  David Taylor  <dtaylor@emc.com>
> +
> +	* ax-gdb.c (gen_expr): Add support for assignment to more than
> +	just trace state variables.
> +	(gen_expr_binop_rest): Ditto.
> +	* ax-general.c (ax_setreg): New function.
> +	* ax.h (ax_setreg): Declare it.
> +	* common/ax.def (setmem8, setmem16, setmem32, setmem64, setreg):
> +	New agent bytecodes.
> +
>  2016-07-12  Tom Tromey  <tom@tromey.com>
>  
>  	PR python/19293:
> diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
> index 7c6cb64..d891ade 100644
> --- a/gdb/ax-gdb.c
> +++ b/gdb/ax-gdb.c
> @@ -1895,6 +1895,7 @@ gen_expr (struct expression *exp, union exp_element **pc,
>  
>  	  (*pc) += 3;
>  	  gen_expr (exp, pc, ax, value);
> +	  require_rvalue (ax, value);
>  	  tsv = find_trace_state_variable (name);
>  	  if (tsv)
>  	    {
> @@ -1903,11 +1904,61 @@ gen_expr (struct expression *exp, union exp_element **pc,
>  		ax_tsv (ax, aop_tracev, tsv->number);
>  	    }
>  	  else
> -	    error (_("$%s is not a trace state variable, "
> -		     "may not assign to it"), name);
> +	    error (_("$%s is convenience variable, not a trace state variable, "
> +		     "You may not assign to it."), name);
> +	}
> +      else if ((*pc)[0].opcode == OP_VAR_VALUE)
> +	{
> +	  int len;
> +	  enum agent_op opcode;
> +	  struct type *type = check_typedef (SYMBOL_TYPE ((*pc)[2].symbol));
> +
> +	  len = TYPE_LENGTH (type);
> +
> +	  if (len == 1)
> +	    opcode = aop_setmem8;
> +	  else if (len == 2)
> +	    opcode = aop_setmem16;
> +	  else if (len == 4)
> +	    opcode = aop_setmem32;
> +	  else if (len == 8)
> +	    opcode = aop_setmem64;
> +	  else
> +	    error (_("Sorry, I don't have an opcode for types of length %d"), len);

I don't know if there's any/much precedent for GDB error message to
refer to GDB in the first person (is that the correct term?).  I think
something like just

    error (_("unsupported OP_VAR_VALUE of length %d"), len);

would be more in keeping with other GDB error messages.

> +
> +	  gen_expr (exp, pc, ax, &value1);
> +
> +	  if (value1.kind != axs_lvalue_register)
> +	    {
> +	      if (ax->tracing)
> +		ax_trace_quick (ax, len); /* record original value */
> +	    }
> +
> +	  gen_expr (exp, pc, ax, value);
> +	  gen_usual_unary (exp, ax, value);
> +	  require_rvalue (ax, value);
> +
> +	  if (value1.kind == axs_lvalue_register)
> +	    {
> +	      ax_reg_mask (ax, value1.u.reg);
> +	      ax_setreg (ax, value1.u.reg);
> +	    }
> +	  else
> +	    ax_simple (ax, opcode);
> +	}
> +      else if ((*pc)[0].opcode == OP_REGISTER)
> +	{
> +	  gen_expr (exp, pc, ax, &value1);
> +
> +	  gen_expr (exp, pc, ax, value);
> +	  gen_usual_unary (exp, ax, value);
> +	  require_rvalue (ax, value);
> +
> +	  ax_reg_mask (ax, value1.u.reg);
> +	  ax_setreg (ax, value1.u.reg);
>  	}
>        else
> -	error (_("May only assign to trace state variables"));
> +	error (_("Sorry, I don't know how to do that (yet?)"));

As with the other error message, however, not that there is
'op_name_standard' which I think will help give a string version of
the opcode, so:

    error (_("opcode %s unsupported", op_name_standard ((*pc)[0].opcode)));




>        break;
>  
>      case BINOP_ASSIGN_MODIFY:
> @@ -1939,8 +1990,74 @@ gen_expr (struct expression *exp, union exp_element **pc,
>  		ax_tsv (ax, aop_tracev, tsv->number);
>  	    }
>  	  else
> -	    error (_("$%s is not a trace state variable, "
> -		     "may not assign to it"), name);
> +	    error (_("Convenience variables such as '$%s' are known only to the debugger and "
> +		     "you may not assign to them in an action."), name);
> +	}
> +      else if ((*pc)[0].opcode == OP_VAR_VALUE)
> +	{
> +	  int len;
> +	  enum agent_op opcode;
> +	  struct type *type;
> +	  struct axs_value saved_value;
> +
> +	  gen_expr (exp, pc, ax, value);
> +	  type = check_typedef (value->type);
> +	  len = TYPE_LENGTH (type);
> +
> +	  if (len == 1)
> +	    opcode = aop_setmem8;
> +	  else if (len == 2)
> +	    opcode = aop_setmem16;
> +	  else if (len == 4)
> +	    opcode = aop_setmem32;
> +	  else if (len == 8)
> +	    opcode = aop_setmem64;
> +	  else
> +	    error (_("Sorry, I don't have an opcode for types of length %d"), len);

reword.

> +
> +	  if (value->kind != axs_lvalue_register)
> +	    {
> +	      ax_simple (ax, aop_dup); /* keep the address around */
> +
> +	      if (ax->tracing)
> +		ax_trace_quick (ax, len); /* record original value */
> +	    }
> +
> +	  value1.kind = value->kind;
> +	  value1.type = value->type;
> +	  value1.u.reg = value->u.reg;
> +
> +	  /* gen_expr_binop_rest will set value, value1, and value2.
> +	     And typically value for value, value1 ends up as
> +	     axs_rvalue.  We need to remember whether it was a
> +	     register and, if so, which register */
> +	  saved_value.kind = value->kind;
> +	  saved_value.u.reg = value->u.reg;
> +
> +	  /* Now do right half of expression.  */
> +	  gen_expr_binop_rest (exp, op2, pc, ax, value, &value1, &value2);
> +
> +	  /* We have the result of the binary op, set the variable */
> +	  if (saved_value.kind == axs_lvalue_register)
> +	    {
> +	      ax_reg_mask (ax, saved_value.u.reg);
> +	      ax_setreg (ax, saved_value.u.reg);
> +	    }
> +	  else
> +	    ax_simple (ax, opcode);
> +	}
> +      else if ((*pc)[0].opcode == OP_REGISTER)
> +	{
> +	  gen_expr (exp, pc, ax, value);
> +
> +	  value1.kind = value->kind;
> +	  value1.type = value->type;
> +	  value1.u.reg = value->u.reg;
> +
> +	  /* Now do right half of expression.  */
> +	  gen_expr_binop_rest (exp, op2, pc, ax, value, &value1, &value2);
> +
> +	  ax_setreg (ax, value->u.reg);
>  	}
>        else
>  	error (_("May only assign to trace state variables"));
> @@ -2246,6 +2363,7 @@ gen_expr_binop_rest (struct expression *exp,
>  {
>    struct type *int_type = builtin_type (exp->gdbarch)->builtin_int;
>  
> +  require_rvalue (ax, value1);
>    gen_expr (exp, pc, ax, value2);
>    gen_usual_unary (exp, ax, value2);
>    gen_usual_arithmetic (exp, ax, value1, value2);
> diff --git a/gdb/ax-general.c b/gdb/ax-general.c
> index 7f27a45..5406ab8 100644
> --- a/gdb/ax-general.c
> +++ b/gdb/ax-general.c
> @@ -323,6 +323,28 @@ ax_reg (struct agent_expr *x, int reg)
>      }
>  }
>  
> +/* Assemble code to pop the top of stack and set register number REG
> +   to its value.  */
> +
> +void
> +ax_setreg (struct agent_expr *x, int reg)
> +{
> +  if (reg >= gdbarch_num_regs (x->gdbarch))
> +    error (_("register number '%d' is a pseudo register or is out of range"), reg);
> +  else
> +    {
> +      /* Make sure the register number is in range.  */
> +      if (reg < 0 || reg > 0xffff)
> +        error (_("GDB bug: ax-general.c (ax_reg): "
> +		 "register number out of range"));

If this indicates a GDB bug then you should switch to either
internal_error or gdb_assert.  If you choose internal_error then you
can drop the 'GDB bug' prefix.

> +      grow_expr (x, 3);
> +      x->buf[x->len] = aop_setreg;
> +      x->buf[x->len + 1] = (reg >> 8) & 0xff;
> +      x->buf[x->len + 2] = (reg) & 0xff;
> +      x->len += 3;
> +    }
> +}
> +
>  /* Assemble code to operate on a trace state variable.  */
>  
>  void
> diff --git a/gdb/ax.h b/gdb/ax.h
> index 536efe8..7b10355 100644
> --- a/gdb/ax.h
> +++ b/gdb/ax.h
> @@ -234,6 +234,11 @@ extern void ax_const_d (struct agent_expr *EXPR, LONGEST d);
>     stack.  */
>  extern void ax_reg (struct agent_expr *EXPR, int REG);
>  
> +/* Assemble code to pop the top of stack and set register number REG
> +   to its value.  */
> +
> +extern void ax_setreg (struct agent_expr *EXPR, int REG);
> +
>  /* Add the given register to the register mask of the expression.  */
>  extern void ax_reg_mask (struct agent_expr *ax, int reg);
>  
> diff --git a/gdb/common/ax.def b/gdb/common/ax.def
> index f375373..86cf4b5 100644
> --- a/gdb/common/ax.def
> +++ b/gdb/common/ax.def
> @@ -95,3 +95,9 @@ DEFOP (pick, 1, 0, 0, 1, 0x32)
>  DEFOP (rot, 0, 0, 3, 3, 0x33)
>  /* Both the argument and consumed numbers are dynamic for this one.  */
>  DEFOP (printf, 0, 0, 0, 0, 0x34)
> +/* The following five operands are not yet implemented.  */
> +DEFOP (setmem8, 0, 8, 2, 1, 0x35)
> +DEFOP (setmem16, 0, 16, 2, 1, 0x36)
> +DEFOP (setmem32, 0, 32, 2, 1, 0x37)
> +DEFOP (setmem64, 0, 64, 2, 1, 0x38)
> +DEFOP (setreg, 2, 0, 1, 1, 0x39)
> diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
> index 385ca41..1ddaf61 100644
> --- a/gdb/doc/ChangeLog
> +++ b/gdb/doc/ChangeLog
> @@ -1,3 +1,8 @@
> +2016-07-14  David Taylor  <dtaylor@emc.com>
> +
> +	* agentexpr.texi (Bytecode Descriptions): Add descriptions of
> +	bytecodes setmem8, setmem16, setmem32, setmem64, and setreg.
> +
>  2016-07-12  Tom Tromey  <tom@tromey.com>
>  
>  	PR python/19293:
> diff --git a/gdb/doc/agentexpr.texi b/gdb/doc/agentexpr.texi
> index ade7638..23e0ccc 100644
> --- a/gdb/doc/agentexpr.texi
> +++ b/gdb/doc/agentexpr.texi
> @@ -515,6 +515,25 @@ stack.  If the purpose of the expression was to compute an lvalue or a
>  range of memory, then the next-to-top of the stack is the lvalue's
>  address, and the top of the stack is the lvalue's size, in bytes.
>  
> +@item @code{setmem8} (0x35): @var{addr} @var{value} @result{} @var{value}
> +@itemx @code{setmem16} (0x36): @var{addr} @var{value} @result{} @var{value}
> +@itemx @code{setmem32} (0x37): @var{addr} @var{value} @result{} @var{value}
> +@itemx @code{setmem64} (0x38): @var{addr} @var{value} @result{} @var{value}
> +For bytecode @code{setmem}@var{n}, set an @var{n}-bit value at
> +@var{addr}, using the @var{n}-bit least significant bits of @var{value}
> +and natural target endianness.  The address @var{addr} is popped off the
> +stack; the value @var{value} is left on the stack.
> +
> +If attempting to write memory at @var{addr} would cause a processor
> +exception of some sort, terminate with an error.
> +
> +@item @code{setreg} (0x39) @var{regnum}: @var{value} @result{} @var{value}
> +Set register @var{regnum} to @var{value}.  The value @var{value} is left
> +on the stack.
> +
> +If the register is read-only or if attempting to write @var{value} to it
> +would cause a processor exception of some sort, terminate with an error.
> +
>  @end table
>  
>  
> diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
> index cb87e48..e0f3a9b 100644
> --- a/gdb/gdbserver/ChangeLog
> +++ b/gdb/gdbserver/ChangeLog
> @@ -1,3 +1,11 @@
> +2016-07-14  David Taylor  <dtaylor@emc.com>
> +
> +	* ax.c (compile_bytecodes): Add cases for new bytecodes (setmem8,
> +	setmem16, setmem32, setmem64, and setreg).
> +	* tracepoint.c (write_inferior_memory, agent_mem_write): New
> +	functions.
> +	* tracepoint.h (agent_mem_write): Declare.
> +
>  2016-07-12  Chung-Lin Tang  <cltang@codesourcery.com>
>  
>  	* linux-nios2-low.c (nios2_fill_gregset): Add type cast
> diff --git a/gdb/gdbserver/ax.c b/gdb/gdbserver/ax.c
> index d5144c2..bf33d42 100644
> --- a/gdb/gdbserver/ax.c
> +++ b/gdb/gdbserver/ax.c
> @@ -736,6 +736,26 @@ compile_bytecodes (struct agent_expr *aexpr)
>  	  UNHANDLED;
>  	  break;
>  
> +	case gdb_agent_op_setmem8:
> +	  UNHANDLED;
> +	  break;
> +
> +	case gdb_agent_op_setmem16:
> +	  UNHANDLED;
> +	  break;
> +
> +	case gdb_agent_op_setmem32:
> +	  UNHANDLED;
> +	  break;
> +
> +	case gdb_agent_op_setmem64:
> +	  UNHANDLED;
> +	  break;
> +
> +	case gdb_agent_op_setreg:
> +	  UNHANDLED;
> +	  break;
> +
>  	  /* GDB never (currently) generates any of these ops.  */
>  	case gdb_agent_op_float:
>  	case gdb_agent_op_ref_float:
> @@ -1322,6 +1342,53 @@ gdb_eval_agent_expr (struct eval_agent_expr_context *ctx,
>  	  }
>  	  break;
>  
> +	  /* for the setmem{8,16,32,64} opcodes, we leave the value on
> +	     the stack (in top), but the address written to is popped
> +	     from the stack */
> +	case gdb_agent_op_setmem8:
> +	  cnv.u8.val = (unsigned char) top;
> +	  agent_mem_write (ctx, cnv.u8.bytes, stack[sp--], 1);
> +	  break;
> +
> +	case gdb_agent_op_setmem16:
> +	  cnv.u16.val = (unsigned short) top;
> +	  agent_mem_write (ctx, cnv.u16.bytes, stack[sp--], 2);
> +	  break;
> +
> +	case gdb_agent_op_setmem32:
> +	  cnv.u32.val = (unsigned int) top;
> +	  agent_mem_write (ctx, cnv.u32.bytes, stack[sp--], 4);
> +	  break;
> +
> +	case gdb_agent_op_setmem64:
> +	  cnv.u64.val = (ULONGEST) top;
> +	  agent_mem_write (ctx, cnv.u64.bytes, stack[sp--], 8);
> +	  break;
> +
> +	case gdb_agent_op_setreg:
> +	  arg = aexpr->bytes[pc++];
> +	  arg = (arg << 8) + aexpr->bytes[pc++];
> +	  switch (register_size (ctx->regcache->tdesc, arg))
> +	    {
> +	    case 8:
> +	      cnv.u8.val = (unsigned char) top;
> +	      break;
> +	    case 16:
> +	      cnv.u16.val = (unsigned short) top;
> +	      break;
> +	    case 32:
> +	      cnv.u32.val = (unsigned int) top;
> +	      break;
> +	    case 64:
> +	      cnv.u64.val = (ULONGEST) top;
> +	      break;
> +	    default:
> +	      internal_error (__FILE__, __LINE__,
> +			      "unhandled register size");
> +	    }
> +	  supply_register (ctx->regcache, arg, &cnv);
> +	  break;
> +
>  	  /* GDB never (currently) generates any of these ops.  */
>  	case gdb_agent_op_float:
>  	case gdb_agent_op_ref_float:
> diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
> index c07e525..c4ee465 100644
> --- a/gdb/gdbserver/tracepoint.c
> +++ b/gdb/gdbserver/tracepoint.c
> @@ -357,6 +357,13 @@ read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
>    return 0;
>  }
>  
> +int
> +write_inferior_memory (CORE_ADDR memaddr, unsigned char const *myaddr, int len)
> +{
> +  memcpy ((void *) (uintptr_t) memaddr, myaddr, len);
> +  return 0;
> +}
> +
>  /* Call this in the functions where GDBserver places a breakpoint, so
>     that the compiler doesn't try to be clever and skip calling the
>     function at all.  This is necessary, even if we tell the compiler
> @@ -4948,6 +4955,14 @@ condition_true_at_tracepoint (struct tracepoint_hit_ctx *ctx,
>  }
>  
>  /* Do memory copies for bytecodes.  */
> +int
> +agent_mem_write (struct eval_agent_expr_context *ctx,
> +		 unsigned char *from, CORE_ADDR to, ULONGEST len)
> +{
> +  return write_inferior_memory (to, from, len);
> +}
> +
> +/* Do memory copies for bytecodes.  */
>  /* Do the recording of memory blocks for actions and bytecodes.  */
>  
>  int
> diff --git a/gdb/gdbserver/tracepoint.h b/gdb/gdbserver/tracepoint.h
> index 679a32a..0886f8e 100644
> --- a/gdb/gdbserver/tracepoint.h
> +++ b/gdb/gdbserver/tracepoint.h
> @@ -154,6 +154,10 @@ int agent_mem_read (struct eval_agent_expr_context *ctx,
>  		    unsigned char *to, CORE_ADDR from,
>  		    ULONGEST len);
>  
> +int agent_mem_write (struct eval_agent_expr_context *ctx,
> +		     unsigned char *from, CORE_ADDR to,
> +		     ULONGEST len);
> +
>  LONGEST agent_get_trace_state_variable_value (int num);
>  void agent_set_trace_state_variable_value (int num, LONGEST val);
>  
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index 7ab1228..7a33041 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,10 @@
> +2016-07-14  David Taylor  <dtaylor@emc.com>
> +
> +	* gdb.trace/actions.c: (get_int_test): New variable.
> +	(struct GDB_STRUCT_TEST): New int field.
> +	(gdb_c_test): Assign values to new variable and struct member.
> +	* gdb.trace/ax.exp: Add new tests.
> +
>  2016-07-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
>  
>  	* gdb.dwarf2/atomic-type.exp: Use function_range for low_pc and high_pc.
> diff --git a/gdb/testsuite/gdb.trace/actions.c b/gdb/testsuite/gdb.trace/actions.c
> index aedd202..ebc1559 100644
> --- a/gdb/testsuite/gdb.trace/actions.c
> +++ b/gdb/testsuite/gdb.trace/actions.c
> @@ -25,12 +25,14 @@
>  
>  static char   gdb_char_test;
>  static short  gdb_short_test;
> +static int    gdb_int_test;
>  static long   gdb_long_test;
>  static char   gdb_arr_test[25];
>  static struct GDB_STRUCT_TEST
>  {
>    char   c;
>    short  s;
> +  int    i;
>    long   l;
>    int    bfield : 11;	/* collect bitfield */
>    char   arr[25];
> @@ -97,6 +99,7 @@ unsigned long   gdb_c_test( unsigned long *parm )
>  
>     gdb_char_test   = gdb_struct1_test.c = (char)   ((long) parm[1] & 0xff);
>     gdb_short_test  = gdb_struct1_test.s = (short)  ((long) parm[2] & 0xffff);
> +   gdb_int_test    = gdb_struct1_test.i = (int)    ((long) parm[3] & 0xffffffff);
>     gdb_long_test   = gdb_struct1_test.l = (long)   ((long) parm[3] & 0xffffffff);
>     gdb_union1_test.l = (long) parm[4];
>     gdb_arr_test[0] = gdb_struct1_test.arr[0] = (char) ((long) parm[1] & 0xff);
> @@ -113,7 +116,7 @@ unsigned long   gdb_c_test( unsigned long *parm )
>     gdb_recursion_test_ptr (3, (long) parm[1], (long) parm[2], (long) parm[3],
>  		       (long) parm[4], (long) parm[5], (long) parm[6]);
>  
> -   gdb_char_test = gdb_short_test = gdb_long_test = 0;
> +   gdb_char_test = gdb_short_test = gdb_int_test = gdb_long_test = 0;
>     gdb_structp_test  = (void *) 0;
>     gdb_structpp_test = (void *) 0;
>     memset ((char *) &gdb_struct1_test, 0, sizeof (gdb_struct1_test));
> diff --git a/gdb/testsuite/gdb.trace/ax.exp b/gdb/testsuite/gdb.trace/ax.exp
> index 5e9783f..da424f3 100644
> --- a/gdb/testsuite/gdb.trace/ax.exp
> +++ b/gdb/testsuite/gdb.trace/ax.exp
> @@ -82,6 +82,17 @@ gdb_test "maint agent &gdb_long_test < &gdb_short_test" "" "maint agent &gdb_lon
>  
>  gdb_test "maint agent (unsigned char)1L" ".*ext 8.*" "maint agent (unsigned char)1L"
>  
> +gdb_test "maint agent long_long = 12" "" "maint agent long_long = 12"
> +
> +gdb_test "maint agent gdb_char_test = 19" "" "maint agent gdb_char_test = 19"
> +
> +gdb_test "maint agent gdb_short_test = 24" "" "maint agent gdb_short_test = 24"
> +
> +gdb_test "maint agent gdb_int_test = 35" "" "maint agent gdb_int_test = 35"
> +
> +gdb_test "maint agent gdb_long_test = 45" "" "maint agent gdb_long_test = 45"
> +
> +gdb_test "maint agent -at gdb_recursion_test, depth = 100" "" "maint agent -at gdb_recursion_test, depth = 100"
>  # Now test eval version of agent expressions.
>  
>  gdb_test "maint agent-eval 12" ".*const8 12.*end.*" "maint agent-eval 12"
> @@ -130,3 +141,15 @@ gdb_test "maint agent-eval &gdb_long_test == &gdb_short_test" ".*equal.*end.*" "
>  
>  gdb_test "maint agent-eval &gdb_long_test < &gdb_short_test" "" "maint agent-eval &gdb_long_test < &gdb_short_test"
>  
> +
> +gdb_test "maint agent-eval long_long = 12" "" "maint agent long_long = 12"
> +
> +gdb_test "maint agent-eval gdb_char_test = 19" "" "maint agent gdb_char_test = 19"
> +
> +gdb_test "maint agent-eval gdb_short_test = 24" "" "maint agent gdb_short_test = 24"
> +
> +gdb_test "maint agent-eval gdb_int_test = 35" "" "maint agent gdb_int_test = 35"
> +
> +gdb_test "maint agent-eval gdb_long_test = 45" "" "maint agent gdb_long_test = 45"
> +
> +gdb_test "maint agent-eval -at gdb_recursion_test, depth = 100" "" "maint agent -at gdb_recursion_test, depth = 100"
> -- 
> 1.9.1
>
taylor, david Aug. 2, 2016, 8:28 p.m. UTC | #3
> From: Andrew Burgess [mailto:andrew.burgess@embecosm.com]
> Sent: Tuesday, August 02, 2016 7:33 AM

> I'm not a maintainer, so can't approve your patch, however, I made a
> few comments inline.

Thanks.

I am testing an updated patch that I believe addresses your issues
with one exception --- see below.  Assuming it passes with no regressions,
Ill rebase it as one commit and post it.

> Thanks,
> Andrew
> 
> 
> * David Taylor <dtaylor@emc.com> [2016-07-15 11:39:21 -0400]:

> > diff --git a/gdb/ax-general.c b/gdb/ax-general.c
> > index 7f27a45..5406ab8 100644
> > --- a/gdb/ax-general.c
> > +++ b/gdb/ax-general.c
> > @@ -323,6 +323,28 @@ ax_reg (struct agent_expr *x, int reg)
> >      }
> >  }
> >
> > +/* Assemble code to pop the top of stack and set register number REG
> > +   to its value.  */
> > +
> > +void
> > +ax_setreg (struct agent_expr *x, int reg)
> > +{
> > +  if (reg >= gdbarch_num_regs (x->gdbarch))
> > +    error (_("register number '%d' is a pseudo register or is out of range"),
> reg);
> > +  else
> > +    {
> > +      /* Make sure the register number is in range.  */
> > +      if (reg < 0 || reg > 0xffff)
> > +        error (_("GDB bug: ax-general.c (ax_reg): "
> > +		 "register number out of range"));
> 
> If this indicates a GDB bug then you should switch to either
> internal_error or gdb_assert.  If you choose internal_error then you
> can drop the 'GDB bug' prefix.

If I counted correctly, there are 9 other, pre-existing, instances of
error (_("GDB bug: ..." in the file ax-general -- which is why I chose
to do this one that way.  I can use internal_error if people feel that
is better than consistency with rest of the file.
Andrew Burgess Aug. 5, 2016, 7:46 p.m. UTC | #4
* taylor, david <david.taylor@emc.com> [2016-08-02 20:28:58 +0000]:

> 
> > From: Andrew Burgess [mailto:andrew.burgess@embecosm.com]
> > Sent: Tuesday, August 02, 2016 7:33 AM
> 
> > I'm not a maintainer, so can't approve your patch, however, I made a
> > few comments inline.
> 
> Thanks.
> 
> I am testing an updated patch that I believe addresses your issues
> with one exception --- see below.  Assuming it passes with no regressions,
> Ill rebase it as one commit and post it.
> 
> > Thanks,
> > Andrew
> > 
> > 
> > * David Taylor <dtaylor@emc.com> [2016-07-15 11:39:21 -0400]:
> 
> > > diff --git a/gdb/ax-general.c b/gdb/ax-general.c
> > > index 7f27a45..5406ab8 100644
> > > --- a/gdb/ax-general.c
> > > +++ b/gdb/ax-general.c
> > > @@ -323,6 +323,28 @@ ax_reg (struct agent_expr *x, int reg)
> > >      }
> > >  }
> > >
> > > +/* Assemble code to pop the top of stack and set register number REG
> > > +   to its value.  */
> > > +
> > > +void
> > > +ax_setreg (struct agent_expr *x, int reg)
> > > +{
> > > +  if (reg >= gdbarch_num_regs (x->gdbarch))
> > > +    error (_("register number '%d' is a pseudo register or is out of range"),
> > reg);
> > > +  else
> > > +    {
> > > +      /* Make sure the register number is in range.  */
> > > +      if (reg < 0 || reg > 0xffff)
> > > +        error (_("GDB bug: ax-general.c (ax_reg): "
> > > +		 "register number out of range"));
> > 
> > If this indicates a GDB bug then you should switch to either
> > internal_error or gdb_assert.  If you choose internal_error then you
> > can drop the 'GDB bug' prefix.
> 
> If I counted correctly, there are 9 other, pre-existing, instances of
> error (_("GDB bug: ..." in the file ax-general -- which is why I chose
> to do this one that way.  I can use internal_error if people feel that
> is better than consistency with rest of the file.

All of those occurrences of "GDB bug:... " date from the original
commit of GDB, back in 1999, which predated internal_error.

As I said, I'm not a maintainer, so they might disagree, but I think
new commits should probably use internal_error.  Obviously there's no
obligation to fix older code that you're not touching

Thanks,
Andrew
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4139a29..887551a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@ 
+2016-07-14  David Taylor  <dtaylor@emc.com>
+
+	* ax-gdb.c (gen_expr): Add support for assignment to more than
+	just trace state variables.
+	(gen_expr_binop_rest): Ditto.
+	* ax-general.c (ax_setreg): New function.
+	* ax.h (ax_setreg): Declare it.
+	* common/ax.def (setmem8, setmem16, setmem32, setmem64, setreg):
+	New agent bytecodes.
+
 2016-07-12  Tom Tromey  <tom@tromey.com>
 
 	PR python/19293:
diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
index 7c6cb64..d891ade 100644
--- a/gdb/ax-gdb.c
+++ b/gdb/ax-gdb.c
@@ -1895,6 +1895,7 @@  gen_expr (struct expression *exp, union exp_element **pc,
 
 	  (*pc) += 3;
 	  gen_expr (exp, pc, ax, value);
+	  require_rvalue (ax, value);
 	  tsv = find_trace_state_variable (name);
 	  if (tsv)
 	    {
@@ -1903,11 +1904,61 @@  gen_expr (struct expression *exp, union exp_element **pc,
 		ax_tsv (ax, aop_tracev, tsv->number);
 	    }
 	  else
-	    error (_("$%s is not a trace state variable, "
-		     "may not assign to it"), name);
+	    error (_("$%s is convenience variable, not a trace state variable, "
+		     "You may not assign to it."), name);
+	}
+      else if ((*pc)[0].opcode == OP_VAR_VALUE)
+	{
+	  int len;
+	  enum agent_op opcode;
+	  struct type *type = check_typedef (SYMBOL_TYPE ((*pc)[2].symbol));
+
+	  len = TYPE_LENGTH (type);
+
+	  if (len == 1)
+	    opcode = aop_setmem8;
+	  else if (len == 2)
+	    opcode = aop_setmem16;
+	  else if (len == 4)
+	    opcode = aop_setmem32;
+	  else if (len == 8)
+	    opcode = aop_setmem64;
+	  else
+	    error (_("Sorry, I don't have an opcode for types of length %d"), len);
+
+	  gen_expr (exp, pc, ax, &value1);
+
+	  if (value1.kind != axs_lvalue_register)
+	    {
+	      if (ax->tracing)
+		ax_trace_quick (ax, len); /* record original value */
+	    }
+
+	  gen_expr (exp, pc, ax, value);
+	  gen_usual_unary (exp, ax, value);
+	  require_rvalue (ax, value);
+
+	  if (value1.kind == axs_lvalue_register)
+	    {
+	      ax_reg_mask (ax, value1.u.reg);
+	      ax_setreg (ax, value1.u.reg);
+	    }
+	  else
+	    ax_simple (ax, opcode);
+	}
+      else if ((*pc)[0].opcode == OP_REGISTER)
+	{
+	  gen_expr (exp, pc, ax, &value1);
+
+	  gen_expr (exp, pc, ax, value);
+	  gen_usual_unary (exp, ax, value);
+	  require_rvalue (ax, value);
+
+	  ax_reg_mask (ax, value1.u.reg);
+	  ax_setreg (ax, value1.u.reg);
 	}
       else
-	error (_("May only assign to trace state variables"));
+	error (_("Sorry, I don't know how to do that (yet?)"));
       break;
 
     case BINOP_ASSIGN_MODIFY:
@@ -1939,8 +1990,74 @@  gen_expr (struct expression *exp, union exp_element **pc,
 		ax_tsv (ax, aop_tracev, tsv->number);
 	    }
 	  else
-	    error (_("$%s is not a trace state variable, "
-		     "may not assign to it"), name);
+	    error (_("Convenience variables such as '$%s' are known only to the debugger and "
+		     "you may not assign to them in an action."), name);
+	}
+      else if ((*pc)[0].opcode == OP_VAR_VALUE)
+	{
+	  int len;
+	  enum agent_op opcode;
+	  struct type *type;
+	  struct axs_value saved_value;
+
+	  gen_expr (exp, pc, ax, value);
+	  type = check_typedef (value->type);
+	  len = TYPE_LENGTH (type);
+
+	  if (len == 1)
+	    opcode = aop_setmem8;
+	  else if (len == 2)
+	    opcode = aop_setmem16;
+	  else if (len == 4)
+	    opcode = aop_setmem32;
+	  else if (len == 8)
+	    opcode = aop_setmem64;
+	  else
+	    error (_("Sorry, I don't have an opcode for types of length %d"), len);
+
+	  if (value->kind != axs_lvalue_register)
+	    {
+	      ax_simple (ax, aop_dup); /* keep the address around */
+
+	      if (ax->tracing)
+		ax_trace_quick (ax, len); /* record original value */
+	    }
+
+	  value1.kind = value->kind;
+	  value1.type = value->type;
+	  value1.u.reg = value->u.reg;
+
+	  /* gen_expr_binop_rest will set value, value1, and value2.
+	     And typically value for value, value1 ends up as
+	     axs_rvalue.  We need to remember whether it was a
+	     register and, if so, which register */
+	  saved_value.kind = value->kind;
+	  saved_value.u.reg = value->u.reg;
+
+	  /* Now do right half of expression.  */
+	  gen_expr_binop_rest (exp, op2, pc, ax, value, &value1, &value2);
+
+	  /* We have the result of the binary op, set the variable */
+	  if (saved_value.kind == axs_lvalue_register)
+	    {
+	      ax_reg_mask (ax, saved_value.u.reg);
+	      ax_setreg (ax, saved_value.u.reg);
+	    }
+	  else
+	    ax_simple (ax, opcode);
+	}
+      else if ((*pc)[0].opcode == OP_REGISTER)
+	{
+	  gen_expr (exp, pc, ax, value);
+
+	  value1.kind = value->kind;
+	  value1.type = value->type;
+	  value1.u.reg = value->u.reg;
+
+	  /* Now do right half of expression.  */
+	  gen_expr_binop_rest (exp, op2, pc, ax, value, &value1, &value2);
+
+	  ax_setreg (ax, value->u.reg);
 	}
       else
 	error (_("May only assign to trace state variables"));
@@ -2246,6 +2363,7 @@  gen_expr_binop_rest (struct expression *exp,
 {
   struct type *int_type = builtin_type (exp->gdbarch)->builtin_int;
 
+  require_rvalue (ax, value1);
   gen_expr (exp, pc, ax, value2);
   gen_usual_unary (exp, ax, value2);
   gen_usual_arithmetic (exp, ax, value1, value2);
diff --git a/gdb/ax-general.c b/gdb/ax-general.c
index 7f27a45..5406ab8 100644
--- a/gdb/ax-general.c
+++ b/gdb/ax-general.c
@@ -323,6 +323,28 @@  ax_reg (struct agent_expr *x, int reg)
     }
 }
 
+/* Assemble code to pop the top of stack and set register number REG
+   to its value.  */
+
+void
+ax_setreg (struct agent_expr *x, int reg)
+{
+  if (reg >= gdbarch_num_regs (x->gdbarch))
+    error (_("register number '%d' is a pseudo register or is out of range"), reg);
+  else
+    {
+      /* Make sure the register number is in range.  */
+      if (reg < 0 || reg > 0xffff)
+        error (_("GDB bug: ax-general.c (ax_reg): "
+		 "register number out of range"));
+      grow_expr (x, 3);
+      x->buf[x->len] = aop_setreg;
+      x->buf[x->len + 1] = (reg >> 8) & 0xff;
+      x->buf[x->len + 2] = (reg) & 0xff;
+      x->len += 3;
+    }
+}
+
 /* Assemble code to operate on a trace state variable.  */
 
 void
diff --git a/gdb/ax.h b/gdb/ax.h
index 536efe8..7b10355 100644
--- a/gdb/ax.h
+++ b/gdb/ax.h
@@ -234,6 +234,11 @@  extern void ax_const_d (struct agent_expr *EXPR, LONGEST d);
    stack.  */
 extern void ax_reg (struct agent_expr *EXPR, int REG);
 
+/* Assemble code to pop the top of stack and set register number REG
+   to its value.  */
+
+extern void ax_setreg (struct agent_expr *EXPR, int REG);
+
 /* Add the given register to the register mask of the expression.  */
 extern void ax_reg_mask (struct agent_expr *ax, int reg);
 
diff --git a/gdb/common/ax.def b/gdb/common/ax.def
index f375373..86cf4b5 100644
--- a/gdb/common/ax.def
+++ b/gdb/common/ax.def
@@ -95,3 +95,9 @@  DEFOP (pick, 1, 0, 0, 1, 0x32)
 DEFOP (rot, 0, 0, 3, 3, 0x33)
 /* Both the argument and consumed numbers are dynamic for this one.  */
 DEFOP (printf, 0, 0, 0, 0, 0x34)
+/* The following five operands are not yet implemented.  */
+DEFOP (setmem8, 0, 8, 2, 1, 0x35)
+DEFOP (setmem16, 0, 16, 2, 1, 0x36)
+DEFOP (setmem32, 0, 32, 2, 1, 0x37)
+DEFOP (setmem64, 0, 64, 2, 1, 0x38)
+DEFOP (setreg, 2, 0, 1, 1, 0x39)
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 385ca41..1ddaf61 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,8 @@ 
+2016-07-14  David Taylor  <dtaylor@emc.com>
+
+	* agentexpr.texi (Bytecode Descriptions): Add descriptions of
+	bytecodes setmem8, setmem16, setmem32, setmem64, and setreg.
+
 2016-07-12  Tom Tromey  <tom@tromey.com>
 
 	PR python/19293:
diff --git a/gdb/doc/agentexpr.texi b/gdb/doc/agentexpr.texi
index ade7638..23e0ccc 100644
--- a/gdb/doc/agentexpr.texi
+++ b/gdb/doc/agentexpr.texi
@@ -515,6 +515,25 @@  stack.  If the purpose of the expression was to compute an lvalue or a
 range of memory, then the next-to-top of the stack is the lvalue's
 address, and the top of the stack is the lvalue's size, in bytes.
 
+@item @code{setmem8} (0x35): @var{addr} @var{value} @result{} @var{value}
+@itemx @code{setmem16} (0x36): @var{addr} @var{value} @result{} @var{value}
+@itemx @code{setmem32} (0x37): @var{addr} @var{value} @result{} @var{value}
+@itemx @code{setmem64} (0x38): @var{addr} @var{value} @result{} @var{value}
+For bytecode @code{setmem}@var{n}, set an @var{n}-bit value at
+@var{addr}, using the @var{n}-bit least significant bits of @var{value}
+and natural target endianness.  The address @var{addr} is popped off the
+stack; the value @var{value} is left on the stack.
+
+If attempting to write memory at @var{addr} would cause a processor
+exception of some sort, terminate with an error.
+
+@item @code{setreg} (0x39) @var{regnum}: @var{value} @result{} @var{value}
+Set register @var{regnum} to @var{value}.  The value @var{value} is left
+on the stack.
+
+If the register is read-only or if attempting to write @var{value} to it
+would cause a processor exception of some sort, terminate with an error.
+
 @end table
 
 
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index cb87e48..e0f3a9b 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,11 @@ 
+2016-07-14  David Taylor  <dtaylor@emc.com>
+
+	* ax.c (compile_bytecodes): Add cases for new bytecodes (setmem8,
+	setmem16, setmem32, setmem64, and setreg).
+	* tracepoint.c (write_inferior_memory, agent_mem_write): New
+	functions.
+	* tracepoint.h (agent_mem_write): Declare.
+
 2016-07-12  Chung-Lin Tang  <cltang@codesourcery.com>
 
 	* linux-nios2-low.c (nios2_fill_gregset): Add type cast
diff --git a/gdb/gdbserver/ax.c b/gdb/gdbserver/ax.c
index d5144c2..bf33d42 100644
--- a/gdb/gdbserver/ax.c
+++ b/gdb/gdbserver/ax.c
@@ -736,6 +736,26 @@  compile_bytecodes (struct agent_expr *aexpr)
 	  UNHANDLED;
 	  break;
 
+	case gdb_agent_op_setmem8:
+	  UNHANDLED;
+	  break;
+
+	case gdb_agent_op_setmem16:
+	  UNHANDLED;
+	  break;
+
+	case gdb_agent_op_setmem32:
+	  UNHANDLED;
+	  break;
+
+	case gdb_agent_op_setmem64:
+	  UNHANDLED;
+	  break;
+
+	case gdb_agent_op_setreg:
+	  UNHANDLED;
+	  break;
+
 	  /* GDB never (currently) generates any of these ops.  */
 	case gdb_agent_op_float:
 	case gdb_agent_op_ref_float:
@@ -1322,6 +1342,53 @@  gdb_eval_agent_expr (struct eval_agent_expr_context *ctx,
 	  }
 	  break;
 
+	  /* for the setmem{8,16,32,64} opcodes, we leave the value on
+	     the stack (in top), but the address written to is popped
+	     from the stack */
+	case gdb_agent_op_setmem8:
+	  cnv.u8.val = (unsigned char) top;
+	  agent_mem_write (ctx, cnv.u8.bytes, stack[sp--], 1);
+	  break;
+
+	case gdb_agent_op_setmem16:
+	  cnv.u16.val = (unsigned short) top;
+	  agent_mem_write (ctx, cnv.u16.bytes, stack[sp--], 2);
+	  break;
+
+	case gdb_agent_op_setmem32:
+	  cnv.u32.val = (unsigned int) top;
+	  agent_mem_write (ctx, cnv.u32.bytes, stack[sp--], 4);
+	  break;
+
+	case gdb_agent_op_setmem64:
+	  cnv.u64.val = (ULONGEST) top;
+	  agent_mem_write (ctx, cnv.u64.bytes, stack[sp--], 8);
+	  break;
+
+	case gdb_agent_op_setreg:
+	  arg = aexpr->bytes[pc++];
+	  arg = (arg << 8) + aexpr->bytes[pc++];
+	  switch (register_size (ctx->regcache->tdesc, arg))
+	    {
+	    case 8:
+	      cnv.u8.val = (unsigned char) top;
+	      break;
+	    case 16:
+	      cnv.u16.val = (unsigned short) top;
+	      break;
+	    case 32:
+	      cnv.u32.val = (unsigned int) top;
+	      break;
+	    case 64:
+	      cnv.u64.val = (ULONGEST) top;
+	      break;
+	    default:
+	      internal_error (__FILE__, __LINE__,
+			      "unhandled register size");
+	    }
+	  supply_register (ctx->regcache, arg, &cnv);
+	  break;
+
 	  /* GDB never (currently) generates any of these ops.  */
 	case gdb_agent_op_float:
 	case gdb_agent_op_ref_float:
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index c07e525..c4ee465 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -357,6 +357,13 @@  read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
   return 0;
 }
 
+int
+write_inferior_memory (CORE_ADDR memaddr, unsigned char const *myaddr, int len)
+{
+  memcpy ((void *) (uintptr_t) memaddr, myaddr, len);
+  return 0;
+}
+
 /* Call this in the functions where GDBserver places a breakpoint, so
    that the compiler doesn't try to be clever and skip calling the
    function at all.  This is necessary, even if we tell the compiler
@@ -4948,6 +4955,14 @@  condition_true_at_tracepoint (struct tracepoint_hit_ctx *ctx,
 }
 
 /* Do memory copies for bytecodes.  */
+int
+agent_mem_write (struct eval_agent_expr_context *ctx,
+		 unsigned char *from, CORE_ADDR to, ULONGEST len)
+{
+  return write_inferior_memory (to, from, len);
+}
+
+/* Do memory copies for bytecodes.  */
 /* Do the recording of memory blocks for actions and bytecodes.  */
 
 int
diff --git a/gdb/gdbserver/tracepoint.h b/gdb/gdbserver/tracepoint.h
index 679a32a..0886f8e 100644
--- a/gdb/gdbserver/tracepoint.h
+++ b/gdb/gdbserver/tracepoint.h
@@ -154,6 +154,10 @@  int agent_mem_read (struct eval_agent_expr_context *ctx,
 		    unsigned char *to, CORE_ADDR from,
 		    ULONGEST len);
 
+int agent_mem_write (struct eval_agent_expr_context *ctx,
+		     unsigned char *from, CORE_ADDR to,
+		     ULONGEST len);
+
 LONGEST agent_get_trace_state_variable_value (int num);
 void agent_set_trace_state_variable_value (int num, LONGEST val);
 
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 7ab1228..7a33041 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,10 @@ 
+2016-07-14  David Taylor  <dtaylor@emc.com>
+
+	* gdb.trace/actions.c: (get_int_test): New variable.
+	(struct GDB_STRUCT_TEST): New int field.
+	(gdb_c_test): Assign values to new variable and struct member.
+	* gdb.trace/ax.exp: Add new tests.
+
 2016-07-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* gdb.dwarf2/atomic-type.exp: Use function_range for low_pc and high_pc.
diff --git a/gdb/testsuite/gdb.trace/actions.c b/gdb/testsuite/gdb.trace/actions.c
index aedd202..ebc1559 100644
--- a/gdb/testsuite/gdb.trace/actions.c
+++ b/gdb/testsuite/gdb.trace/actions.c
@@ -25,12 +25,14 @@ 
 
 static char   gdb_char_test;
 static short  gdb_short_test;
+static int    gdb_int_test;
 static long   gdb_long_test;
 static char   gdb_arr_test[25];
 static struct GDB_STRUCT_TEST
 {
   char   c;
   short  s;
+  int    i;
   long   l;
   int    bfield : 11;	/* collect bitfield */
   char   arr[25];
@@ -97,6 +99,7 @@  unsigned long   gdb_c_test( unsigned long *parm )
 
    gdb_char_test   = gdb_struct1_test.c = (char)   ((long) parm[1] & 0xff);
    gdb_short_test  = gdb_struct1_test.s = (short)  ((long) parm[2] & 0xffff);
+   gdb_int_test    = gdb_struct1_test.i = (int)    ((long) parm[3] & 0xffffffff);
    gdb_long_test   = gdb_struct1_test.l = (long)   ((long) parm[3] & 0xffffffff);
    gdb_union1_test.l = (long) parm[4];
    gdb_arr_test[0] = gdb_struct1_test.arr[0] = (char) ((long) parm[1] & 0xff);
@@ -113,7 +116,7 @@  unsigned long   gdb_c_test( unsigned long *parm )
    gdb_recursion_test_ptr (3, (long) parm[1], (long) parm[2], (long) parm[3],
 		       (long) parm[4], (long) parm[5], (long) parm[6]);
 
-   gdb_char_test = gdb_short_test = gdb_long_test = 0;
+   gdb_char_test = gdb_short_test = gdb_int_test = gdb_long_test = 0;
    gdb_structp_test  = (void *) 0;
    gdb_structpp_test = (void *) 0;
    memset ((char *) &gdb_struct1_test, 0, sizeof (gdb_struct1_test));
diff --git a/gdb/testsuite/gdb.trace/ax.exp b/gdb/testsuite/gdb.trace/ax.exp
index 5e9783f..da424f3 100644
--- a/gdb/testsuite/gdb.trace/ax.exp
+++ b/gdb/testsuite/gdb.trace/ax.exp
@@ -82,6 +82,17 @@  gdb_test "maint agent &gdb_long_test < &gdb_short_test" "" "maint agent &gdb_lon
 
 gdb_test "maint agent (unsigned char)1L" ".*ext 8.*" "maint agent (unsigned char)1L"
 
+gdb_test "maint agent long_long = 12" "" "maint agent long_long = 12"
+
+gdb_test "maint agent gdb_char_test = 19" "" "maint agent gdb_char_test = 19"
+
+gdb_test "maint agent gdb_short_test = 24" "" "maint agent gdb_short_test = 24"
+
+gdb_test "maint agent gdb_int_test = 35" "" "maint agent gdb_int_test = 35"
+
+gdb_test "maint agent gdb_long_test = 45" "" "maint agent gdb_long_test = 45"
+
+gdb_test "maint agent -at gdb_recursion_test, depth = 100" "" "maint agent -at gdb_recursion_test, depth = 100"
 # Now test eval version of agent expressions.
 
 gdb_test "maint agent-eval 12" ".*const8 12.*end.*" "maint agent-eval 12"
@@ -130,3 +141,15 @@  gdb_test "maint agent-eval &gdb_long_test == &gdb_short_test" ".*equal.*end.*" "
 
 gdb_test "maint agent-eval &gdb_long_test < &gdb_short_test" "" "maint agent-eval &gdb_long_test < &gdb_short_test"
 
+
+gdb_test "maint agent-eval long_long = 12" "" "maint agent long_long = 12"
+
+gdb_test "maint agent-eval gdb_char_test = 19" "" "maint agent gdb_char_test = 19"
+
+gdb_test "maint agent-eval gdb_short_test = 24" "" "maint agent gdb_short_test = 24"
+
+gdb_test "maint agent-eval gdb_int_test = 35" "" "maint agent gdb_int_test = 35"
+
+gdb_test "maint agent-eval gdb_long_test = 45" "" "maint agent gdb_long_test = 45"
+
+gdb_test "maint agent-eval -at gdb_recursion_test, depth = 100" "" "maint agent -at gdb_recursion_test, depth = 100"