Re: [PATCH] Dynamic printf for a target agent

[Stan Shebs <stanshebs at earthlink dot net>]

On 6/27/12 1:44 PM, Tom Tromey wrote:
> > > > > > "Stan" == Stan Shebs <stanshebs@earthlink.net> writes:
> Stan> +static void
> Stan> +maint_agent_printf_command (char *exp, int from_tty)
> Stan> +{
> [...]
> Stan> +      expr = parse_exp_1 (&cmd1, (struct block *) 0, 1);
>
> Now that Hui's "-at" patch is going in, perhaps this function should
> have the same treatment.

Yeah, that would be good for him to do. :-)

> Stan> +void
> Stan> +ax_string (struct agent_expr *x, char *str, int slen)
> Stan> +{
> Stan> +  int i;
> Stan> +
> Stan> +  grow_expr (x, slen + 3);
> Stan> +  x->buf[x->len++] = ((slen + 1) >> 8) & 0xff;
> Stan> +  x->buf[x->len++] = (slen + 1) & 0xff;
>
> I think this should check that the length fits in 2 bytes.

Done.

> Stan> +@item @code{printf} (0x34)  @var{numargs} @var{string} @result{}
> Stan> +Do a formatted print, in the style of the C function @code{printf}).
> Stan> +The value of @var{numargs} is the number of arguments to expect on the
> Stan> +stack, while @var{string} is the format string, prefixed with a
> Stan> +two-byte length, and suffixed with a zero byte.  The format string
>
> I think the docs should whether the length includes the zero byte.

I clarified the doc.

