[PING][PATCH] Remove isize output argument from fast_tracepoint_valid_at

[Pierre Langlois <pierre dot langlois at arm dot com>]

On 24/07/15 10:24, Pierre Langlois wrote:
> Hi all,
> 
> This patch removes the isize output argument from the
> fast_tracepoint_valid_at gdbarch hook.  It was used to return the size
> of the instruction that needs to be replaced when installing a fast
> tracepoint.  Instead of getting this value from the
> fast_tracepoint_valid_at hook, we can call the gdb_insn_length function.
> 
> If we do not do this, then architectures which do not have a restriction
> on where to install the fast tracepoint will send uninitialized memory
> off to GDBserver.  See remote_download_tracepoint:
> 
> ~~~
> int isize;
> 
> if (gdbarch_fast_tracepoint_valid_at (target_gdbarch (),
> 				      tpaddr, &isize, NULL))
>   xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":F%x",
> 	     isize);
> ~~~
> 
> The default implementation of fast_tracepoint_valid_at will not set
> isize resulting in uninitialized memory being sent.  Later on, GDBserver
> could use this information to compute a jump offset.
> 
> Regression tested with native-gdbserver on x86_64.
> 
> Thanks,
> Pierre
> 
> gdb/ChangeLog:
> 
> 	* arch-utils.c (default_fast_tracepoint_valid_at): Remove unused
> 	isize argument.
> 	* arch-utils.h (default_fast_tracepoint_valid_at): Likewise.
> 	* breakpoint.c (check_fast_tracepoint_sals): Adjust call to
> 	gdbarch_fast_tracepoint_valid_at.
> 	* gdbarch.sh (fast_tracepoint_valid_at): Remove isize argument.
> 	* gdbarch.h: Regenerate.
> 	* gdbarch.c: Regenerate.
> 	* i386-tdep.c (i386_fast_tracepoint_valid_at): Remove isize
> 	argument.  Do not set it.
> 	* remote.c (remote_download_tracepoint): Adjust call to
> 	gdbarch_fast_tracepoint_valid_at.  Call gdb_insn_length to get
> 	the instruction length.

Any thoughts on this patch?

Thanks,
Pierre

