[patch 1/2] New gdbarch hook user_register_name

[patch 1/2] New gdbarch hook user_register_name

[Yao Qi]

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

Current GDB has a flexible mechanism adding standard register aliases by
backends, and register/alias name is used as a key to find the
corresponding register number.  However, this mechanism can't handle the
case if different registers have the same alias name.  It sounds a
little bit unreasonable, but 'fp' on ARM/Thumb is this case.

alias 'fp' is assumed to be r11, both in ARM mode and Thumb mode in
current GDB.  However, the expected (or correct) behavior is that 'fp'
is r11 on ARM mode, and 'fp' is r7 on Thumb mode, when user types 'p/x
$fp' in GDB.

Existing GDB can't meet such needs, so this patch is to refactor GDB a
little bit.  In this patch, a new gdbarch hook user_register_name is
added, and its default implementation is to look for register aliases in
user_regs_data, which is same as before in logic.  This patch should not
affect any functions of GDB.

Regression tested on x86_64-unknown-linux.  Comments?

Once this patch is applied, we leave more flexibility to backend to
determine what is the correct register number given a register alias.

-- 
Yao (齐尧)

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

2010-12-15  Yao Qi  <yao@codesourcery.com>

	* gdbarch.sh: Add new gdbarch hook user_register_name.
	* gdbarch.c: Regenerate.
	* gdbarch.h: Likewise.
	* user-regs.c (user_reg_map_name_to_regnum): Move some code to ...
	(default_user_register_name): ... here.  New.
	* user-regs.h : Declare default_user_register_name.

diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 78e5c48..cca0943 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -45,6 +45,7 @@
 #include "gdb_assert.h"
 #include "gdb_string.h"
 #include "reggroups.h"
+#include "user-regs.h"
 #include "osabi.h"
 #include "gdb_obstack.h"
 #include "observer.h"
@@ -180,6 +181,7 @@ struct gdbarch
   int call_dummy_location;
   gdbarch_push_dummy_code_ftype *push_dummy_code;
   gdbarch_print_registers_info_ftype *print_registers_info;
+  gdbarch_user_register_name_ftype *user_register_name;
   gdbarch_print_float_info_ftype *print_float_info;
   gdbarch_print_vector_info_ftype *print_vector_info;
   gdbarch_register_sim_regno_ftype *register_sim_regno;
@@ -330,6 +332,7 @@ struct gdbarch startup_gdbarch =
   0,  /* call_dummy_location */
   0,  /* push_dummy_code */
   default_print_registers_info,  /* print_registers_info */
+  default_user_register_name,  /* user_register_name */
   0,  /* print_float_info */
   0,  /* print_vector_info */
   legacy_register_sim_regno,  /* register_sim_regno */
@@ -474,6 +477,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->deprecated_fp_regnum = -1;
   gdbarch->call_dummy_location = AT_ENTRY_POINT;
   gdbarch->print_registers_info = default_print_registers_info;
+  gdbarch->user_register_name = default_user_register_name;
   gdbarch->register_sim_regno = legacy_register_sim_regno;
   gdbarch->cannot_fetch_register = cannot_register_not;
   gdbarch->cannot_store_register = cannot_register_not;
@@ -611,6 +615,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of call_dummy_location, invalid_p == 0 */
   /* Skip verify of push_dummy_code, has predicate */
   /* Skip verify of print_registers_info, invalid_p == 0 */
+  /* Skip verify of user_register_name, invalid_p == 0 */
   /* Skip verify of print_float_info, has predicate */
   /* Skip verify of print_vector_info, has predicate */
   /* Skip verify of register_sim_regno, invalid_p == 0 */
@@ -1226,6 +1231,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
                       "gdbarch_dump: unwind_sp = <%s>\n",
                       host_address_to_string (gdbarch->unwind_sp));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: user_register_name = <%s>\n",
