Re: Target-specific command logging facility

[Jim Blandy <jimb@codesourcery.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.