Re: [PATCH 1/8] gdb: Add write_guessed_tracepoint_pc hook to gdbarch.

[Andreas Arnez <arnez@linux.vnet.ibm.com>]

On Sun, Jan 24 2016, Marcin Kościelnicki wrote:

> When we're looking at a tracefile trace frame where registers are not
> available, and the tracepoint has only one location, we supply
> the location's address as the PC register.  However, this only works
> if PC is not a pseudo register.  Add a gdbarch hook that will handle
> that for pseudo registers.

> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index 4ac6b90..b67ea72 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -417,6 +417,7 @@ v:int:char_signed:::1:-1:1
>  #
>  F:CORE_ADDR:read_pc:struct regcache *regcache:regcache
>  F:void:write_pc:struct regcache *regcache, CORE_ADDR val:regcache, val
> +F:void:write_guessed_tracepoint_pc:struct regcache *regcache, CORE_ADDR val:regcache, val

Rather than write_guessed_tracepoint_pc, maybe the method could be named
more generically, like "supply_pc"?  Also, a comment describing the
method's purpose and interface would be nice, particularly how it
differs from the "write_pc" method or from regcache_write_pc().

>  # Function for getting target's idea of a frame pointer.  FIXME: GDB's
>  # whole scheme for dealing with "frames" and "frame pointers" needs a
>  # serious shakedown.
> diff --git a/gdb/tracefile.c b/gdb/tracefile.c
> index fef4ed9..7c2649d 100644
> --- a/gdb/tracefile.c
> +++ b/gdb/tracefile.c
> @@ -396,16 +396,18 @@ tracefile_fetch_registers (struct regcache *regcache, int regno)
>       as the address of the tracepoint.  */
>    pc_regno = gdbarch_pc_regnum (gdbarch);
>  
> -  /* XXX This guessing code below only works if the PC register isn't
> -     a pseudo-register.  The value of a pseudo-register isn't stored
> -     in the (non-readonly) regcache -- instead it's recomputed
> -     (probably from some other cached raw register) whenever the
> -     register is read.  This guesswork should probably move to some
> -     higher layer.  */

I wonder whether the last statement in this comment still applies?

> -  if (pc_regno < 0 || pc_regno >= gdbarch_num_regs (gdbarch))
> +  if (pc_regno < 0)
>      return;
>  
> -  if (regno == -1 || regno == pc_regno)
> +  /* We try to guess PC if:
> +
> +     1) We want all registers, or
> +     2) PC is a real register, and we want exactly it, or
> +     3) PC is a pseudo register (we don't know which real register it
> +        corresponds to, so let's try to play safe).  */
> +
> +  if (regno == -1 || regno == pc_regno ||
> +      pc_regno >= gdbarch_num_regs (gdbarch))
>      {
>        struct tracepoint *tp = get_tracepoint (get_tracepoint_number ());
>        gdb_byte *regs;
> @@ -429,11 +431,23 @@ tracefile_fetch_registers (struct regcache *regcache, int regno)
>  	      return;
>  	    }
>  
> -	  regs = (gdb_byte *) alloca (register_size (gdbarch, pc_regno));
> -	  store_unsigned_integer (regs, register_size (gdbarch, pc_regno),
> -				  gdbarch_byte_order (gdbarch),
> -				  tp->base.loc->address);
> -	  regcache_raw_supply (regcache, pc_regno, regs);
> +	  if (pc_regno >= gdbarch_num_regs (gdbarch))
> +	    {
> +	      /* PC is a pseudo, let gdbarch deal with that.  If it doesn't
> +	         know how, just bail.  */

Nit: The indentation space of this line and the second one below
contains 8 consecutive spaces where a tab could be used instead.  This
occurs several times in the patch series.

> +	      if (gdbarch_write_guessed_tracepoint_pc_p (gdbarch))
> +	        gdbarch_write_guessed_tracepoint_pc (gdbarch, regcache,
> +						     tp->base.loc->address);
> +	    }
> +	  else
> +	    {
> +	      /* PC is a real register.  */
> +	      regs = (gdb_byte *) alloca (register_size (gdbarch, pc_regno));
> +	      store_unsigned_integer (regs, register_size (gdbarch, pc_regno),
> +				      gdbarch_byte_order (gdbarch),
> +				      tp->base.loc->address);
> +	      regcache_raw_supply (regcache, pc_regno, regs);
> +	    }
>  	}
>      }
>  }