+                      host_address_to_string (gdbarch->user_register_name));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: value_from_register = <%s>\n",
                       host_address_to_string (gdbarch->value_from_register));
   fprintf_unfiltered (file,
@@ -2041,6 +2049,23 @@ set_gdbarch_print_registers_info (struct gdbarch *gdbarch,
 }
 
 int
+gdbarch_user_register_name (struct gdbarch *gdbarch, const char *regname, int len)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->user_register_name != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_user_register_name called\n");
+  return gdbarch->user_register_name (gdbarch, regname, len);
+}
+
+void
+set_gdbarch_user_register_name (struct gdbarch *gdbarch,
+                                gdbarch_user_register_name_ftype user_register_name)
+{
+  gdbarch->user_register_name = user_register_name;
+}
+
+int
 gdbarch_print_float_info_p (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 443e1d5..deda8fc 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -322,6 +322,10 @@ typedef void (gdbarch_print_registers_info_ftype) (struct gdbarch *gdbarch, stru
 extern void gdbarch_print_registers_info (struct gdbarch *gdbarch, struct ui_file *file, struct frame_info *frame, int regnum, int all);
 extern void set_gdbarch_print_registers_info (struct gdbarch *gdbarch, gdbarch_print_registers_info_ftype *print_registers_info);
 
+typedef int (gdbarch_user_register_name_ftype) (struct gdbarch *gdbarch, const char *regname, int len);
+extern int gdbarch_user_register_name (struct gdbarch *gdbarch, const char *regname, int len);
+extern void set_gdbarch_user_register_name (struct gdbarch *gdbarch, gdbarch_user_register_name_ftype *user_register_name);
+
 extern int gdbarch_print_float_info_p (struct gdbarch *gdbarch);
 
 typedef void (gdbarch_print_float_info_ftype) (struct gdbarch *gdbarch, struct ui_file *file, struct frame_info *frame, const char *args);
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 5b66116..9291da5 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -462,6 +462,10 @@ v:int:call_dummy_location::::AT_ENTRY_POINT::0
 M:CORE_ADDR:push_dummy_code:CORE_ADDR sp, CORE_ADDR funaddr, struct value **args, int nargs, struct type *value_type, CORE_ADDR *real_pc, CORE_ADDR *bp_addr, struct regcache *regcache:sp, funaddr, args, nargs, value_type, real_pc, bp_addr, regcache
 
 m:void:print_registers_info:struct ui_file *file, struct frame_info *frame, int regnum, int all:file, frame, regnum, all::default_print_registers_info::0
+
+# Search REGNAME in user name space, and return register number if found.  Return -1 otherwise.
+m:int:user_register_name:const char *regname, int len:regname, len::default_user_register_name::0
+
 M:void:print_float_info:struct ui_file *file, struct frame_info *frame, const char *args:file, frame, args
 M:void:print_vector_info:struct ui_file *file, struct frame_info *frame, const char *args:file, frame, args
 # MAP a GDB RAW register number onto a simulator register number.  See
@@ -1245,6 +1249,7 @@ cat <<EOF
 #include "gdb_assert.h"
 #include "gdb_string.h"
 #include "reggroups.h"
+#include "user-regs.h"
 #include "osabi.h"
 #include "gdb_obstack.h"
 #include "observer.h"
diff --git a/gdb/user-regs.c b/gdb/user-regs.c
index 0107377..eb1ef10 100644
--- a/gdb/user-regs.c
+++ b/gdb/user-regs.c
@@ -150,21 +150,28 @@ user_reg_map_name_to_regnum (struct gdbarch *gdbarch, const char *name,
       }
   }
 
-  /* Search the user name space.  */
-  {
-    struct gdb_user_regs *regs = gdbarch_data (gdbarch, user_regs_data);
-    struct user_reg *reg;
-    int nr;
+  return gdbarch_user_register_name (gdbarch, name, len);
+}
 
-    for (nr = 0, reg = regs->first; reg != NULL; reg = reg->next, nr++)
-      {
-	if ((len < 0 && strcmp (reg->name, name))
-	    || (len == strlen (reg->name)
-		&& strncmp (reg->name, name, len) == 0))
-	  return gdbarch_num_regs (gdbarch)
-		 + gdbarch_num_pseudo_regs (gdbarch) + nr;
-      }
-  }
+/* Search REGNAME in user name space, and return register number if found,
+   otherwise return -1.  */
+
+int
+default_user_register_name (struct gdbarch *gdbarch, const char *regname,
+			    int len)
+{
+  struct gdb_user_regs *regs = gdbarch_data (gdbarch, user_regs_data);
+  struct user_reg *reg;
+  int nr;
+
+  for (nr = 0, reg = regs->first; reg != NULL; reg = reg->next, nr++)
+    {
+      if ((len < 0 && strcmp (reg->name, regname))
+	  || (len == strlen (reg->name)
+	      && strncmp (reg->name, regname, len) == 0))
+	return gdbarch_num_regs (gdbarch)
+	  + gdbarch_num_pseudo_regs (gdbarch) + nr;
+    }
 
   return -1;
 }
