[patch, arm] Consistent display of registers in corefile

[patch, arm] Consistent display of registers in corefile

[Yao Qi]

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

GDB trunk has a test failure on ARM,

   FAIL: gdb.base/gcore.exp: corefile restored general registers

In short, this failure is caused by output of 'info registers' before
coredump doesn't match output of 'info registers' when corefole is
loaded again, there are mainly two differences, [1] and [2].

Output before coredump,
r0             0x12008  73736^M
r1             0xbea1f0c0       -1096683328^M
[...]
sp             0xbea1f0a4       0xbea1f0a4^M
lr             0x849b   33947^M
pc             0x83fc   0x83fc <terminal_func+4>^M
cpsr           0x20000030       536870960^M

Output when corefile is loaded,
r0             0x12008  73736^M
r1             0xbea1f0c0       3198283968^M  // <---- [1]
[...]
sp             0xbea1f0a4       0xbea1f0a4^M
lr             0x849b   33947^M
pc             0x83fc   0x83fc <terminal_func+4>^M
fps            0x727a622f       1920623151^M  // <---- [2]
cpsr           0x20000030       536870960^M

The difference [1] is caused by different register types, uint32 vs.
int32.  In tdesc, the type of general register is "int", while in
arm_register_type, it is regarded as builtin_uint32.  This can be fixed
when register type is handled in a consistent way (in reg_type.patch).

The difference [2] is about displaying "fps" in output of "info
registers".  In default_register_reggroup_p, the group of register is
determined by the type of register, which is not very precise.  FPS
should be in float group, but its type is INT.  This can be fixed by
defining ARM's own register_reggroup_p to override
default_register_reggroup_p (in arm_fps_group.patch).

Regression tested with combination of
"\{-mthumb,-marm\}\{-fstack-protector,-fno-stack-protector}\{-march=armv7-a,-march=armv5t\}".

OK for mainline?

-- 
Yao (齐尧)

[-- Attachment #2: reg_type.patch --]
[-- Type: text/x-patch, Size: 610 bytes --]

gdb/
	* arm-tdep.c (arm_register_type): Return builtin_int32 to be
	consistent with types in tdesc.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 0f38b6b..53d7c1c 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -2966,7 +2966,8 @@ arm_register_type (struct gdbarch *gdbarch, int regnum)
        an XML description.  */
     return builtin_type (gdbarch)->builtin_int0;
   else
-    return builtin_type (gdbarch)->builtin_uint32;
+    /* To be consistent with types in tdesc.  */
+    return builtin_type (gdbarch)->builtin_int32;
 }
 
 /* Map a DWARF register REGNUM onto the appropriate GDB register

[-- Attachment #3: arm_fps_group.patch --]
[-- Type: text/x-patch, Size: 1418 bytes --]

gdb/
	* arm-tdep.c (arm_register_reggroup_p): New.
	(arm_gdbarch_init): Set arm_register_reggroup_p for hook
	register_reggroup_p.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 678d44c..21ad876 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -29,6 +29,7 @@
 #include "gdb_string.h"
 #include "dis-asm.h"		/* For register styles. */
 #include "regcache.h"
+#include "reggroups.h"
 #include "doublest.h"
 #include "value.h"
 #include "arch-utils.h"
@@ -7191,6 +7192,17 @@ arm_elf_osabi_sniffer (bfd *abfd)
   return osabi;
 }
 
+static int
+arm_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
+			  struct reggroup *group)
+{
+  /* FPS register's type is INT, but belongs to float_group.  */
+  if (regnum == ARM_FPS_REGNUM)
+    return (group == float_reggroup);
+  else
+    return default_register_reggroup_p (gdbarch, regnum, group);
+}
+
 \f
 /* Initialize the current architecture based on INFO.  If possible,
    re-use an architecture from ARCHES, which is a list of
@@ -7655,6 +7667,7 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_pc_regnum (gdbarch, ARM_PC_REGNUM);
   set_gdbarch_num_regs (gdbarch, ARM_NUM_REGS);
   set_gdbarch_register_type (gdbarch, arm_register_type);
+  set_gdbarch_register_reggroup_p (gdbarch, arm_register_reggroup_p);
 
   /* This "info float" is FPA-specific.  Use the generic version if we
      do not have FPA.  */

