Message ID | 14037.1468597161@usendtaylorx2l |
---|---|
State | New |
Headers | show |
I forgot to add -- built and tested on x86-64 GNU/Linux with no regressions.
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 >
> 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.
* 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 --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"