diff --git a/gdb/user-regs.h b/gdb/user-regs.h
index f5fa12e..093ac9d 100644
--- a/gdb/user-regs.h
+++ b/gdb/user-regs.h
@@ -69,4 +69,6 @@ extern void user_reg_add_builtin (const char *name,
 extern void user_reg_add (struct gdbarch *gdbarch, const char *name, 
 			  user_reg_read_ftype *read, const void *baton);
 
+extern int default_user_register_name (struct gdbarch *gdbarch,
+				       const char *regname, int len);
 #endif

[patch 2/2] Implement gdbarch hook user_register_name on ARM

[Yao Qi]

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

On 12/15/2010 06:22 PM, Yao Qi wrote:
> Once this patch is applied, we leave more flexibility to backend to
> determine what is the correct register number given a register alias.

This patch is to implement user_register_name on ARM.  With this, we can
handle alias "fp" according to the current frame's mode (ARM or Thumb).

Regression testing is still running on ARM.  Comments are welcome.

-- 
Yao (齐尧)

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

2010-12-15  Yao Qi  <yao@codesourcery.com>

	* arm-tdep.c (arm_user_register_name): New.
	(arm_gdbarch_init): Install arm_user_register_nam on gdbarch hook
	user_register_name.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 636c1de..618f82f 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -2918,6 +2918,27 @@ arm_neon_quad_type (struct gdbarch *gdbarch)
   return tdep->neon_quad_type;
 }
 
+static int
+arm_user_register_name (struct gdbarch *gdbarch, const char *regname, int len)
+{
+  if ((len < 0 && strcmp ("fp", regname))
+      || (len == strlen ("fp")
+	  && strncmp ("fp", regname, len) == 0))
+    {
+      /* `fp' register means different registers on ARM and Thumb.  Return
+	 the correct `fp' register number according the state of current
+	 frame.  */
+      struct frame_info *frame = get_selected_frame (NULL);
+      if (arm_frame_is_thumb (frame))
+	return THUMB_FP_REGNUM;
+      else
+	return ARM_FP_REGNUM;
+    }
+  else
+    /* Leave search to other registers to default implementation.  */
+    return default_user_register_name (gdbarch, regname, len);
+}
+
 /* Return the GDB type object for the "standard" data type of data in
    register N.  */
 
@@ -7462,6 +7483,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_user_register_name (gdbarch, arm_user_register_name);
 
   /* This "info float" is FPA-specific.  Use the generic version if we
      do not have FPA.  */

Re: [patch 2/2] Implement gdbarch hook user_register_name on ARM

[Ulrich Weigand]

Yao Qi wrote:
> On 12/15/2010 06:22 PM, Yao Qi wrote:
> > Once this patch is applied, we leave more flexibility to backend to
> > determine what is the correct register number given a register alias.
> 
> This patch is to implement user_register_name on ARM.  With this, we can
> handle alias "fp" according to the current frame's mode (ARM or Thumb).
> 
> Regression testing is still running on ARM.  Comments are welcome.

I'm wondering why the ARM back-end actively defines "fp" as user register
anyway.  If it simply were to *not* do so, GDB would fall back to the
default implementation of $fp using value_of_builtin_frame_fp_reg, which
seems to do always the correct thing anyway.  It returns the value of the
frame base as returned by arm_normal_frame_base, which will have been
determined from r11 or r7 as appropriate for the mode.

