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

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

[Kevin Buettner]

The patch below fixes 119 failures for the default sh-elf multilib (using
sh-sim.)  I can post a list if there's interest in seeing them.

Comments?  Is there a better way to test for the existence of a register?

Kevin

	* 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 00:31:31 -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);
+      const char *fpscr_name = gdbarch_register_name (gdbarch, FPSCR_REGNUM);
+
+      if (fpscr_name && fpscr_name[0])
+	fpscr = get_frame_register_unsigned (this_frame, FPSCR_REGNUM);
+      else
+	fpscr = 0;
       sh_analyze_prologue (gdbarch, cache->pc, current_pc, cache, fpscr);
     }
 

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

[Joel Brobecker]

> 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.

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

-- 
Joel

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

[Kevin Buettner]

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

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

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 832 bytes --]

Hi!

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.)

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?

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?)


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

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

[Joel Brobecker]

> 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.

FWIW, I don't know if it is *the* canonical approach (or if there is
any), but it seems like a very reasonable one.

-- 
Joel

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

[Kevin Buettner]

On Thu, 1 Mar 2012 07:56:45 -0800
Joel Brobecker <brobecker@adacore.com> wrote:

> > 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.
> 
> FWIW, I don't know if it is *the* canonical approach (or if there is
> any), but it seems like a very reasonable one.

Thanks for looking it over.

I'll go with this approach then, unless there are objections.

Kevin

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

[Kevin Buettner]

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

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

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 1397 bytes --]

Hi Kevin!

On Thu, 1 Mar 2012 11:19:26 -0700, Kevin Buettner <kevinb@redhat.com> wrote:
> I think it is still okay because [...]

I agree with your reasoning.


> On Thu, 01 Mar 2012 11:30:13 +0100
> Thomas Schwinge <thomas@codesourcery.com> wrote:
> > 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.

Sure, but a person adding a new variant of sh_register_reggroup_p would
surely be checking all places where the current only implementation is
invoked, and decide which is now applicable, the old or the new -- or
simply defer that decision to gdbarch_register_reggroup_p then; so we
might as well do that right now.


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

Looks good; before committing, you may want to unify spaces/tabs usage
used for indenting the comment.


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

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

[Kevin Buettner]

On Fri, 02 Mar 2012 10:23:47 +0100
Thomas Schwinge <thomas@codesourcery.com> wrote:

> > 	* sh-tdep.c (sh_frame_cache): Don't fetch the FPSCR register
> > 	unless it exists for this architecture.
> 
> Looks good; before committing, you may want to unify spaces/tabs usage
> used for indenting the comment.

Thanks for noticing that.  I've fixed it in my tree.  (Don't think
it's worth posting a new patch for it.)

Kevin

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

[Kevin Buettner]

On Thu, 1 Mar 2012 11:19:26 -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.

Committed.

Thanks to Joel and Thomas for looking it over.

Kevin