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

Andreas Arnez
2016-01-26T14:58:59Z

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);
> + }
> }
> }
> }