Bye,
Ulrich

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

Re: [patch 2/2] Implement gdbarch hook user_register_name on ARM

[Yao Qi]

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

On 12/22/2010 03:07 AM, Ulrich Weigand wrote:
> Yao Qi wrote:
>> On 12/15/2010 06:22 PM, Yao Qi wrote:
>>> Once this patch is applied, we leave more flexibility to backend to
>>> determine what is the correct register number given a register alias.
>>
>> This patch is to implement user_register_name on ARM.  With this, we can
>> handle alias "fp" according to the current frame's mode (ARM or Thumb).
>>
>> Regression testing is still running on ARM.  Comments are welcome.
> 
> I'm wondering why the ARM back-end actively defines "fp" as user register
> anyway.  If it simply were to *not* do so, GDB would fall back to the
> default implementation of $fp using value_of_builtin_frame_fp_reg, which
> seems to do always the correct thing anyway.  It returns the value of the
> frame base as returned by arm_normal_frame_base, which will have been
> determined from r11 or r7 as appropriate for the mode.

Good catch! set_gdbarch_deprecated_fp_regnum is called in
arm_gdbarch_init since 2003, introduced by this patch "Deprecate
TARGET_READ_FP, read_fp and FP_REGNUM"
http://sourceware.org/ml/gdb-patches/2003-04/msg00471.html

set_gdbarch_deprecated_fp_regnum is no longer used in most of targets,
and I don't see any reason why we have to keep it in ARM, so I draft
this one-line patch.

Any comments/objections to this one-line patch?  [Regression test is
still running.]

-- 
Yao (齐尧)

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

gdb/
	* arm-tdep.c (arm_gdbarch_init): Remove invoke to
	set_gdbarch_deprecated_fp_regnum.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 636c1de..1f112ee 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -7457,7 +7457,6 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 					 arm_remote_breakpoint_from_pc);
 
   /* Information about registers, etc.  */
-  set_gdbarch_deprecated_fp_regnum (gdbarch, ARM_FP_REGNUM);	/* ??? */
   set_gdbarch_sp_regnum (gdbarch, ARM_SP_REGNUM);
   set_gdbarch_pc_regnum (gdbarch, ARM_PC_REGNUM);
   set_gdbarch_num_regs (gdbarch, ARM_NUM_REGS);

Re: [patch 2/2] Implement gdbarch hook user_register_name on ARM

[Richard Earnshaw]

On Wed, 2010-12-15 at 18:47 +0800, Yao Qi wrote:
> On 12/15/2010 06:22 PM, Yao Qi wrote:
> > Once this patch is applied, we leave more flexibility to backend to
> > determine what is the correct register number given a register alias.
> 
> This patch is to implement user_register_name on ARM.  With this, we can
> handle alias "fp" according to the current frame's mode (ARM or Thumb).
> 
> Regression testing is still running on ARM.  Comments are welcome.

I think this is misguided.  Historically, the ARM back-end to gcc
defined an alias of fp for R11, whether in ARM or Thumb mode.  The fact
that the Thumb-1 back-end doesn't use that register as a frame-pointer
is not particularly relevant.

The situation is quite messy, as GCC uses different registers in Thumb-1
and Thumb-2 code for a frame pointer; your code doesn't handle that
case, but seems to assume that all thumb code will use R7 as the frame
pointer.

Overall, I think it's just best if 'fp' is treated like any other
standard register alias on ARM and therefore that it always refers to
R11.  If we really want a register alias that refers to the *current*
frame pointer register, then we need a new name that doesn't conflict
with anything in the current or previous ABIs.  Maybe a
slightly-long-winded name like 'frame'?

R.

Re: [patch 2/2] Implement gdbarch hook user_register_name on ARM

[Mark Kettenis]

