Re: [RFC] Document replacement for frame_register_read (deprecated).

[Pedro Alves <palves@redhat.com>]

On 10/23/2012 06:19 PM, Tom Tromey wrote:
>>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
> 
> Joel> This is inspired by a discussion from a long time ago:
> Joel> http://www.sourceware.org/ml/gdb-patches/2011-03/msg01095.html
> 
> Joel> If you agree with the patch, we will also have to update the ARI
> Joel> script to mention get_frame_register_value as the alternative.
> 
> Joel> And we might also want to rename the function as well. I don't think
> Joel> we can rely on the ARI alone to avoid new uses of this function.
> Joel> And once new uses are in, it's much harder to make sure we undo them.
> 
> Joel> Thoughts?
> 
> The patch seems reasonable to me.

Me too.

> 
> Renaming seems like a decent idea, since there is a use in jit.c, which
> was added long after this was deprecated.

Yeah.

> I count 22 mentions (including the declaration) of frame_register_read
> in the tree right now.  Getting rid of all of them doesn't seem so
> difficult, aside from the testing.  This would be clearly better...
> 
> At least one of the current uses seems suspicious since it ignores the
> result.  I assume I'm missing some context though.

I took a look too.

> frame.c:775:do_frame_register_read (void *src, int regnum, gdb_byte *buf)
> frame.c:777:  if (!frame_register_read (src, regnum, buf))

Probably innocuous, but need to check if there's a path that calls
this for frames other than #0, or if possible to get here for cores and
tracing (IIRC, this is used for infcalls, which can't work on those cases).
If so, since failure always maps to REG_UNAVAILABLE, we're losing info here.

> frame.c:1218:     frame_register_read (frame, regnum, buf);

A read-modify-write that doesn't take in account whether the read
was successful.  Can't be good.

> i386-tdep.c:1748:         && frame_register_read (this_frame, cache->saved_sp_reg, buf))

I think it's okay.

> infcmd.c:2052:      if (! frame_register_read (frame, i, value_contents_raw (val)))

I guess we should be saying "*optimized out*" if the register is not saved, and
leave "*value not available*" for the case where we can't get at it because it's
missing from the truncated core or from the tracebuffer.

> jit.c:1056:  value->defined = frame_register_read (priv->this_frame, gdb_reg,

No idea.

> mips-tdep.c:4606:  if (!frame_register_read (frame, regno, raw_buffer))
> mips-tdep.c:4643:      if (!frame_register_read (frame, regno, rare_buffer))
> mips-tdep.c:4879:      if (!frame_register_read (frame, regnum, raw_buffer))
> mt-tdep.c:688:    frame_register_read (frame, regnum, buff);
> mt-tdep.c:713:    frame_register_read (frame, MT_COPRO_REGNUM, buf);
> mt-tdep.c:734:    frame_register_read (frame, MT_MAC_REGNUM, buf);
> mt-tdep.c:740:        frame_register_read (frame, MT_EXMAC_REGNUM, buf);
> sh64-tdep.c:2054:  if (!frame_register_read (frame, regnum, raw_buffer))
> sh64-tdep.c:2170:  if (!frame_register_read (frame, regnum, raw_buffer))

All these should be fixed to handle unavailableness gracefully.  It just
hasn't been pressing no target with with arch can do tracepoints,
AFAIK (though there are other ways the registers could be missing).

-- 
Pedro Alves