[SH] PATCH: Define the register groups

[SH] PATCH: Define the register groups

[Andrew STUBBS]

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

Hi all,

The SH port does not currently define any register groups. Instead, it 
just relies on the default in reggroup.c. This is not quite adequate as 
it is not always clear from the type where a register should naturally 
sit. In particular the 'fv' vector registers are classified as general, 
not float or vector.

The attached patch adds a function to do the register classification.

It does not know about all the registers of sh2a or any of the dsp 
variants (because I don't), but should not make the situation any worse 
for those.

Andrew Stubbs

[-- Attachment #2: sh_regroups.patch --]
[-- Type: text/plain, Size: 2572 bytes --]

2005-10-27  Andrew Stubbs  <andrew.stubbs@st.com>

	* sh-tdep.c (sh_register_reggroup_p): New function.
	(sh_gdbarch_init): Add call to set_gdbarch_register_reggroup_p.

Index: src/gdb/sh-tdep.c
===================================================================
--- src.orig/gdb/sh-tdep.c	2005-10-17 14:06:03.000000000 +0100
+++ src/gdb/sh-tdep.c	2005-10-27 17:37:00.000000000 +0100
@@ -44,6 +44,7 @@
 #include "regcache.h"
 #include "doublest.h"
 #include "osabi.h"
+#include "reggroups.h"
 
 #include "sh-tdep.h"
 
@@ -1812,6 +1813,51 @@ sh_default_register_type (struct gdbarch
   return builtin_type_int;
 }
 
+/* Is a register in a reggroup?
+   The default code in reggroup.c doesn't identify system registers, some
+   float registers or any of the vector registers.
+   TODO: sh2a and dsp registers.  */
+int
+sh_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
+			struct reggroup *reggroup)
+{
+  if (REGISTER_NAME (regnum) == NULL
+      || *REGISTER_NAME (regnum) == '\0')
+    return 0;
+  if (reggroup == all_reggroup)
+    return 1;
+  if (reggroup == save_reggroup || reggroup == restore_reggroup)
+    return regnum < NUM_REGS;  /* i.e. not pseudo regs */
+
+  if (FP0_REGNUM != -1
+      && ((regnum >= FP0_REGNUM && regnum <= FP_LAST_REGNUM)
+	  || (regnum >= DR0_REGNUM && regnum <= DR_LAST_REGNUM)
+	  || (regnum >= FV0_REGNUM && regnum <= FV_LAST_REGNUM)
+	  || (regnum == FPUL_REGNUM)
+	  || (regnum == FPSCR_REGNUM)))
+    {
+      if (reggroup == float_reggroup)
+	return 1;
+      if (regnum >= FV0_REGNUM && regnum <= FV_LAST_REGNUM
+	  && reggroup == vector_reggroup)
+	return 1;
+    }
+  else if (reggroup == system_reggroup
+	   && (regnum == PC_REGNUM
+               || regnum == PR_REGNUM
+               || regnum == GBR_REGNUM
+               || regnum == VBR_REGNUM
+	       || regnum == SR_REGNUM
+	       || regnum == FPSCR_REGNUM
+	       || regnum == SSR_REGNUM
+	       || regnum == SPC_REGNUM))
+    return 1;
+  else if (reggroup == general_reggroup)
+    return 1;
+
+  return 0;
+}
+
 /* On the sh4, the DRi pseudo registers are problematic if the target
    is little endian. When the user writes one of those registers, for
    instance with 'ser var $dr0=1', we want the double to be stored
@@ -2549,6 +2595,7 @@ sh_gdbarch_init (struct gdbarch_info inf
   set_gdbarch_num_pseudo_regs (gdbarch, 0);
 
   set_gdbarch_register_type (gdbarch, sh_default_register_type);
+  set_gdbarch_register_reggroup_p (gdbarch, sh_register_reggroup_p);
 
   set_gdbarch_print_registers_info (gdbarch, sh_print_registers_info);
 

Re: [SH] PATCH: Define the register groups

[Andrew STUBBS]

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

Andrew Stubbs wrote:
> Hi all,
> 
> The SH port does not currently define any register groups. Instead, it 
> just relies on the default in reggroup.c. This is not quite adequate as 
> it is not always clear from the type where a register should naturally 
> sit. In particular the 'fv' vector registers are classified as general, 
> not float or vector.
> 
> The attached patch adds a function to do the register classification.
> 
> It does not know about all the registers of sh2a or any of the dsp 
> variants (because I don't), but should not make the situation any worse 
> for those.

Is this patch OK?

I attach an updated patch with the Makefile properly adjusted.

Andrew Stubbs

[-- Attachment #2: sh_reggroups.patch --]
[-- Type: text/plain, Size: 3416 bytes --]

2005-11-07  Andrew Stubbs  <andrew.stubbs@st.com>

	* sh-tdep.c: Include reggroups.h.
	(sh_register_reggroup_p): New function.
	(sh_gdbarch_init): Add call to set_gdbarch_register_reggroup_p.
	* Makefile.in (sh-tdep.o): Add dependency on reggroups.h.

Index: src/gdb/sh-tdep.c
===================================================================
--- src.orig/gdb/sh-tdep.c	2005-11-07 11:50:24.000000000 +0000
+++ src/gdb/sh-tdep.c	2005-11-07 14:57:31.000000000 +0000
@@ -44,6 +44,7 @@
 #include "regcache.h"
 #include "doublest.h"
 #include "osabi.h"
+#include "reggroups.h"
 
 #include "sh-tdep.h"
 
@@ -1812,6 +1813,51 @@ sh_default_register_type (struct gdbarch
   return builtin_type_int;
 }
 
+/* Is a register in a reggroup?
+   The default code in reggroup.c doesn't identify system registers, some
+   float registers or any of the vector registers.
+   TODO: sh2a and dsp registers.  */
+int
+sh_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
+			struct reggroup *reggroup)
+{
+  if (REGISTER_NAME (regnum) == NULL
+      || *REGISTER_NAME (regnum) == '\0')
+    return 0;
+  if (reggroup == all_reggroup)
+    return 1;
+  if (reggroup == save_reggroup || reggroup == restore_reggroup)
+    return regnum < NUM_REGS;  /* i.e. not pseudo regs */
+
+  if (FP0_REGNUM != -1
+      && ((regnum >= FP0_REGNUM && regnum <= FP_LAST_REGNUM)
+	  || (regnum >= DR0_REGNUM && regnum <= DR_LAST_REGNUM)
+	  || (regnum >= FV0_REGNUM && regnum <= FV_LAST_REGNUM)
+	  || (regnum == FPUL_REGNUM)
+	  || (regnum == FPSCR_REGNUM)))
+    {
+      if (reggroup == float_reggroup)
+	return 1;
+      if (regnum >= FV0_REGNUM && regnum <= FV_LAST_REGNUM
+	  && reggroup == vector_reggroup)
+	return 1;
+    }
+  else if (reggroup == system_reggroup
+	   && (regnum == PC_REGNUM
+               || regnum == PR_REGNUM
+               || regnum == GBR_REGNUM
+               || regnum == VBR_REGNUM
+	       || regnum == SR_REGNUM
+	       || regnum == FPSCR_REGNUM
+	       || regnum == SSR_REGNUM
+	       || regnum == SPC_REGNUM))
+    return 1;
+  else if (reggroup == general_reggroup)
+    return 1;
+
+  return 0;
+}
+
 /* On the sh4, the DRi pseudo registers are problematic if the target
    is little endian. When the user writes one of those registers, for
    instance with 'ser var $dr0=1', we want the double to be stored
@@ -2549,6 +2595,7 @@ sh_gdbarch_init (struct gdbarch_info inf
   set_gdbarch_num_pseudo_regs (gdbarch, 0);
 
   set_gdbarch_register_type (gdbarch, sh_default_register_type);
+  set_gdbarch_register_reggroup_p (gdbarch, sh_register_reggroup_p);
 
   set_gdbarch_print_registers_info (gdbarch, sh_print_registers_info);
 
Index: src/gdb/Makefile.in
===================================================================
--- src.orig/gdb/Makefile.in	2005-11-07 14:50:50.000000000 +0000
+++ src/gdb/Makefile.in	2005-11-07 14:58:39.000000000 +0000
@@ -2535,7 +2535,7 @@ sh-tdep.o: sh-tdep.c $(defs_h) $(frame_h
 	$(value_h) $(dis_asm_h) $(inferior_h) $(gdb_string_h) \
 	$(gdb_assert_h) $(arch_utils_h) $(floatformat_h) $(regcache_h) \
 	$(doublest_h) $(osabi_h) $(sh_tdep_h) $(elf_bfd_h) $(solib_svr4_h) \
-	$(elf_sh_h) $(gdb_sim_sh_h)
+	$(elf_sh_h) $(gdb_sim_sh_h) $(reggroups_h)
 solib-aix5.o: solib-aix5.c $(defs_h) $(gdb_string_h) $(elf_external_h) \
 	$(symtab_h) $(bfd_h) $(symfile_h) $(objfiles_h) $(gdbcore_h) \
 	$(command_h) $(target_h) $(frame_h) $(gdb_regex_h) $(inferior_h) \

Re: [SH] PATCH: Define the register groups

[Andrew STUBBS]

Ping! This was first posted in October and then in November. Nobody has 
ever said anything about it. I don't think there are any SH specific 
maintainers.

Andrew Stubbs wrote:
> Andrew Stubbs wrote:
> 
>> Hi all,
>>
>> The SH port does not currently define any register groups. Instead, it 
>> just relies on the default in reggroup.c. This is not quite adequate 
>> as it is not always clear from the type where a register should 
>> naturally sit. In particular the 'fv' vector registers are classified 
>> as general, not float or vector.
>>
>> The attached patch adds a function to do the register classification.
>>
>> It does not know about all the registers of sh2a or any of the dsp 
>> variants (because I don't), but should not make the situation any 
>> worse for those.
> 
> 
> Is this patch OK?
> 
> I attach an updated patch with the Makefile properly adjusted.
> 
> Andrew Stubbs
> 
> 
> ------------------------------------------------------------------------
> 
> 2005-11-07  Andrew Stubbs  <andrew.stubbs@st.com>
> 
> 	* sh-tdep.c: Include reggroups.h.
> 	(sh_register_reggroup_p): New function.
> 	(sh_gdbarch_init): Add call to set_gdbarch_register_reggroup_p.
> 	* Makefile.in (sh-tdep.o): Add dependency on reggroups.h.
> 
> Index: src/gdb/sh-tdep.c
> ===================================================================
> --- src.orig/gdb/sh-tdep.c	2005-11-07 11:50:24.000000000 +0000
> +++ src/gdb/sh-tdep.c	2005-11-07 14:57:31.000000000 +0000
> @@ -44,6 +44,7 @@
>  #include "regcache.h"
>  #include "doublest.h"
>  #include "osabi.h"
> +#include "reggroups.h"
>  
>  #include "sh-tdep.h"
>  
> @@ -1812,6 +1813,51 @@ sh_default_register_type (struct gdbarch
>    return builtin_type_int;
>  }
>  
> +/* Is a register in a reggroup?
> +   The default code in reggroup.c doesn't identify system registers, some
> +   float registers or any of the vector registers.
> +   TODO: sh2a and dsp registers.  */
> +int
> +sh_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
> +			struct reggroup *reggroup)
> +{
> +  if (REGISTER_NAME (regnum) == NULL
> +      || *REGISTER_NAME (regnum) == '\0')
> +    return 0;
> +  if (reggroup == all_reggroup)
> +    return 1;
> +  if (reggroup == save_reggroup || reggroup == restore_reggroup)
> +    return regnum < NUM_REGS;  /* i.e. not pseudo regs */
> +
> +  if (FP0_REGNUM != -1
> +      && ((regnum >= FP0_REGNUM && regnum <= FP_LAST_REGNUM)
> +	  || (regnum >= DR0_REGNUM && regnum <= DR_LAST_REGNUM)
> +	  || (regnum >= FV0_REGNUM && regnum <= FV_LAST_REGNUM)
> +	  || (regnum == FPUL_REGNUM)
> +	  || (regnum == FPSCR_REGNUM)))
> +    {
> +      if (reggroup == float_reggroup)
> +	return 1;
> +      if (regnum >= FV0_REGNUM && regnum <= FV_LAST_REGNUM
> +	  && reggroup == vector_reggroup)
> +	return 1;
> +    }
> +  else if (reggroup == system_reggroup
> +	   && (regnum == PC_REGNUM
> +               || regnum == PR_REGNUM
> +               || regnum == GBR_REGNUM
> +               || regnum == VBR_REGNUM
> +	       || regnum == SR_REGNUM
> +	       || regnum == FPSCR_REGNUM
> +	       || regnum == SSR_REGNUM
> +	       || regnum == SPC_REGNUM))
> +    return 1;
> +  else if (reggroup == general_reggroup)
> +    return 1;
> +
> +  return 0;
> +}
> +
>  /* On the sh4, the DRi pseudo registers are problematic if the target
>     is little endian. When the user writes one of those registers, for
>     instance with 'ser var $dr0=1', we want the double to be stored
> @@ -2549,6 +2595,7 @@ sh_gdbarch_init (struct gdbarch_info inf
>    set_gdbarch_num_pseudo_regs (gdbarch, 0);
>  
>    set_gdbarch_register_type (gdbarch, sh_default_register_type);
> +  set_gdbarch_register_reggroup_p (gdbarch, sh_register_reggroup_p);
>  
>    set_gdbarch_print_registers_info (gdbarch, sh_print_registers_info);
>  
> Index: src/gdb/Makefile.in
> ===================================================================
> --- src.orig/gdb/Makefile.in	2005-11-07 14:50:50.000000000 +0000
> +++ src/gdb/Makefile.in	2005-11-07 14:58:39.000000000 +0000
> @@ -2535,7 +2535,7 @@ sh-tdep.o: sh-tdep.c $(defs_h) $(frame_h
>  	$(value_h) $(dis_asm_h) $(inferior_h) $(gdb_string_h) \
>  	$(gdb_assert_h) $(arch_utils_h) $(floatformat_h) $(regcache_h) \
>  	$(doublest_h) $(osabi_h) $(sh_tdep_h) $(elf_bfd_h) $(solib_svr4_h) \
> -	$(elf_sh_h) $(gdb_sim_sh_h)
> +	$(elf_sh_h) $(gdb_sim_sh_h) $(reggroups_h)
>  solib-aix5.o: solib-aix5.c $(defs_h) $(gdb_string_h) $(elf_external_h) \
>  	$(symtab_h) $(bfd_h) $(symfile_h) $(objfiles_h) $(gdbcore_h) \
>  	$(command_h) $(target_h) $(frame_h) $(gdb_regex_h) $(inferior_h) \

Re: [SH] PATCH: Define the register groups

[Daniel Jacobowitz]

On Mon, Nov 07, 2005 at 03:04:23PM +0000, Andrew STUBBS wrote:
> 	* sh-tdep.c: Include reggroups.h.
> 	(sh_register_reggroup_p): New function.
> 	(sh_gdbarch_init): Add call to set_gdbarch_register_reggroup_p.
> 	* Makefile.in (sh-tdep.o): Add dependency on reggroups.h.

Just a couple of questions for you, only one of which involves changing
anything:

  - Are the fv registers for floating point vectors?  Since you
    put them in both info float and info vector.  I'm guessing that
    they are.  Makes sense.

  - Can we reuse default_register_reggroup_p for most of this, and
    just handle the register numbers that it's likely to get
    wrong?  e.g. we probably do need to handle FPUL here - but
    calling the default will handle dr0 just fine.

  - You've put a bunch of registers in system_reggroup (e.g. pc, pr)
    that other folks don't; but no one besides the TUI uses that
    anyway, so it doesn't really matter.  I wouldn't have included
    those two.  I don't remember what the other ones you listed do,
    so I've got no idea about them :-)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [SH] PATCH: Define the register groups

[Andrew STUBBS]

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

Daniel Jacobowitz wrote:
>   - Are the fv registers for floating point vectors?  Since you
>     put them in both info float and info vector.  I'm guessing that
>     they are.  Makes sense.

Yes.

>   - Can we reuse default_register_reggroup_p for most of this, and
>     just handle the register numbers that it's likely to get
>     wrong?  e.g. we probably do need to handle FPUL here - but
>     calling the default will handle dr0 just fine.

Yes, I hadn't thought of that, but I suppose it can. I attach an 
implementation of this patch. Actually, FPUL is handled fine, but FPSCR 
is not, and, more to the point, none of the vector registers are.

>   - You've put a bunch of registers in system_reggroup (e.g. pc, pr)
>     that other folks don't; but no one besides the TUI uses that
>     anyway, so it doesn't really matter.  I wouldn't have included
>     those two.  I don't remember what the other ones you listed do,
>     so I've got no idea about them :-)

Well, there is, of course, a debate to be had over many of the 
registers. I have included FPSCR, a system register, in the float 
reggroup because (to the initiated) it helps you read the other float 
registers. I'm not really sure what system_reggroup is supposed to be 
for - I imagine sr (status register) that control various features (FPU, 
exceptions etc.) of the CPU is a definite member of this group. Others 
are less clear - how about spc and spr which are like pc and pr for 
exceptions. Incidentally, insight also uses these groups.

The attached patch has pc and pr moved from system to general. I have 
also moved GBR similarly. I had previously classified this as system 
because it is not generally useful (the compiler does not normally 
generate code that uses it), but on reflection it just isn't a system 
register.

At the end of the day the important thing is that 'info registers', 
'info float' and 'info vector' print the right thing.

OK?

Andrew Stubbs




P.S. Your email headers contain:

   Mail-Followup-To: Andrew STUBBS <andrew.stubbs@st.com>,
	  gdb-patches@sources.redhat.com

which means I end up replying to myself! Is this deliberate? It is 
slightly irritating.

[-- Attachment #2: sh_reggroups-2.patch --]
[-- Type: text/plain, Size: 3107 bytes --]

2006-01-23  Andrew Stubbs  <andrew.stubbs@st.com>

	* sh-tdep.c: Include reggroups.h.
	(sh_register_reggroup_p): New function.
	(sh_gdbarch_init): Add call to set_gdbarch_register_reggroup_p.
	* Makefile.in (sh-tdep.o): Add dependency on reggroups.h.

Index: src/gdb/sh-tdep.c
===================================================================
--- src.orig/gdb/sh-tdep.c	2006-01-16 12:44:50.000000000 +0000
+++ src/gdb/sh-tdep.c	2006-01-23 14:42:11.000000000 +0000
@@ -44,6 +44,7 @@
 #include "regcache.h"
 #include "doublest.h"
 #include "osabi.h"
+#include "reggroups.h"
 
 #include "sh-tdep.h"
 
@@ -1812,6 +1813,47 @@ sh_default_register_type (struct gdbarch
   return builtin_type_int;
 }
 
+/* Is a register in a reggroup?
+   The default code in reggroup.c doesn't identify system registers, some
+   float registers or any of the vector registers.
+   TODO: sh2a and dsp registers.  */
+int
+sh_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
+			struct reggroup *reggroup)
+{
+  if (REGISTER_NAME (regnum) == NULL
+      || *REGISTER_NAME (regnum) == '\0')
+    return 0;
+
+  if (reggroup == float_reggroup
+      && (regnum == FPUL_REGNUM
+	  || regnum == FPSCR_REGNUM))
+    return 1;
+
+  if (regnum >= FV0_REGNUM && regnum <= FV_LAST_REGNUM)
+    {
+      if (reggroup == vector_reggroup || reggroup == float_reggroup)
+	return 1;
+      if (reggroup == general_reggroup)
+	return 0;
+    }
+
+  if (regnum == VBR_REGNUM
+      || regnum == SR_REGNUM
+      || regnum == FPSCR_REGNUM
+      || regnum == SSR_REGNUM
+      || regnum == SPC_REGNUM)
+    {
+      if (reggroup == system_reggroup)
+	return 1;
+      if (reggroup == general_reggroup)
+	return 0;
+    }
+
+  /* The default code can cope with any other registers.  */
+  return default_register_reggroup_p (gdbarch, regnum, reggroup);
+}
+
 /* On the sh4, the DRi pseudo registers are problematic if the target
    is little endian. When the user writes one of those registers, for
    instance with 'ser var $dr0=1', we want the double to be stored
@@ -2371,6 +2413,7 @@ sh_gdbarch_init (struct gdbarch_info inf
   set_gdbarch_num_pseudo_regs (gdbarch, 0);
 
   set_gdbarch_register_type (gdbarch, sh_default_register_type);
+  set_gdbarch_register_reggroup_p (gdbarch, sh_register_reggroup_p);
 
   set_gdbarch_breakpoint_from_pc (gdbarch, sh_breakpoint_from_pc);
 
Index: src/gdb/Makefile.in
===================================================================
--- src.orig/gdb/Makefile.in	2006-01-23 12:33:23.000000000 +0000
+++ src/gdb/Makefile.in	2006-01-23 12:39:55.000000000 +0000
@@ -2538,7 +2538,7 @@ sh-tdep.o: sh-tdep.c $(defs_h) $(frame_h
 	$(value_h) $(dis_asm_h) $(inferior_h) $(gdb_string_h) \
 	$(gdb_assert_h) $(arch_utils_h) $(floatformat_h) $(regcache_h) \
 	$(doublest_h) $(osabi_h) $(sh_tdep_h) $(elf_bfd_h) $(solib_svr4_h) \
-	$(elf_sh_h) $(gdb_sim_sh_h)
+	$(elf_sh_h) $(gdb_sim_sh_h) $(reggroups_h)
 sol2-tdep.o: sol2-tdep.c $(defs_h) $(frame_h) $(symtab_h)
 solib-aix5.o: solib-aix5.c $(defs_h) $(gdb_string_h) $(elf_external_h) \
 	$(symtab_h) $(bfd_h) $(symfile_h) $(objfiles_h) $(gdbcore_h) \

Re: [SH] PATCH: Define the register groups

[Daniel Jacobowitz]

On Mon, Jan 23, 2006 at 03:48:44PM +0000, Andrew STUBBS wrote:
> At the end of the day the important thing is that 'info registers', 
> 'info float' and 'info vector' print the right thing.
> 
> OK?

This patch is fine.  Thanks!

> P.S. Your email headers contain:
> 
>   Mail-Followup-To: Andrew STUBBS <andrew.stubbs@st.com>,
> 	  gdb-patches@sources.redhat.com
> 
> which means I end up replying to myself! Is this deliberate? It is 
> slightly irritating.

This is yet another fallout from the fact that M-F-T isn't quite
standard... your mail client is supposed to be smart enough to edit
your own address out of the reply list!  Sorry about that, but I'm not
sure if there's anything I can readily do about it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [SH] PATCH: Define the register groups

[Andrew STUBBS]

Daniel Jacobowitz wrote:
> On Mon, Jan 23, 2006 at 03:48:44PM +0000, Andrew STUBBS wrote:
>> At the end of the day the important thing is that 'info registers', 
>> 'info float' and 'info vector' print the right thing.
>>
>> OK?
> 
> This patch is fine.  Thanks!

Thanks, committed.

>> P.S. Your email headers contain:
>>
>>   Mail-Followup-To: Andrew STUBBS <andrew.stubbs@st.com>,
>> 	  gdb-patches@sources.redhat.com
>>
>> which means I end up replying to myself! Is this deliberate? It is 
>> slightly irritating.
> 
> This is yet another fallout from the fact that M-F-T isn't quite
> standard... your mail client is supposed to be smart enough to edit
> your own address out of the reply list!  Sorry about that, but I'm not
> sure if there's anything I can readily do about it.

I don't suppose signing up as a Mozilla Thunderbird developer, fixing 
the problem and getting it into the next release would be 'readily'? :)