> Date: Thu, 23 Dec 2010 00:34:22 +0800
> From: Yao Qi <yao@codesourcery.com>
> 
> On 12/22/2010 03:07 AM, Ulrich Weigand wrote:
> > Yao Qi wrote:
> >> On 12/15/2010 06:22 PM, Yao Qi wrote:
> >>> Once this patch is applied, we leave more flexibility to backend to
> >>> determine what is the correct register number given a register alias.
> >>
> >> This patch is to implement user_register_name on ARM.  With this, we can
> >> handle alias "fp" according to the current frame's mode (ARM or Thumb).
> >>
> >> Regression testing is still running on ARM.  Comments are welcome.
> > 
> > I'm wondering why the ARM back-end actively defines "fp" as user register
> > anyway.  If it simply were to *not* do so, GDB would fall back to the
> > default implementation of $fp using value_of_builtin_frame_fp_reg, which
> > seems to do always the correct thing anyway.  It returns the value of the
> > frame base as returned by arm_normal_frame_base, which will have been
> > determined from r11 or r7 as appropriate for the mode.
> 
> Good catch! set_gdbarch_deprecated_fp_regnum is called in
> arm_gdbarch_init since 2003, introduced by this patch "Deprecate
> TARGET_READ_FP, read_fp and FP_REGNUM"
> http://sourceware.org/ml/gdb-patches/2003-04/msg00471.html
> 
> set_gdbarch_deprecated_fp_regnum is no longer used in most of targets,
> and I don't see any reason why we have to keep it in ARM, so I draft
> this one-line patch.
> 
> Any comments/objections to this one-line patch?  [Regression test is
> still running.]

Actually grep tells me that arm is the only target that still uses
this.  A sure sign that we can get rid of it!

A followup diff to remove all the deprected fp stuff would be nice ;)
But don't let you stop that from committing this.

> gdb/
> 	* arm-tdep.c (arm_gdbarch_init): Remove invoke to
> 	set_gdbarch_deprecated_fp_regnum.
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 636c1de..1f112ee 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -7457,7 +7457,6 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  					 arm_remote_breakpoint_from_pc);
>  
>    /* Information about registers, etc.  */
> -  set_gdbarch_deprecated_fp_regnum (gdbarch, ARM_FP_REGNUM);	/* ??? */
>    set_gdbarch_sp_regnum (gdbarch, ARM_SP_REGNUM);
>    set_gdbarch_pc_regnum (gdbarch, ARM_PC_REGNUM);
>    set_gdbarch_num_regs (gdbarch, ARM_NUM_REGS);

Re: [patch 2/2] Implement gdbarch hook user_register_name on ARM

[Ulrich Weigand]

Yao Qi wrote:

> Good catch! set_gdbarch_deprecated_fp_regnum is called in
> arm_gdbarch_init since 2003, introduced by this patch "Deprecate
> TARGET_READ_FP, read_fp and FP_REGNUM"
> http://sourceware.org/ml/gdb-patches/2003-04/msg00471.html
> 
> set_gdbarch_deprecated_fp_regnum is no longer used in most of targets,
> and I don't see any reason why we have to keep it in ARM, so I draft
> this one-line patch.

Huh, I didn't even see this, I was refering to this line:
  { "fp", 11 },
in arm_register_aliases.  As long as this line is there, changes to
set_gdbarch_deprecated_fp_regnum probably don't matter as this isn't
even evaluated, since "fp" is just treated as a user register instead
of a standard register.

But you're right that you need to remove the set_gdbarch_deprecated_fp_regnum
*also* in order to get the full default logic I had described.

Bye,
Ulrich

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

Re: [patch 2/2] Implement gdbarch hook user_register_name on ARM

[Ulrich Weigand]

Richard Earnshaw wrote:
> Overall, I think it's just best if 'fp' is treated like any other
> standard register alias on ARM and therefore that it always refers to
> R11.  If we really want a register alias that refers to the *current*
> frame pointer register, then we need a new name that doesn't conflict
> with anything in the current or previous ABIs.  Maybe a
> slightly-long-winded name like 'frame'?

Well, the problem is that that "fp" itself has a fixed meaning in GDB.

This shows up most problematically in the MI interface.  When the MI
user interface application wants to select a frame in certain MI
commands, that frame has to identified via its "frame base" value.

