This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Target-specific command logging facility
- From: Jim Blandy <jimb at codesourcery dot com>
- To: "Maciej W. Rozycki" <macro at mips dot com>
- Cc: gdb-patches at sourceware dot org, "Maciej W. Rozycki" <macro at linux-mips dot org>
- Date: Thu, 29 Nov 2007 10:19:31 -0800
- Subject: Re: Target-specific command logging facility
- References: <Pine.LNX.4.61.0711291525550.3161@perivale.mips.com>
"Maciej W. Rozycki" <macro at mips.com> writes:
> In preparation for submitting support for the MDI target I propose the
> following enhancement to the command logging facility. Right now
> serial_log_command() is called unconditionally which does nothing if the
> serial target subsystem is not being used.
>
> I propose to change this behaviour and provide a (*to_log_command)()
> member of the target vector so that the callee may be selected as
> appropriate. The change below is meant to preserve the current semantics
> -- all the currently supported targets that make use of the serial target
> subsystem initialise the new member to serial_log_command().
>
> Tested using the mipsisa32-sde-elf target, with the mips-sim-sde32/-EB
> and mips-sim-sde32/-EL boards with no regressions.
>
> 2007-11-29 Maciej W. Rozycki <macro@mips.com>
>
> * target.c (update_current_target): Inherit to_log_command.
> * target.h (struct target_ops). Add to_log_command.
> (target_log_command): New macro.
> * top.c (execute_command): Call target_log_command() if available
> rather than serial_log_command().
> * monitor.c (init_base_monitor_ops): Initialize to_log_command.
> * remote-m32r-sdi.c (init_m32r_ops): Likewise.
> * remote-mips.c (_initialize_remote_mips): Likewise.
> * remote.c (init_remote_ops): Likewise.
>
> OK to apply?
This looks good; I have some minor suggestions.
> 14616.diff
> Index: binutils-quilt/src/gdb/target.h
> ===================================================================
> --- binutils-quilt.orig/src/gdb/target.h 2007-04-16 13:04:50.000000000 +0100
> +++ binutils-quilt/src/gdb/target.h 2007-04-16 13:06:27.000000000 +0100
> @@ -402,6 +402,7 @@
> int);
> struct exception_event_record *(*to_get_current_exception_event) (void);
> char *(*to_pid_to_exec_file) (int pid);
> + void (*to_log_command) (const char *);
> enum strata to_stratum;
> int to_has_all_memory;
> int to_has_memory;
> @@ -1201,6 +1202,10 @@
>
> extern const struct target_desc *target_read_description (struct target_ops *);
>
> +/* Command logging facility. */
> +
> +#define target_log_command current_target.to_log_command
Could we have parens around the definition here?
> Index: binutils-quilt/src/gdb/top.c
> ===================================================================
> --- binutils-quilt.orig/src/gdb/top.c 2007-04-16 12:14:49.000000000 +0100
> +++ binutils-quilt/src/gdb/top.c 2007-04-16 13:06:27.000000000 +0100
> @@ -387,7 +387,8 @@
> if (p == NULL)
> return;
>
> - serial_log_command (p);
> + if (target_log_command)
> + target_log_command (p);
I think it's better style to have the conditional in the definition of
target_log_command itself, so that every caller doesn't have to test
whether current target happens to have such a function. I grant this
is the only caller, but people do look at existing code for precedent.
If you fix those, this is fine to commit.