Re: [patch, arm] Consistent display of registers in corefile

[Mark Kettenis]

> Date: Fri, 10 Dec 2010 21:37:30 +0800
> From: Yao Qi <yao@codesourcery.com>
> 
> GDB trunk has a test failure on ARM,
> 
>    FAIL: gdb.base/gcore.exp: corefile restored general registers
> 
> In short, this failure is caused by output of 'info registers' before
> coredump doesn't match output of 'info registers' when corefole is
> loaded again, there are mainly two differences, [1] and [2].
> 
> Output before coredump,
> r0             0x12008  73736^M
> r1             0xbea1f0c0       -1096683328^M
> [...]
> sp             0xbea1f0a4       0xbea1f0a4^M
> lr             0x849b   33947^M
> pc             0x83fc   0x83fc <terminal_func+4>^M
> cpsr           0x20000030       536870960^M
> 
> Output when corefile is loaded,
> r0             0x12008  73736^M
> r1             0xbea1f0c0       3198283968^M  // <---- [1]
> [...]
> sp             0xbea1f0a4       0xbea1f0a4^M
> lr             0x849b   33947^M
> pc             0x83fc   0x83fc <terminal_func+4>^M
> fps            0x727a622f       1920623151^M  // <---- [2]
> cpsr           0x20000030       536870960^M
> 
> The difference [1] is caused by different register types, uint32 vs.
> int32.  In tdesc, the type of general register is "int", while in
> arm_register_type, it is regarded as builtin_uint32.  This can be fixed
> when register type is handled in a consistent way (in reg_type.patch).
> 
> The difference [2] is about displaying "fps" in output of "info
> registers".  In default_register_reggroup_p, the group of register is
> determined by the type of register, which is not very precise.  FPS
> should be in float group, but its type is INT.  This can be fixed by
> defining ARM's own register_reggroup_p to override
> default_register_reggroup_p (in arm_fps_group.patch).
> 
> Regression tested with combination of
> "\{-mthumb,-marm\}\{-fstack-protector,-fno-stack-protector}\{-march=armv7-a,-march=armv5t\}".
> 
> OK for mainline?

I would suspect that the proper thing to do would be to align the
tdesc with the code instead of the other way around.  The arm-core.xml
file seems to underspecify things by omitting the type=xxx clause on
many registers.  Whoever wrote arm_register_type() at least had to
make a conscious decision about the signedness of the type used for
the general purpose registers.

Re: [patch, arm] Consistent display of registers in corefile

[Yao Qi]

On 12/10/2010 10:43 PM, Mark Kettenis wrote:
>> Date: Fri, 10 Dec 2010 21:37:30 +0800
>> From: Yao Qi <yao@codesourcery.com>
>>
>> GDB trunk has a test failure on ARM,
>>
>>    FAIL: gdb.base/gcore.exp: corefile restored general registers
>>
>> In short, this failure is caused by output of 'info registers' before
>> coredump doesn't match output of 'info registers' when corefole is
>> loaded again, there are mainly two differences, [1] and [2].
>>
>> Output before coredump,
>> r0             0x12008  73736^M
>> r1             0xbea1f0c0       -1096683328^M
>> [...]
>> sp             0xbea1f0a4       0xbea1f0a4^M
>> lr             0x849b   33947^M
>> pc             0x83fc   0x83fc <terminal_func+4>^M
>> cpsr           0x20000030       536870960^M
>>
>> Output when corefile is loaded,
>> r0             0x12008  73736^M
>> r1             0xbea1f0c0       3198283968^M  // <---- [1]
>> [...]
>> sp             0xbea1f0a4       0xbea1f0a4^M
>> lr             0x849b   33947^M
>> pc             0x83fc   0x83fc <terminal_func+4>^M
>> fps            0x727a622f       1920623151^M  // <---- [2]
>> cpsr           0x20000030       536870960^M
>>
>> The difference [1] is caused by different register types, uint32 vs.
>> int32.  In tdesc, the type of general register is "int", while in
>> arm_register_type, it is regarded as builtin_uint32.  This can be fixed
>> when register type is handled in a consistent way (in reg_type.patch).
> 
> I would suspect that the proper thing to do would be to align the
> tdesc with the code instead of the other way around.  The arm-core.xml
> file seems to underspecify things by omitting the type=xxx clause on
> many registers.  Whoever wrote arm_register_type() at least had to
> make a conscious decision about the signedness of the type used for
> the general purpose registers.