> Stan> +	case string_arg:
> Stan> +	  {
> Stan> +	    gdb_byte *str;
> Stan> +	    CORE_ADDR tem;
> Stan> +	    int j;
> Stan> +
> Stan> +	    tem = args[i];
> Stan> +
> Stan> +	    /* This is a %s argument.  Find the length of the string.  */
> Stan> +	    for (j = 0;; j++)
> Stan> +	      {
> Stan> +		gdb_byte c;
> Stan> +
> Stan> +		read_inferior_memory (tem + j, &c, 1);
> Stan> +		if (c == 0)
> Stan> +		  break;
> Stan> +	      }
> Stan> +
> Stan> +	      /* Copy the string contents into a string inside GDB.  */
> Stan> +	      str = (gdb_byte *) alloca (j + 1);
> Stan> +	      if (j != 0)
> Stan> +		read_inferior_memory (tem, str, j);
> Stan> +	      str[j] = 0;
> Stan> +
> Stan> +              printf (current_substring, (char *) str);
>
> Is it ever possible for the argument to "%s" to be NULL?  It seems like
> it should be; but then the length-finding code seems wrong, and the
> printing of a NULL should be handled directly, not left to the host
> printf.

A NULL argument to %s is not necessarily a problem, depending on the 
target.  In any case, it's left up to read_inferior_memory to error out 
if any part of the string is not at a valid memory address.

> Stan> +	case gdb_agent_op_printf:
> Stan> +	  {
> Stan> +	    int nargs, slen, i;
> Stan> +	    CORE_ADDR fn = 0, chan = 0;
> Stan> +	    /* Can't have more args than the entire size of the stack.  */
> Stan> +	    ULONGEST args[STACK_MAX];
> Stan> +	    char *format;
> Stan> +
> Stan> +	    nargs = aexpr->bytes[pc++];
> Stan> +	    slen = aexpr->bytes[pc++];
> Stan> +	    slen = (slen << 8) + aexpr->bytes[pc++];
> Stan> +	    format = (char *) &(aexpr->bytes[pc]);
>
> Perhaps double-check that the terminating \0 byte is in fact present.

Also a good idea, and done.

> Stan> +if $target_can_dprintf {
> Stan> +
> Stan> +    gdb_run_cmd
> Stan> +
> Stan> +    gdb_test "" "Breakpoint"
>
> This seems weird.

It does look a little weird, but seems to be done a fair amount, not 
always obviously because it's often broken up over several lines.

I went ahead and committed with these changes, so we can start 
collecting some user experience.

Stan
stan@codesourcery.com

2012-07-02  Stan Shebs  <stan@codesourcery.com>

    Add target-side support for dynamic printf.
    * NEWS: Mention the additional style.
    * breakpoint.h (struct bp_target_info): New fields tcommands, persist.
    (struct bp_location): New field cmd_bytecode.
    * breakpoint.c: Include format.h.
    (disconnected_dprintf): New global.
    (parse_cmd_to_aexpr): New function.
    (build_target_command_list): New function.
    (insert_bp_location): Call it.
    (remove_breakpoints_pid): Skip dprintf breakpoints.
    (print_one_breakpoint_location): Ditto.
    (dprintf_style_agent): New global.
    (dprintf_style_enums): Add dprintf_style_agent.
    (update_dprintf_command_list): Add agent case.
    (agent_printf_command): New function.
    (_initialize_breakpoint): Add new commands.
    * common/ax.def (printf): New bytecode.
    * ax.h (ax_string): Declare.
    * ax-gdb.h (gen_printf): Declare.
    * ax-gdb.c: Include cli-utils.h, format.h.
    (gen_printf): New function.
    (maint_agent_print_command): New function.
    (_initialize_ax_gdb): Add maint agent-printf command.
    * ax-general.c (ax_string): New function.
    (ax_print): Add printf disassembly.
    * Makefile.in (SFILES): Add format.c
    (COMMON_OBS): Add format.o.
    * common/format.h: New file.
    * common/format.c: New file.
    * printcmd.c: Include format.h.
    (ui_printf): Call parse_format_string.
    * remote.c (remote_state): New field breakpoint_commands.
    (PACKET_BreakpointCommands): New enum.
    (remote_breakpoint_commands_feature): New function.
    (remote_protocol_features): Add new BreakpointCommands entry.
    (remote_can_run_breakpoint_commands): New function.
    (remote_add_target_side_commands): New function.
    (remote_insert_breakpoint): Call it.
    (remote_insert_hw_breakpoint): Ditto.
    (_initialize_remote): Add new packet configuration for
    target-side breakpoint commands.
    * target.h (struct target_ops): New field
    to_can_run_breakpoint_commands.
    (target_can_run_breakpoint_commands): New macro.
    * target.c (update_current_target): Handle
    to_can_run_breakpoint_commands.

    [gdbserver]
    * Makefile.in (WARN_CFLAGS_NO_FORMAT): Define.
    (ax.o): Add it to build rule.
    (ax-ipa.o): Ditto.
    (OBS): Add format.o.
    (IPA_OBS): Add format.o.
    * server.c (handle_query): Claim support for breakpoint commands.
    (process_point_options): Add command case.
    (process_serial_event): Leave running if there are printfs in
    effect.
    * mem-break.h (any_persistent_commands): Declare.
    (add_breakpoint_commands): Declare.
    (gdb_no_commands_at_breakpoint): Declare.
    (run_breakpoint_commands): Declare.
    * mem-break.c (struct point_command_list): New struct.
    (struct breakpoint): New field command_list.
    (any_persistent_commands): New function.
    (add_commands_to_breakpoint): New function.
    (add_breakpoint_commands): New function.
    (gdb_no_commands_at_breakpoint): New function.
    (run_breakpoint_commands): New function.
    * linux-low.c (linux_wait_1): Test for and run breakpoint commands
    locally.
    * ax.c: Include format.h.
    (ax_printf): New function.
    (gdb_eval_agent_expr): Add printf opcode.

    [doc]
    * gdb.texinfo (Dynamic Printf): Mention agent style and
    disconnected dprintf.
    (Maintenance Commands): Describe maint agent-printf.
    (General Query Packets): Mention BreakpointCommands feature.
    (Packets): Document commands extension to Z0 packet.
    * agentexpr.texi (Bytecode Descriptions): Document printf
    bytecode.

    [testsuite]
    * gdb.base/dprintf.exp: Add agent style tests.

Attachment: dprintf-patch-6
Description: Text document