Re: [RFC] sh-tdep.c: Don't fetch FPSCR register if it doesn't exist

Kevin Buettner
2012-03-01T05:09:54Z

On Wed, 29 Feb 2012 17:33:18 -0800
Joel Brobecker <brobecker@adacore.com> wrote:

> > Comments? Is there a better way to test for the existence of a register?
>
> Perhaps an idea... Apparently, this is a property of the arch, so
> something relatively static. This is typically the type of information
> the sh_gdbarch_init routine would save in the gdbarch_tdep structure.
> You can create a new flag in that structure, and then set it at the
> same time you register the register_name gdbarch hook.

I considered that approach. It'll work, but seemed like more coding
than it was worth when I first wrote the patch. But thinking about
it now, I don't find this to be a very compelling reason to code it
the way that I did.

That said, I'm not fond of the tdep flag idea, at least not in this
instance...

If we were to use a tdep flag, we must ensure that this flag is set
correctly for each architecture variant. For sh, there are a bunch of
`register_name' and `register_type' implementations. It seems like a
shame to have to add redundant knowledge about fpscr to the tdep
struct.

If we were to keep that knowledge together or somehow derive it from
knowledge we already had it'd be different. But in order to determine
the correct value of this new flag, the most obvious course of action
is (for one of us) to examine the register names to see if fpscr is
present. We'll have to transcribe that knowledge to the various cases
in sh_gdbarch_init().

It's still doable, but I'd like to consider another approach first.

I've seen the register name test used in other instances to check for
the existence of a register. One of the places it's used is in
various `register_reggroup_p' implementations. It's used in both
default_register_reggroup_p() and sh_register_reggroup_p().

This suggests the following approach:

if (gdbarch_register_reggroup_p (gdbarch, FPSCR_REGNUM, all_reggroup))
...
else
...

We know that gdbarch_register_reggroup_p() called in that fashion
will somehow determine whether the given register number is a member
of the "all" register group. We don't care how it figures this out,
whether it be by examining the register name, or a flag in the tdep
struct, or some other method altogether. But we do know that it'll
return 1 for registers that exist and 0 for those that don't.

What do you think?

Here's an updated patch:

* sh-tdep.c (sh_frame_cache): Don't fetch the FPSCR register
unless it exists for this architecture.

Index: sh-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sh-tdep.c,v
retrieving revision 1.239
diff -u -p -r1.239 sh-tdep.c
--- sh-tdep.c 27 Feb 2012 16:40:48 -0000 1.239
+++ sh-tdep.c 1 Mar 2012 05:02:47 -0000
@@ -2553,7 +2553,12 @@ sh_frame_cache (struct frame_info *this_
if (cache->pc != 0)
{
ULONGEST fpscr;
- fpscr = get_frame_register_unsigned (this_frame, FPSCR_REGNUM);
+
+ /* Check existence of FPSCR. */
+ if (gdbarch_register_reggroup_p (gdbarch, FPSCR_REGNUM, all_reggroup))
+ fpscr = get_frame_register_unsigned (this_frame, FPSCR_REGNUM);
+ else
+ fpscr = 0;
sh_analyze_prologue (gdbarch, cache->pc, current_pc, cache, fpscr);
}