I prefer unsigned for general purpose registers.  Any objections?  If we
agree on this, I'll add type="uint32" to r0-r12 in arm-core.xml.

-- 
Yao (齐尧)

Re: [patch, arm] Consistent display of registers in corefile

[Daniel Jacobowitz]

On Fri, Dec 10, 2010 at 03:43:15PM +0100, Mark Kettenis wrote:
> I would suspect that the proper thing to do would be to align the
> tdesc with the code instead of the other way around.  The arm-core.xml
> file seems to underspecify things by omitting the type=xxx clause on
> many registers.  Whoever wrote arm_register_type() at least had to
> make a conscious decision about the signedness of the type used for
> the general purpose registers.

Yeah, I agree.  It was probably my mistake.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch, arm] Consistent display of registers in corefile

[Yao Qi]

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

On 12/13/2010 10:57 AM, Daniel Jacobowitz wrote:
> On Fri, Dec 10, 2010 at 03:43:15PM +0100, Mark Kettenis wrote:
>> I would suspect that the proper thing to do would be to align the
>> tdesc with the code instead of the other way around.  The arm-core.xml
>> file seems to underspecify things by omitting the type=xxx clause on
>> many registers.  Whoever wrote arm_register_type() at least had to
>> make a conscious decision about the signedness of the type used for
>> the general purpose registers.
> 
> Yeah, I agree.  It was probably my mistake.
> 

In this new patch, 'type="uint32"' is added for registers from r0 to r12
except r11.  r11 is 'type="data_ptr"'.  features/arm*.c files are
regenerated by Makefile.  Regression tested along with the other patch
arm_fps_group.patch on armv7l-unknown-linux-gnueabi, "corefile restored
general registers" failure in gdb.base/gcore.exp goes away.  Is it OK
for GDB mainline?

-- 
Yao (齐尧)

