On 6/27/12 1:44 PM, Tom Tromey wrote: >>>>>> "Stan" == Stan Shebs 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 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.