> ---
>  gdb/arch-utils.c | 4 ++--
>  gdb/arch-utils.h | 3 +--
>  gdb/breakpoint.c | 3 +--
>  gdb/gdbarch.c    | 4 ++--
>  gdb/gdbarch.h    | 4 ++--
>  gdb/gdbarch.sh   | 2 +-
>  gdb/i386-tdep.c  | 6 ++----
>  gdb/remote.c     | 9 ++++-----
>  8 files changed, 15 insertions(+), 20 deletions(-)
> 
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index e9c622d..46a6db0 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -795,8 +795,8 @@ default_has_shared_address_space (struct gdbarch *gdbarch)
>  }
>  
>  int
> -default_fast_tracepoint_valid_at (struct gdbarch *gdbarch,
> -				  CORE_ADDR addr, int *isize, char **msg)
> +default_fast_tracepoint_valid_at (struct gdbarch *gdbarch, CORE_ADDR addr,
> +				  char **msg)
>  {
>    /* We don't know if maybe the target has some way to do fast
>       tracepoints that doesn't need gdbarch, so always say yes.  */
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index 27f4787..18e2290 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -161,8 +161,7 @@ extern struct gdbarch *get_current_arch (void);
>  extern int default_has_shared_address_space (struct gdbarch *);
>  
>  extern int default_fast_tracepoint_valid_at (struct gdbarch *gdbarch,
> -					     CORE_ADDR addr,
> -					     int *isize, char **msg);
> +					     CORE_ADDR addr, char **msg);
>  
>  extern void default_remote_breakpoint_from_pc (struct gdbarch *,
>  					       CORE_ADDR *pcptr, int *kindptr);
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index af0d167..2a55a6f 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -9406,8 +9406,7 @@ check_fast_tracepoint_sals (struct gdbarch *gdbarch,
>  	 associated with SAL.  */
>        if (sarch == NULL)
>  	sarch = gdbarch;
> -      rslt = gdbarch_fast_tracepoint_valid_at (sarch, sal->pc,
> -					       NULL, &msg);
> +      rslt = gdbarch_fast_tracepoint_valid_at (sarch, sal->pc, &msg);
>        old_chain = make_cleanup (xfree, msg);
>  
>        if (!rslt)
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index c289334..07b38a3 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -4394,13 +4394,13 @@ set_gdbarch_has_shared_address_space (struct gdbarch *gdbarch,
>  }
>  
>  int
> -gdbarch_fast_tracepoint_valid_at (struct gdbarch *gdbarch, CORE_ADDR addr, int *isize, char **msg)
> +gdbarch_fast_tracepoint_valid_at (struct gdbarch *gdbarch, CORE_ADDR addr, char **msg)
>  {
>    gdb_assert (gdbarch != NULL);
>    gdb_assert (gdbarch->fast_tracepoint_valid_at != NULL);
>    if (gdbarch_debug >= 2)
>      fprintf_unfiltered (gdb_stdlog, "gdbarch_fast_tracepoint_valid_at called\n");
> -  return gdbarch->fast_tracepoint_valid_at (gdbarch, addr, isize, msg);
> +  return gdbarch->fast_tracepoint_valid_at (gdbarch, addr, msg);
>  }
>  
>  void
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 7d6a0cf..d714281 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -1310,8 +1310,8 @@ extern void set_gdbarch_has_shared_address_space (struct gdbarch *gdbarch, gdbar
>  
>  /* True if a fast tracepoint can be set at an address. */
>  
> -typedef int (gdbarch_fast_tracepoint_valid_at_ftype) (struct gdbarch *gdbarch, CORE_ADDR addr, int *isize, char **msg);
> -extern int gdbarch_fast_tracepoint_valid_at (struct gdbarch *gdbarch, CORE_ADDR addr, int *isize, char **msg);
> +typedef int (gdbarch_fast_tracepoint_valid_at_ftype) (struct gdbarch *gdbarch, CORE_ADDR addr, char **msg);
> +extern int gdbarch_fast_tracepoint_valid_at (struct gdbarch *gdbarch, CORE_ADDR addr, char **msg);
>  extern void set_gdbarch_fast_tracepoint_valid_at (struct gdbarch *gdbarch, gdbarch_fast_tracepoint_valid_at_ftype *fast_tracepoint_valid_at);
>  
>  /* Return the "auto" target charset. */
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index 6c5d684..14a5f9c 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -1018,7 +1018,7 @@ v:int:has_global_breakpoints:::0:0::0
>  m:int:has_shared_address_space:void:::default_has_shared_address_space::0
>  
>  # True if a fast tracepoint can be set at an address.
> -m:int:fast_tracepoint_valid_at:CORE_ADDR addr, int *isize, char **msg:addr, isize, msg::default_fast_tracepoint_valid_at::0
> +m:int:fast_tracepoint_valid_at:CORE_ADDR addr, char **msg:addr, msg::default_fast_tracepoint_valid_at::0
>  
>  # Return the "auto" target charset.
>  f:const char *:auto_charset:void::default_auto_charset:default_auto_charset::0
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 371a282..9d52d4a 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -8045,8 +8045,8 @@ static const int i386_record_regmap[] =
>     string.  */
>  
>  static int
> -i386_fast_tracepoint_valid_at (struct gdbarch *gdbarch,
> -			       CORE_ADDR addr, int *isize, char **msg)
> +i386_fast_tracepoint_valid_at (struct gdbarch *gdbarch, CORE_ADDR addr,
> +			       char **msg)
>  {
>    int len, jumplen;
>    static struct ui_file *gdb_null = NULL;
> @@ -8078,8 +8078,6 @@ i386_fast_tracepoint_valid_at (struct gdbarch *gdbarch,
>  
>    /* Check for fit.  */
>    len = gdb_print_insn (gdbarch, addr, gdb_null, NULL);
> -  if (isize)
> -    *isize = len;
>  
>    if (len < jumplen)
>      {
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 94899bd..3e685c4 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -44,6 +44,7 @@
>  #include "gdb_bfd.h"
>  #include "filestuff.h"
>  #include "rsp-low.h"
> +#include "disasm.h"
>  
>  #include <sys/time.h>
>  
> @@ -11082,12 +11083,10 @@ remote_download_tracepoint (struct target_ops *self, struct bp_location *loc)
>  	 target capabilities at definition time.  */
>        if (remote_supports_fast_tracepoints ())
>  	{
> -	  int isize;
> -
> -	  if (gdbarch_fast_tracepoint_valid_at (target_gdbarch (),
> -						tpaddr, &isize, NULL))
> +	  if (gdbarch_fast_tracepoint_valid_at (loc->gdbarch, tpaddr,
> +						NULL))
>  	    xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":F%x",
> -		       isize);
> +		       gdb_insn_length (loc->gdbarch, tpaddr));
>  	  else
>  	    /* If it passed validation at definition but fails now,
>  	       something is very wrong.  */
>