[-- Attachment #2: feature.patch --]
[-- Type: text/x-patch, Size: 11541 bytes --]

gdb/
	* features/arm-core.xml: Add attribute type to reg from r0 to r12.
	* features/arm-with-iwmmxt.c: Regenerate.
	* features/arm-with-neon.c: Regenerate.
	* features/arm-with-vfpv2.c: Regenerate.

diff --git a/gdb/features/arm-core.xml b/gdb/features/arm-core.xml
index 1624901..97260a8 100644
--- a/gdb/features/arm-core.xml
+++ b/gdb/features/arm-core.xml
@@ -7,19 +7,19 @@
 
 <!DOCTYPE feature SYSTEM "gdb-target.dtd">
 <feature name="org.gnu.gdb.arm.core">
-  <reg name="r0" bitsize="32"/>
-  <reg name="r1" bitsize="32"/>
-  <reg name="r2" bitsize="32"/>
-  <reg name="r3" bitsize="32"/>
-  <reg name="r4" bitsize="32"/>
-  <reg name="r5" bitsize="32"/>
-  <reg name="r6" bitsize="32"/>
-  <reg name="r7" bitsize="32"/>
-  <reg name="r8" bitsize="32"/>
-  <reg name="r9" bitsize="32"/>
-  <reg name="r10" bitsize="32"/>
-  <reg name="r11" bitsize="32"/>
-  <reg name="r12" bitsize="32"/>
+  <reg name="r0" bitsize="32" type="uint32"/>
+  <reg name="r1" bitsize="32" type="uint32"/>
+  <reg name="r2" bitsize="32" type="uint32"/>
+  <reg name="r3" bitsize="32" type="uint32"/>
+  <reg name="r4" bitsize="32" type="uint32"/>
+  <reg name="r5" bitsize="32" type="uint32"/>
+  <reg name="r6" bitsize="32" type="uint32"/>
+  <reg name="r7" bitsize="32" type="uint32"/>
+  <reg name="r8" bitsize="32" type="uint32"/>
+  <reg name="r9" bitsize="32" type="uint32"/>
+  <reg name="r10" bitsize="32" type="uint32"/>
+  <reg name="r11" bitsize="32" type="data_ptr"/>
+  <reg name="r12" bitsize="32" type="uint32"/>
   <reg name="sp" bitsize="32" type="data_ptr"/>
   <reg name="lr" bitsize="32"/>
   <reg name="pc" bitsize="32" type="code_ptr"/>
diff --git a/gdb/features/arm-with-iwmmxt.c b/gdb/features/arm-with-iwmmxt.c
index 71bd364..f3918bb 100644
--- a/gdb/features/arm-with-iwmmxt.c
+++ b/gdb/features/arm-with-iwmmxt.c
@@ -1,6 +1,7 @@
 /* THIS FILE IS GENERATED.  Original: arm-with-iwmmxt.xml */
 
 #include "defs.h"
+#include "osabi.h"
 #include "target-descriptions.h"
 
 struct target_desc *tdesc_arm_with_iwmmxt;
@@ -14,19 +15,19 @@ initialize_tdesc_arm_with_iwmmxt (void)
   set_tdesc_architecture (result, bfd_scan_arch ("iwmmxt"));
 
   feature = tdesc_create_feature (result, "org.gnu.gdb.arm.core");
-  tdesc_create_reg (feature, "r0", 0, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r1", 1, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r2", 2, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r3", 3, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r4", 4, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r5", 5, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r6", 6, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r7", 7, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r8", 8, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r9", 9, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r10", 10, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r11", 11, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r12", 12, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r0", 0, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r1", 1, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r2", 2, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r3", 3, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r4", 4, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r5", 5, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r6", 6, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r7", 7, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r8", 8, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r9", 9, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r10", 10, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r11", 11, 1, NULL, 32, "data_ptr");
+  tdesc_create_reg (feature, "r12", 12, 1, NULL, 32, "uint32");
   tdesc_create_reg (feature, "sp", 13, 1, NULL, 32, "data_ptr");
   tdesc_create_reg (feature, "lr", 14, 1, NULL, 32, "int");
   tdesc_create_reg (feature, "pc", 15, 1, NULL, 32, "code_ptr");
diff --git a/gdb/features/arm-with-neon.c b/gdb/features/arm-with-neon.c
index d669d08..d63c399 100644
--- a/gdb/features/arm-with-neon.c
+++ b/gdb/features/arm-with-neon.c
@@ -1,7 +1,7 @@
 /* THIS FILE IS GENERATED.  Original: arm-with-neon.xml */
 
 #include "defs.h"
-#include "gdbtypes.h"
+#include "osabi.h"
 #include "target-descriptions.h"
 
 struct target_desc *tdesc_arm_with_neon;
@@ -10,22 +10,22 @@ initialize_tdesc_arm_with_neon (void)
 {
   struct target_desc *result = allocate_target_description ();
   struct tdesc_feature *feature;
-  struct type *field_type, *type;
+  struct tdesc_type *field_type, *type;
 
   feature = tdesc_create_feature (result, "org.gnu.gdb.arm.core");
-  tdesc_create_reg (feature, "r0", 0, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r1", 1, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r2", 2, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r3", 3, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r4", 4, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r5", 5, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r6", 6, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r7", 7, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r8", 8, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r9", 9, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r10", 10, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r11", 11, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r12", 12, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r0", 0, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r1", 1, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r2", 2, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r3", 3, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r4", 4, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r5", 5, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r6", 6, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r7", 7, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r8", 8, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r9", 9, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r10", 10, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r11", 11, 1, NULL, 32, "data_ptr");
+  tdesc_create_reg (feature, "r12", 12, 1, NULL, 32, "uint32");
   tdesc_create_reg (feature, "sp", 13, 1, NULL, 32, "data_ptr");
   tdesc_create_reg (feature, "lr", 14, 1, NULL, 32, "int");
   tdesc_create_reg (feature, "pc", 15, 1, NULL, 32, "code_ptr");
diff --git a/gdb/features/arm-with-vfpv2.c b/gdb/features/arm-with-vfpv2.c
index 687e140..bab1899 100644
--- a/gdb/features/arm-with-vfpv2.c
+++ b/gdb/features/arm-with-vfpv2.c
@@ -1,7 +1,7 @@
 /* THIS FILE IS GENERATED.  Original: arm-with-vfpv2.xml */
 
 #include "defs.h"
-#include "gdbtypes.h"
+#include "osabi.h"
 #include "target-descriptions.h"
 
 struct target_desc *tdesc_arm_with_vfpv2;
@@ -10,22 +10,22 @@ initialize_tdesc_arm_with_vfpv2 (void)
 {
   struct target_desc *result = allocate_target_description ();
   struct tdesc_feature *feature;
-  struct type *field_type, *type;
+  struct tdesc_type *field_type, *type;
 
   feature = tdesc_create_feature (result, "org.gnu.gdb.arm.core");
-  tdesc_create_reg (feature, "r0", 0, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r1", 1, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r2", 2, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r3", 3, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r4", 4, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r5", 5, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r6", 6, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r7", 7, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r8", 8, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r9", 9, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r10", 10, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r11", 11, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r12", 12, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r0", 0, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r1", 1, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r2", 2, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r3", 3, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r4", 4, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r5", 5, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r6", 6, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r7", 7, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r8", 8, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r9", 9, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r10", 10, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r11", 11, 1, NULL, 32, "data_ptr");
+  tdesc_create_reg (feature, "r12", 12, 1, NULL, 32, "uint32");
   tdesc_create_reg (feature, "sp", 13, 1, NULL, 32, "data_ptr");
   tdesc_create_reg (feature, "lr", 14, 1, NULL, 32, "int");
   tdesc_create_reg (feature, "pc", 15, 1, NULL, 32, "code_ptr");
diff --git a/gdb/features/arm-with-vfpv3.c b/gdb/features/arm-with-vfpv3.c
index efb851d..06ebdda 100644
--- a/gdb/features/arm-with-vfpv3.c
+++ b/gdb/features/arm-with-vfpv3.c
@@ -1,7 +1,7 @@
 /* THIS FILE IS GENERATED.  Original: arm-with-vfpv3.xml */
 
 #include "defs.h"
-#include "gdbtypes.h"
+#include "osabi.h"
 #include "target-descriptions.h"
 
 struct target_desc *tdesc_arm_with_vfpv3;
@@ -10,22 +10,22 @@ initialize_tdesc_arm_with_vfpv3 (void)
 {
   struct target_desc *result = allocate_target_description ();
   struct tdesc_feature *feature;
-  struct type *field_type, *type;
+  struct tdesc_type *field_type, *type;
 
   feature = tdesc_create_feature (result, "org.gnu.gdb.arm.core");
-  tdesc_create_reg (feature, "r0", 0, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r1", 1, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r2", 2, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r3", 3, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r4", 4, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r5", 5, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r6", 6, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r7", 7, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r8", 8, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r9", 9, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r10", 10, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r11", 11, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "r12", 12, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "r0", 0, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r1", 1, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r2", 2, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r3", 3, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r4", 4, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r5", 5, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r6", 6, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r7", 7, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r8", 8, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r9", 9, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r10", 10, 1, NULL, 32, "uint32");
+  tdesc_create_reg (feature, "r11", 11, 1, NULL, 32, "data_ptr");
+  tdesc_create_reg (feature, "r12", 12, 1, NULL, 32, "uint32");
   tdesc_create_reg (feature, "sp", 13, 1, NULL, 32, "data_ptr");
   tdesc_create_reg (feature, "lr", 14, 1, NULL, 32, "int");
   tdesc_create_reg (feature, "pc", 15, 1, NULL, 32, "code_ptr");

Re: [patch, arm] Consistent display of registers in corefile

[Daniel Jacobowitz]

On Mon, Dec 13, 2010 at 06:00:32PM +0800, Yao Qi wrote:
> On 12/13/2010 10:57 AM, Daniel Jacobowitz wrote:
> > On Fri, Dec 10, 2010 at 03:43:15PM +0100, Mark Kettenis wrote:
> >> I would suspect that the proper thing to do would be to align the
> >> tdesc with the code instead of the other way around.  The arm-core.xml
> >> file seems to underspecify things by omitting the type=xxx clause on
> >> many registers.  Whoever wrote arm_register_type() at least had to
> >> make a conscious decision about the signedness of the type used for
> >> the general purpose registers.
> > 
> > Yeah, I agree.  It was probably my mistake.
> > 
> 
> In this new patch, 'type="uint32"' is added for registers from r0 to r12
> except r11.  r11 is 'type="data_ptr"'.  features/arm*.c files are
> regenerated by Makefile.  Regression tested along with the other patch
> arm_fps_group.patch on armv7l-unknown-linux-gnueabi, "corefile restored
> general registers" failure in gdb.base/gcore.exp goes away.  Is it OK
> for GDB mainline?

Please use uint32 for r11 also.  It's sometimes the frame pointer, but
that's not required and it may have any arbitrary data in it.

Otherwise OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch, arm] Consistent display of registers in corefile

[Yao Qi]

On 12/20/2010 02:23 AM, Daniel Jacobowitz wrote:
> On Mon, Dec 13, 2010 at 06:00:32PM +0800, Yao Qi wrote:
>> On 12/13/2010 10:57 AM, Daniel Jacobowitz wrote:
>>> On Fri, Dec 10, 2010 at 03:43:15PM +0100, Mark Kettenis wrote:
>>>> I would suspect that the proper thing to do would be to align the
>>>> tdesc with the code instead of the other way around.  The arm-core.xml
>>>> file seems to underspecify things by omitting the type=xxx clause on
>>>> many registers.  Whoever wrote arm_register_type() at least had to
>>>> make a conscious decision about the signedness of the type used for
>>>> the general purpose registers.
>>>
>>> Yeah, I agree.  It was probably my mistake.
>>>
>>
>> In this new patch, 'type="uint32"' is added for registers from r0 to r12
>> except r11.  r11 is 'type="data_ptr"'.  features/arm*.c files are
>> regenerated by Makefile.  Regression tested along with the other patch
>> arm_fps_group.patch on armv7l-unknown-linux-gnueabi, "corefile restored
>> general registers" failure in gdb.base/gcore.exp goes away.  Is it OK
>> for GDB mainline?
> 
> Please use uint32 for r11 also.  It's sometimes the frame pointer, but
> that's not required and it may have any arbitrary data in it.
> 
> Otherwise OK.
> 

Thanks, Dan.  I'll use uint32 for r11 in my commit.

Do you have comments to arm_fps_group.patch, which fixes the 2nd problem
I pointed out in [1]?  In short, this fix will make GDB treat fps
register as registers in float group, fps will disappear in output of
"info register".

gdb/
	* arm-tdep.c (arm_register_reggroup_p): New.
	(arm_gdbarch_init): Set arm_register_reggroup_p for hook
	register_reggroup_p.

arm_fps_group.patch is the 2nd patch I attached in [1].

[1] http://sourceware.org/ml/gdb-patches/2010-12/msg00134.html

-- 
Yao (齐尧)

Re: [patch, arm] Consistent display of registers in corefile

[Daniel Jacobowitz]

On Mon, Dec 20, 2010 at 10:29:15AM +0800, Yao Qi wrote:
> Do you have comments to arm_fps_group.patch, which fixes the 2nd problem
> I pointed out in [1]?  In short, this fix will make GDB treat fps
> register as registers in float group, fps will disappear in output of
> "info register".
> 
> gdb/
> 	* arm-tdep.c (arm_register_reggroup_p): New.
> 	(arm_gdbarch_init): Set arm_register_reggroup_p for hook
> 	register_reggroup_p.
> 
> arm_fps_group.patch is the 2nd patch I attached in [1].
> 
> [1] http://sourceware.org/ml/gdb-patches/2010-12/msg00134.html

That looks OK to me.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch, arm] Consistent display of registers in corefile

[Yao Qi]

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

On 12/10/2010 07:37 AM, Yao Qi wrote:
 > gdb/
 > 	* arm-tdep.c (arm_register_reggroup_p): New.
 > 	(arm_gdbarch_init): Set arm_register_reggroup_p for hook
 > 	register_reggroup_p.

> +static int
> +arm_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
> +			  struct reggroup *group)
> +{
> +  /* FPS register's type is INT, but belongs to float_group.  */
> +  if (regnum == ARM_FPS_REGNUM)
> +    return (group == float_reggroup);

FPS belongs to float_reggroup, save_reggroup, and restore_reggroup, 
instead of float_reggroup only.

OK to apply?

> +  else
> +    return default_register_reggroup_p (gdbarch, regnum, group);
> +}

-- 
Yao Qi

[-- Attachment #2: fsp_save_restore_reggroup.patch --]
[-- Type: text/x-patch, Size: 782 bytes --]

gdb/
	* arm-tdep.c (arm_register_reggroup_p): FPS register is in
	save_reggroup and restore_reggroup.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 9ec410d..d32b685 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -7234,9 +7234,12 @@ static int
 arm_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
 			  struct reggroup *group)
 {
-  /* FPS register's type is INT, but belongs to float_group.  */
+  /* FPS register's type is INT, but belongs to float_reggroup,
+     save_regroup, and restore_reggroup.  */
   if (regnum == ARM_FPS_REGNUM)
-    return (group == float_reggroup);
+    return (group == float_reggroup
+	    || group == save_reggroup
+	    || group == restore_reggroup);
   else
     return default_register_reggroup_p (gdbarch, regnum, group);
 }

Re: [patch, arm] Consistent display of registers in corefile

[Ulrich Weigand]

Yao Qi wrote:

> FPS belongs to float_reggroup, save_reggroup, and restore_reggroup, 
> instead of float_reggroup only.

> +  /* FPS register's type is INT, but belongs to float_reggroup,
> +     save_regroup, and restore_reggroup.  */
>    if (regnum == ARM_FPS_REGNUM)
> -    return (group == float_reggroup);
> +    return (group == float_reggroup
> +	    || group == save_reggroup
> +	    || group == restore_reggroup);
>    else
>      return default_register_reggroup_p (gdbarch, regnum, group);

Well, FPS should also belong to all_reggroup, otherwise it wil not
show up in "info all-registers" ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch, arm] Consistent display of registers in corefile

[Yao Qi]

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

On 01/13/2011 09:55 AM, Ulrich Weigand wrote:
> Yao Qi wrote:
>
>> FPS belongs to float_reggroup, save_reggroup, and restore_reggroup,
>> instead of float_reggroup only.
>
>> +  /* FPS register's type is INT, but belongs to float_reggroup,
>> +     save_regroup, and restore_reggroup.  */
>>     if (regnum == ARM_FPS_REGNUM)
>> -    return (group == float_reggroup);
>> +    return (group == float_reggroup
>> +	    || group == save_reggroup
>> +	    || group == restore_reggroup);
>>     else
>>       return default_register_reggroup_p (gdbarch, regnum, group);
>
> Well, FPS should also belong to all_reggroup, otherwise it wil not
> show up in "info all-registers" ...

OK, fix it in my new patch.

-- 
Yao Qi

[-- Attachment #2: fps_save_restore_reggroup.patch --]
[-- Type: text/x-patch, Size: 896 bytes --]

gdb/
	* arm-tdep.c (arm_register_reggroup_p): FPS register is in
	save_reggroup, restore_reggroup and all_reggroup.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 9ec410d..e24a6d9 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -7234,9 +7234,14 @@ static int
 arm_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
 			  struct reggroup *group)
 {
-  /* FPS register's type is INT, but belongs to float_group.  */
+  /* FPS register's type is INT, but belongs to float_reggroup.  Beside
+     this, FPS register belongs to save_regroup, restore_reggroup, and
+     all_reggroup, of course.  */
   if (regnum == ARM_FPS_REGNUM)
-    return (group == float_reggroup);
+    return (group == float_reggroup
+	    || group == save_reggroup
+	    || group == restore_reggroup
+	    || group == all_reggroup);
   else
     return default_register_reggroup_p (gdbarch, regnum, group);
 }

Re: [patch, arm] Consistent display of registers in corefile

[Ulrich Weigand]

Yao Qi wrote:

> 	* arm-tdep.c (arm_register_reggroup_p): FPS register is in
> 	save_reggroup, restore_reggroup and all_reggroup.

This is OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com