Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Friday, January 10 2014, Pedro Alves wrote:

> Hmm, OK, I think I see.  I don't think the register block
> size in the header needs to be correct for this test -- tfile.c
> portability sounds like a red herring to me.  (I don't think 500
> is correct for x86/x86_64 either?)  There's no register block
> in the trace file anyway.  Only memory blocks.
>
> tfile knows to infer the PC from the tracepoint's address if
> the PC wasn't collected (tfile_fetch_registers) but,
> (guessing) the issue is that PC register in s390 is a
> pseudo-register.  tfile_fetch_registers guesses the PC from the
> tracepoint's address and stores it in the regcache, but
> when reading a (non-readonly) pseudo register from a regcache,
> we never use the stored value in the cache -- we always
> recompute it.  In fact, writing the pseudo PC to the regcache
> in tfile_fetch_registers with regcache_raw_supply seems reachable
> when regnum==-1, and I'm surprised this isn't triggering the assertion
> that makes sure the regnum being written to is a raw register
> (as the regcache's buffer only holds the raw registers -- see
> regcache_xmalloc_1 and regcache_raw_write).

Thanks for the investigation.

By debugging it a little more, I see that we don't write the pseudo PC
to the regcache.  You are probably talking about this loop:

  /* We get here if no register data has been found.  Mark registers
     as unavailable.  */
  for (regn = 0; regn < gdbarch_num_regs (gdbarch); regn++)
    regcache_raw_supply (regcache, regn, NULL);

The pseudo PC register number on s390x is 90, and "gdbarch_num_regs
(gdbarch)" returns exactly 90.

What happens is that when one requests the pseudo PC on s390x, it reads
S390_PSWM_REGNUM, which is 1:

  static enum register_status
  s390_pseudo_register_read (struct gdbarch *gdbarch, struct regcache *regcache,
                             int regnum, gdb_byte *buf)
  {
    struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
    int regsize = register_size (gdbarch, regnum);
    ULONGEST val;

    if (regnum == tdep->pc_regnum)
      {
        enum register_status status;

        status = regcache_raw_read_unsigned (regcache, S390_PSWA_REGNUM, &val);

Then, this check on tfile_fetch_registers evaluates to false:

  /* We can often usefully guess that the PC is going to be the same
     as the address of the tracepoint.  */
  pc_regno = gdbarch_pc_regnum (gdbarch);
  if (pc_regno >= 0 && (regno == -1 || regno == pc_regno))

Because regno == 1 and pc_regno == 90.  Maybe you knew all that, but I
thought it would be better to explicitly mention.

> When debugging a live traceframe, i.e., with gdbserver,
> it's gdbserver itself that does this same PC guessing.  And
> of course, that also only works when the PC isn't a pseudo
> register, as the target backend only ever sees raw registers
> accesses.
>
> Seems like this PC guessing should move out of
> target_fetch_registers to somewhere higher level...
>
> I.e., this is not a bug specific to s390, but to the
> tracing/unavailable machinery itself, for all targets
> whose PC is a pseudo register.

I see.  I haven't had the chance to test on other targets where PC is a
pseud register.

> It's fine with me to skip the test on targets with pseudo
> register PCs for now, though probably we should record this
> info somewhere, probably in the code, like in the patch below.

Nice, I like the comments, though your patch doesn't really change
anything for s390x because of what I explained above (regno == 1 and
pc_regno == 90), but I like to make things more explicit and your patch
does that.

I can push both our patches if you wish, btw.

>> There is also another error before, but it was already failing for
>> 7.6.2.
>
> Hard to say anything about it if you don't show it.  :-)

The same error:

&"tfind 0\n"^M
&"PC not available\n"^M
~"Found trace frame 0, tracepoint 1\n"^M
~"#-1 <unavailable> in ?? ()\n"^M
^done^M
(gdb) ^M
FAIL: gdb.trace/mi-traceframe-changed.exp: tfile: tfind 0 again

So I think the same explanation is valid.

> -------
> Subject: [PATCH] tfile: Don't infer the PC from the tracepoint if the PC is a
>  pseudo-register.
>
> This PC guessing can't work when the PC is a pseudo-register.
> Pseudo-register values don't end up stored in the regcache, they're
> always recomputed.  And, it's actually wrong to try to write a
> pseudo-register with regcache_raw_supply.  Skip it and add a comment.
>
> gdb/
> 2014-01-10  Pedro Alves  <palves@redhat.com>
>
> 	* tracepoint.c (tfile_fetch_registers): Don't infer the PC from
> 	the tracepoint if the PC is a pseudo-register.
> ---
>  gdb/tracepoint.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
> index 582c284..4b47282 100644
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -5060,7 +5060,17 @@ tfile_fetch_registers (struct target_ops *ops,
>    /* We can often usefully guess that the PC is going to be the same
>       as the address of the tracepoint.  */
>    pc_regno = gdbarch_pc_regnum (gdbarch);
> -  if (pc_regno >= 0 && (regno == -1 || regno == pc_regno))
> +
> +  /* 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.  */
> +  if (pc_regno < 0 || pc_regno >= gdbarch_num_regs (gdbarch))
> +    return;
> +
> +  if (regno == -1 || regno == pc_regno)
>      {
>        struct tracepoint *tp = get_tracepoint (tracepoint_number);
>
> -- 
> 1.7.11.7

-- 
Sergio