However, there is no defined way to *query* that value.  Therefore,
my understanding is that the MI frontends typically query the value
of the standard "fp" register to retrieve this value.  If an architecture
back-end now goes and redefines what "fp" stands for, this will break
this MI frontend convention ...

Bye,
Ulrich

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

Re: [patch 2/2] Implement gdbarch hook user_register_name on ARM

[Joel Brobecker]

> However, there is no defined way to *query* that value.  Therefore,
> my understanding is that the MI frontends typically query the value
> of the standard "fp" register to retrieve this value.  If an architecture
> back-end now goes and redefines what "fp" stands for, this will break
> this MI frontend convention ...

I am not sure I understand the problem.  Could we use "get_frame_base"
instead?

-- 
Joel

Re: [patch 2/2] Implement gdbarch hook user_register_name on ARM

[Yao Qi]

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

On 12/23/2010 06:02 AM, Ulrich Weigand wrote:
> Yao Qi wrote:
> 
>> Good catch! set_gdbarch_deprecated_fp_regnum is called in
>> arm_gdbarch_init since 2003, introduced by this patch "Deprecate
>> TARGET_READ_FP, read_fp and FP_REGNUM"
>> http://sourceware.org/ml/gdb-patches/2003-04/msg00471.html
>>
>> set_gdbarch_deprecated_fp_regnum is no longer used in most of targets,
>> and I don't see any reason why we have to keep it in ARM, so I draft
>> this one-line patch.
> 
> Huh, I didn't even see this, I was refering to this line:
>   { "fp", 11 },
> in arm_register_aliases.  As long as this line is there, changes to
> set_gdbarch_deprecated_fp_regnum probably don't matter as this isn't
> even evaluated, since "fp" is just treated as a user register instead
> of a standard register.
> 
Ulrich,
changes to set_gdbarch_deprecated_fp_regnum matters here.

There are two "fp", "pc" and "sp" in user registers.  The first one is
added from builtin_user_regs (fp, pc, sp, ps) in user_regs_init, and the
second one is added from `arm_register_aliases' in our case.  The first
one is always used, so these three in arm_register_aliases are redundant.

  { "fp", 11 },
  { "sp", 13 },
  { "pc", 15 },

How about this patch to remove them?

> But you're right that you need to remove the set_gdbarch_deprecated_fp_regnum
> *also* in order to get the full default logic I had described.
> 

Regression test is done.  Two failures are fixed.
FAIL: gdb.mi/mi-var-display.exp: create variable a2 in different scope
FAIL: gdb.mi/mi2-var-display.exp: create variable a2 in different scope

Committed.

-- 
Yao (齐尧)

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

2010-12-23  Yao Qi  <yao@codesourcery.com>

	arm-tdep.c: (arm_register_aliases): Remove sp, pc, and fp

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 636c1de..11c75cd 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -175,12 +175,9 @@ static const struct
   { "tr", 9 },
   /* Special names.  */
   { "ip", 12 },
-  { "sp", 13 },
   { "lr", 14 },
-  { "pc", 15 },
   /* Names used by GCC (not listed in the ARM EABI).  */
   { "sl", 10 },
-  { "fp", 11 },
   /* A special name from the older ATPCS.  */
   { "wr", 7 },
 };

Re: [patch 2/2] Implement gdbarch hook user_register_name on ARM

[Yao Qi]

On 12/23/2010 01:43 AM, Mark Kettenis wrote:
> Actually grep tells me that arm is the only target that still uses
> this.  A sure sign that we can get rid of it!
> 
grep tells me something different,

frv-tdep.c:  set_gdbarch_deprecated_fp_regnum (gdbarch, fp_regnum);
rs6000-tdep.c:  set_gdbarch_deprecated_fp_regnum (gdbarch, PPC_R0_REGNUM
+ 1);

> A followup diff to remove all the deprected fp stuff would be nice ;)
> But don't let you stop that from committing this.

I'd like to, if we can prove set_gdbarch_deprecated_fp_regnum is not
useful to frv and rs6000.

-- 
Yao (齐尧)

Re: [patch 2/2] Implement gdbarch hook user_register_name on ARM

[Mark Kettenis]

> Date: Thu, 23 Dec 2010 07:18:53 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > However, there is no defined way to *query* that value.  Therefore,
> > my understanding is that the MI frontends typically query the value
> > of the standard "fp" register to retrieve this value.  If an architecture
> > back-end now goes and redefines what "fp" stands for, this will break
> > this MI frontend convention ...
> 
> I am not sure I understand the problem.  Could we use "get_frame_base"
> instead?

As far as I know, that is exactly what happens when you *don't* set
deprecated_fp_regnum, and you don't have a "hard" frame pointer
register explicitly named "fp".

See std-reg.c:value_of_builtin_frame_fp_reg().

Really, if arm can get away with not setting deprecated_fp_regnum(),
we should go for it.

Re: [patch 2/2] Implement gdbarch hook user_register_name on ARM

[Ulrich Weigand]

Yao Qi wrote:
> On 12/23/2010 06:02 AM, Ulrich Weigand wrote:
> > Huh, I didn't even see this, I was refering to this line:
> >   { "fp", 11 },
> > in arm_register_aliases.  As long as this line is there, changes to
> > set_gdbarch_deprecated_fp_regnum probably don't matter as this isn't
> > even evaluated, since "fp" is just treated as a user register instead
> > of a standard register.
> > 
> Ulrich,
> changes to set_gdbarch_deprecated_fp_regnum matters here.
> 
> There are two "fp", "pc" and "sp" in user registers.  The first one is
> added from builtin_user_regs (fp, pc, sp, ps) in user_regs_init, and the
> second one is added from `arm_register_aliases' in our case.  The first
> one is always used, so these three in arm_register_aliases are redundant.

