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

[Kevin Buettner <kevinb at redhat dot com>]

On Thu, 01 Mar 2012 11:30:13 +0100
Thomas Schwinge <thomas@codesourcery.com> wrote:

> On Wed, 29 Feb 2012 22:09:32 -0700, Kevin Buettner <kevinb@redhat.com> wrote:
> > 	* sh-tdep.c (sh_frame_cache): Don't fetch the FPSCR register
> > 	unless it exists for this architecture.
> 
> Your reasoning and this patch look good to me.  (But I didn't test it.)
> Basically, you now relay the test to sh_register_reggroup_p, which
> already contains the FPSCR existence test you first proposed.  (The same
> test exists in arch-utils.c:legacy_register_sim_regno, by the way.)

Thanks for looking it over.

Yes, that test in legacy_register_sim_regno bit me recently when doing
the rl78 register rearrangement.

That's an interesting case to consider, actually.  For rl78 (and for
mips too as I recall), there are a certain set of "raw" registers which
are then mapped to "cooked" versions.  The raw registers do not have
names, but the cooked ones do.

So, assuming the same implementation of `register_reggroup_p', asking
about one of the raw registers will fail (return 0), but asking about
a cooked register will succeed.

The reason I'm pondering this is that I'm now wondering about the
suitability of using `register_reggroup_p' for this purpose.

I think it is still okay because presumably the code should be asking
about the cooked version of the register instead of the uncooked
version.  In fact, for rl78, this is how the stack pointer is implemented;
it's a cooked register formed from two raw registers.  It is never correct
for anything aside from the pseudo register read/write code to try to
access the raw versions of the two stack pointer registers.

> Still learning about GDB's code layout -- is there any benefit in
> invoking gdbarch_register_reggroup_p (as you're doing) in contrast to
> directly going for sh_register_reggroup_p?

Well, the way I'm doing it is slower.

But, if at some point someone were to add another
`register_reggroup_p' method for some other sh variant (as is the case
for `register_name'), the code that I wrote will still work.  If I
called sh_register_reggroup_p directly, then presumably the wrong
function would be called for that new architecture variant.

> Also, we assume that simply passing 0 for fpscr to sh_analyze_prologue
> doesn't do any harm (which it doesn't).  (Worth a comment, perhaps?)

Yes, that's a good idea.  Below is 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 18:11:17 -0000
@@ -2553,7 +2553,16 @@ sh_frame_cache (struct frame_info *this_
   if (cache->pc != 0)
     {
       ULONGEST fpscr;
-      fpscr = get_frame_register_unsigned (this_frame, FPSCR_REGNUM);
+
+      /* Check for the existence of the FPSCR register.  If it exists,
+         fetch its value for use in prologue analysis.  Passing a zero
+	 value is the best choice for architecture variants upon which
+	 there's no FPSCR register.  */
+      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);
     }