I see.  I had thought the calls in arm_register_aliases would override the
standard registers, but it looks like you're right, they don't.

>   { "fp", 11 },
>   { "sp", 13 },
>   { "pc", 15 },
> 
> How about this patch to remove them?

If they don't actually take effect, I agree it is better to remove them.
However, I'd prefer to see some comment in the code explaining why these
names are not (and should not be) added as aliases ...

Bye,
Ulrich

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

Re: [patch 2/2] Implement gdbarch hook user_register_name on ARM

[Yao Qi]

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

On 12/24/2010 01:56 AM, Ulrich Weigand wrote:

>
>>    { "fp", 11 },
>>    { "sp", 13 },
>>    { "pc", 15 },
>>
>> How about this patch to remove them?
>
> If they don't actually take effect, I agree it is better to remove them.
> However, I'd prefer to see some comment in the code explaining why these
> names are not (and should not be) added as aliases ...

How about this?

-- 
Yao

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

gdb/

	* arm-tdep.c (arm_register_aliases): Remove sp, pc, and fp.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 2a9303c..959c449 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -135,7 +135,10 @@ static const char *arm_force_mode_string = "auto";
 /* Number of different reg name sets (options).  */
 static int num_disassembly_options;
 
-/* The standard register names, and all the valid aliases for them.  */
+/* The standard register names, and all the valid aliases for them.  Note
+   that `fp', `sp' and `pc' are not added in this alias list, because they
+   have been added as builtin user registers in
+   std-regs.c:_initialize_frame_reg.  */
 static const struct
 {
   const char *name;
@@ -176,12 +179,9 @@ static const struct
   { "tr", 9 },
   /* Special names.  */
   { "ip", 12 },
-  { "sp", 13 },
   { "lr", 14 },
-  { "pc", 15 },
   /* Names used by GCC (not listed in the ARM EABI).  */
   { "sl", 10 },
-  { "fp", 11 },
   /* A special name from the older ATPCS.  */
   { "wr", 7 },
 };

Re: [patch 2/2] Implement gdbarch hook user_register_name on ARM

[Ulrich Weigand]

Yao Qi wrote:

> How about this?

> 	* arm-tdep.c (arm_register_aliases): Remove sp, pc, and fp.

This is OK, thanks!

Bye,
Ulrich

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