[patch] mips-tdep: info registers

[patch] mips-tdep: info registers

[Aleksandar Ristovski]

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

Hello,

This is applicable to mips targets.

When using info registers $N command, gdb_assert will be 
triggered if N falls in the raw register number area. 
However, this is counter-intuitive.

The change simply maps raw-register number to pseudo 
register number and continues as before.

-------------------------------------
Example: current situation

(gdb) info registers $1
../../gdb/mips-tdep.c:4307: internal-error: 
mips_print_registers_info:
Assertion `regnum >= gdbarch_num_regs (current_gdbarch)' 
failed.

Example: with the patch

(gdb) info registers $1
at: 0x8020000
--------------------------------------



Thanks,

Aleksandar Ristovski
QNX Software Systems


[-- Attachment #2: mips-tdep.c-info-registers-20090212.diff.ChangeLog --]
[-- Type: text/plain, Size: 146 bytes --]

2009-02-12  Aleksandar Ristovski  <aristovski@qnx.com>

	* mips-tdep.c (mips_print_registers_info): Map raw register number to
	pseudo register.


[-- Attachment #3: mips-tdep.c-info-registers-20090212.diff --]
[-- Type: text/x-patch, Size: 743 bytes --]

Index: gdb/mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.489
diff -u -p -r1.489 mips-tdep.c
--- gdb/mips-tdep.c	3 Jan 2009 05:57:52 -0000	1.489
+++ gdb/mips-tdep.c	12 Feb 2009 16:19:02 -0000
@@ -4601,7 +4601,9 @@ mips_print_registers_info (struct gdbarc
 {
   if (regnum != -1)		/* do one specified register */
     {
-      gdb_assert (regnum >= gdbarch_num_regs (gdbarch));
+      if (regnum < gdbarch_num_regs (gdbarch)
+	  && regnum >= 0)
+	regnum += gdbarch_num_regs (gdbarch); /* Print pseudo register.  */
       if (*(gdbarch_register_name (gdbarch, regnum)) == '\0')
 	error (_("Not a valid register for the current processor type"));
 

Re: [patch] mips-tdep: info registers

[Joel Brobecker]

> (gdb) info registers $1
> ../../gdb/mips-tdep.c:4307: internal-error: 
> mips_print_registers_info:
> Assertion `regnum >= gdbarch_num_regs (current_gdbarch)' 
> failed.

Surprisingly, this feature is not documented yet. The documentation
mentions "info registers regname", but not "info registers regno".
Would you mind adding a line or two and sending a (separate) patch
to this list? Eli is the documentation guru...

> (gdb) info registers $1
> at: 0x8020000

> 2009-02-12  Aleksandar Ristovski  <aristovski@qnx.com>
> 
> 	* mips-tdep.c (mips_print_registers_info): Map raw register number to
> 	pseudo register.

This seems reasonable to me.

Just being a documentation freak, can you provide a more detailed
comment about what is happening and what you are doing?
Below is my suggestion - adjust as you think is best...

> -      gdb_assert (regnum >= gdbarch_num_regs (gdbarch));
> +      if (regnum < gdbarch_num_regs (gdbarch)
> +	  && regnum >= 0)
> +	regnum += gdbarch_num_regs (gdbarch); /* Print pseudo register.  */

    /* If given a raw register number, convert it to its associated
       pseudo register number first, since the rest of this code requires
       a pseudo register.  */

Pre-approved with the addition of the comment requested. Can you also
confirm that you were able to check this change against the testsuite?
I don't see how this could negatively affect the debugger, but you
never know sometimes...

Thanks,
-- 
Joel

Re: [patch] mips-tdep: info registers

[Daniel Jacobowitz]

On Sun, Feb 22, 2009 at 06:08:20PM -0800, Joel Brobecker wrote:
> > (gdb) info registers $1
> > ../../gdb/mips-tdep.c:4307: internal-error: 
> > mips_print_registers_info:
> > Assertion `regnum >= gdbarch_num_regs (current_gdbarch)' 
> > failed.
> 
> Surprisingly, this feature is not documented yet. The documentation
> mentions "info registers regname", but not "info registers regno".
> Would you mind adding a line or two and sending a (separate) patch
> to this list? Eli is the documentation guru...

Should this feature even exist?  This is a different "$1" than
anywhere else in GDB you might type that...

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] mips-tdep: info registers

[Joel Brobecker]

> > Surprisingly, this feature is not documented yet. The documentation
> > mentions "info registers regname", but not "info registers regno".
> > Would you mind adding a line or two and sending a (separate) patch
> > to this list? Eli is the documentation guru...
> 
> Should this feature even exist?  This is a different "$1" than
> anywhere else in GDB you might type that...

I was asking myself this question, but there is explict code in
GDB to handle that case, so I thought that this was deliberate.
On the other hand, I also thought that this was a very cool way
of knowing what register number NUM actually is. For instance,
on x86, register number 3 is (drums...) ebx:

    (gdb) info reg $3
    ebx            0xb7e84ff4       -1209511948

With x86, the number of registers is fairly limited, but there
are other processors where this isn't the case. The CPU that gave
me the largest number of pimples so far is ia64...

So I found that the above syntax was a cool thing to know about
and worth retaining. But I've been known to debug the debugger to
obtain that information, so I'm not terribly attached to this
syntax. I agree it is a little confusing with our convenience
variables...

-- 
Joel

Re: [patch] mips-tdep: info registers

[Eli Zaretskii]

> Date: Sun, 22 Feb 2009 19:00:14 -0800
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > > Surprisingly, this feature is not documented yet. The documentation
> > > mentions "info registers regname", but not "info registers regno".
> > > Would you mind adding a line or two and sending a (separate) patch
> > > to this list? Eli is the documentation guru...
> > 
> > Should this feature even exist?  This is a different "$1" than
> > anywhere else in GDB you might type that...
> 
> I was asking myself this question, but there is explict code in
> GDB to handle that case, so I thought that this was deliberate.
> On the other hand, I also thought that this was a very cool way
> of knowing what register number NUM actually is. For instance,
> on x86, register number 3 is (drums...) ebx:
> 
>     (gdb) info reg $3
>     ebx            0xb7e84ff4       -1209511948
> 
> With x86, the number of registers is fairly limited, but there
> are other processors where this isn't the case. The CPU that gave
> me the largest number of pimples so far is ia64...
> 
> So I found that the above syntax was a cool thing to know about
> and worth retaining.

I agree.  Documenting it would be good, thanks.

Re: [patch] mips-tdep: info registers

[Aleksandar Ristovski]

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

Eli Zaretskii wrote:
>> Date: Sun, 22 Feb 2009 19:00:14 -0800
>> From: Joel Brobecker <brobecker@adacore.com>
>>
>>>> Surprisingly, this feature is not documented yet. The documentation
>>>> mentions "info registers regname", but not "info registers regno".
>>>> Would you mind adding a line or two and sending a (separate) patch
>>>> to this list? Eli is the documentation guru...
>>> Should this feature even exist?  This is a different "$1" than
>>> anywhere else in GDB you might type that...
>> I was asking myself this question, but there is explict code in
>> GDB to handle that case, so I thought that this was deliberate.
>> On the other hand, I also thought that this was a very cool way
>> of knowing what register number NUM actually is. For instance,
>> on x86, register number 3 is (drums...) ebx:
>>
>>     (gdb) info reg $3
>>     ebx            0xb7e84ff4       -1209511948
>>
>> With x86, the number of registers is fairly limited, but there
>> are other processors where this isn't the case. The CPU that gave
>> me the largest number of pimples so far is ia64...
>>
>> So I found that the above syntax was a cool thing to know about
>> and worth retaining.
> 
> I agree.  Documenting it would be good, thanks.
> 

I added a note about the regno syntax.

Thanks,

Aleksandar Ristovski
QNX Software Systems


ChangeLog:

* gdb.texinfo (info registers): Add a note about permitted
info registers regno syntax.



[-- Attachment #2: mips-tdep.c-info-registers-doc-20090223.diff --]
[-- Type: text/x-patch, Size: 523 bytes --]

Index: gdb/doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.558
diff -r1.558 gdb.texinfo
7436a7437,7442
> 
> Note that syntax using register number (@var{regno}) in place of 
> @var{regname} is also permitted.  While for some architectures (like x86)
> @var{regno} has a meaning only within @value{GDBN} context and is 
> subject to change in different @value{GDBN} versions, others (like mips)
> explicitly define them.

Re: [patch] mips-tdep: info registers

[Eli Zaretskii]

> From:  Aleksandar Ristovski <aristovski@qnx.com>
> Date:  Mon, 23 Feb 2009 11:05:03 -0500
> 
> > I agree.  Documenting it would be good, thanks.
> > 
> 
> I added a note about the regno syntax.

Thank you.

> ChangeLog:
> 
> * gdb.texinfo (info registers): Add a note about permitted
> info registers regno syntax.

The string in parentheses should be the name of the node in which you
made a change, in this case "Registers".

> Index: gdb/doc/gdb.texinfo
> ===================================================================
> RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
> retrieving revision 1.558
> diff -r1.558 gdb.texinfo
> 7436a7437,7442
> > 
> > Note that syntax using register number (@var{regno}) in place of 
> > @var{regname} is also permitted.  While for some architectures (like x86)
> > @var{regno} has a meaning only within @value{GDBN} context and is 
> > subject to change in different @value{GDBN} versions, others (like mips)
> > explicitly define them.

This is fine, thanks.

Re: [patch] mips-tdep: info registers

[Mark Kettenis]

> Date: Sun, 22 Feb 2009 21:52:30 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Sun, Feb 22, 2009 at 06:08:20PM -0800, Joel Brobecker wrote:
> > > (gdb) info registers $1
> > > ../../gdb/mips-tdep.c:4307: internal-error: 
> > > mips_print_registers_info:
> > > Assertion `regnum >= gdbarch_num_regs (current_gdbarch)' 
> > > failed.
> > 
> > Surprisingly, this feature is not documented yet. The documentation
> > mentions "info registers regname", but not "info registers regno".
> > Would you mind adding a line or two and sending a (separate) patch
> > to this list? Eli is the documentation guru...
> 
> Should this feature even exist?  This is a different "$1" than
> anywhere else in GDB you might type that...

I think it shouldn't; the register numbers are internal to GDB an may
change (once they've been properly decoupled from the remote
interface).

Re: [patch] mips-tdep: info registers

[Aleksandar Ristovski]

Mark Kettenis wrote:
>> Date: Sun, 22 Feb 2009 21:52:30 -0500
>> From: Daniel Jacobowitz <drow@false.org>
>>
>> On Sun, Feb 22, 2009 at 06:08:20PM -0800, Joel Brobecker wrote:
>>>> (gdb) info registers $1
>>>> ../../gdb/mips-tdep.c:4307: internal-error: 
>>>> mips_print_registers_info:
>>>> Assertion `regnum >= gdbarch_num_regs (current_gdbarch)' 
>>>> failed.
>>> Surprisingly, this feature is not documented yet. The documentation
>>> mentions "info registers regname", but not "info registers regno".
>>> Would you mind adding a line or two and sending a (separate) patch
>>> to this list? Eli is the documentation guru...
>> Should this feature even exist?  This is a different "$1" than
>> anywhere else in GDB you might type that...
> 
> I think it shouldn't; the register numbers are internal to GDB an may
> change (once they've been properly decoupled from the remote
> interface).
> 

The use-case where the problem came up is debugging inline 
assembly code which uses register numbers instead of 
register names.

while "$1" may be confusing due to convenience variables, 
syntax:
(gdb) info register 1
is also permitted and looks quite straight-forward.



Thanks,

Aleksandar Ristovski
QNX Software Systems

Re: [patch] mips-tdep: info registers

[Daniel Jacobowitz]

On Mon, Feb 23, 2009 at 11:06:20AM -0500, Aleksandar Ristovski wrote:
> The use-case where the problem came up is debugging inline assembly code 
> which uses register numbers instead of register names.
>
> while "$1" may be confusing due to convenience variables, syntax:
> (gdb) info register 1
> is also permitted and looks quite straight-forward.

The problem is, this doesn't do what you want it to do.  It looks up
GDB internal register number 1.  That just happens, at the moment,
to match up with the raw register backing $at.  But it might change
in the future.  It's not "the register named $1", which is what a
MIPS user should expect.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] mips-tdep: info registers

[Joel Brobecker]

> The problem is, this doesn't do what you want it to do.  It looks up
> GDB internal register number 1.  That just happens, at the moment,
> to match up with the raw register backing $at.  But it might change
> in the future.  It's not "the register named $1", which is what a
> MIPS user should expect.

I agree, now, that we shouldn't have this feature. I can have a look
at removing it...

-- 
Joel

Re: [patch] mips-tdep: info registers

[Aleksandar Ristovski]

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

Joel Brobecker wrote:
>> The problem is, this doesn't do what you want it to do.  It looks up
>> GDB internal register number 1.  That just happens, at the moment,
>> to match up with the raw register backing $at.  But it might change
>> in the future.  It's not "the register named $1", which is what a
>> MIPS user should expect.
> 
> I agree, now, that we shouldn't have this feature. I can have a look
> at removing it...
> 

Joel, the attached removes numeric value syntax, but allows 
it for mips; mips will explicitly map regno to raw regno 
(which is currently 1-1).

I will let you take it from here.


Thanks,

Aleksandar Ristovski
QNX Software Systems

[-- Attachment #2: mips-tdep.c-info-registers-20090223.diff --]
[-- Type: text/x-patch, Size: 1406 bytes --]

Index: gdb/infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.231
diff -r1.231 infcmd.c
1951,1965d1950
<       /* A register number?  (how portable is this one?).  */
<       {
< 	char *endptr;
< 	int regnum = strtol (start, &endptr, 0);
< 	if (endptr == end
< 	    && regnum >= 0
< 	    && regnum < gdbarch_num_regs (gdbarch)
< 			+ gdbarch_num_pseudo_regs (gdbarch))
< 	  {
< 	    gdbarch_print_registers_info (gdbarch, gdb_stdout,
< 					  frame, regnum, fpregs);
< 	    continue;
< 	  }
<       }
< 
Index: gdb/mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.490
diff -r1.490 mips-tdep.c
442a443,449
>   static char *mips_gpr_numeric_names[] = {
>     "0", "1", "2", "3", "4", "5", "6", "7",
>     "8", "9", "10", "11", "12", "13", "14", "15",
>     "16", "17", "18", "19", "20", "21", "22", "23",
>     "24", "25", "26", "27", "28", "29", "30", "31"
>   };
> 
449c456,461
<     return "";
---
>     {
>       if (regno >= 0)
> 	return mips_gpr_numeric_names [regno];
>       else
> 	return "";
>     }
4604c4616,4618
<       gdb_assert (regnum >= gdbarch_num_regs (gdbarch));
---
>       if (regnum < gdbarch_num_regs (gdbarch)
> 	  && regnum >= 0)
> 	regnum += gdbarch_num_regs (gdbarch); /* Print pseudo register.  */

Re: [patch] mips-tdep: info registers

[Aleksandar Ristovski]

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

Aleksandar Ristovski wrote:
> Joel Brobecker wrote:
>>> The problem is, this doesn't do what you want it to do.  It looks up
>>> GDB internal register number 1.  That just happens, at the moment,
>>> to match up with the raw register backing $at.  But it might change
>>> in the future.  It's not "the register named $1", which is what a
>>> MIPS user should expect.
>>
>> I agree, now, that we shouldn't have this feature. I can have a look
>> at removing it...
>>
> 
> Joel, the attached removes numeric value syntax, but allows it for mips; 
> mips will explicitly map regno to raw regno (which is currently 1-1).
> 
> I will let you take it from here.
> 
> 
> Thanks,
> 
> Aleksandar Ristovski
> QNX Software Systems
> 

Reposting a diff generated with -up.

[-- Attachment #2: mips-tdep.c-info-registers-20090223.diff --]
[-- Type: text/x-patch, Size: 2469 bytes --]

Index: gdb/infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.231
diff -u -p -r1.231 infcmd.c
--- gdb/infcmd.c	25 Jan 2009 23:35:51 -0000	1.231
+++ gdb/infcmd.c	23 Feb 2009 18:21:54 -0000
@@ -1948,21 +1948,6 @@ registers_info (char *addr_exp, int fpre
 	  }
       }
 
-      /* A register number?  (how portable is this one?).  */
-      {
-	char *endptr;
-	int regnum = strtol (start, &endptr, 0);
-	if (endptr == end
-	    && regnum >= 0
-	    && regnum < gdbarch_num_regs (gdbarch)
-			+ gdbarch_num_pseudo_regs (gdbarch))
-	  {
-	    gdbarch_print_registers_info (gdbarch, gdb_stdout,
-					  frame, regnum, fpregs);
-	    continue;
-	  }
-      }
-
       /* A register group?  */
       {
 	struct reggroup *group;
Index: gdb/mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.490
diff -u -p -r1.490 mips-tdep.c
--- gdb/mips-tdep.c	22 Feb 2009 01:02:17 -0000	1.490
+++ gdb/mips-tdep.c	23 Feb 2009 18:21:57 -0000
@@ -440,13 +440,25 @@ mips_register_name (struct gdbarch *gdba
     "t8", "t9", "k0", "k1", "gp", "sp", "s8", "ra"
   };
 
+  static char *mips_gpr_numeric_names[] = {
+    "0", "1", "2", "3", "4", "5", "6", "7",
+    "8", "9", "10", "11", "12", "13", "14", "15",
+    "16", "17", "18", "19", "20", "21", "22", "23",
+    "24", "25", "26", "27", "28", "29", "30", "31"
+  };
+
   enum mips_abi abi = mips_abi (gdbarch);
 
   /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers, 
      but then don't make the raw register names visible.  */
   int rawnum = regno % gdbarch_num_regs (gdbarch);
   if (regno < gdbarch_num_regs (gdbarch))
-    return "";
+    {
+      if (regno >= 0)
+	return mips_gpr_numeric_names [regno];
+      else
+	return "";
+    }
 
   /* The MIPS integer registers are always mapped from 0 to 31.  The
      names of the registers (which reflects the conventions regarding
@@ -4601,7 +4613,9 @@ mips_print_registers_info (struct gdbarc
 {
   if (regnum != -1)		/* do one specified register */
     {
-      gdb_assert (regnum >= gdbarch_num_regs (gdbarch));
+      if (regnum < gdbarch_num_regs (gdbarch)
+	  && regnum >= 0)
+	regnum += gdbarch_num_regs (gdbarch); /* Print pseudo register.  */
       if (*(gdbarch_register_name (gdbarch, regnum)) == '\0')
 	error (_("Not a valid register for the current processor type"));
 

Re: [patch] mips-tdep: info registers

[Joel Brobecker]

Aleksandar,

>Joel, the attached removes numeric value syntax, but allows it for mips; 
>mips will explicitly map regno to raw regno (which is currently 1-1).
>
>I will let you take it from here.

I am afraid that I won't have time to do much more than review your
change. Just reviewing changes right now is a struggle for me, so
do you think you could submit them officially?

I think there are two parts to this patch:

> Index: gdb/infcmd.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infcmd.c,v
> retrieving revision 1.231
> diff -u -p -r1.231 infcmd.c
> --- gdb/infcmd.c	25 Jan 2009 23:35:51 -0000	1.231
> +++ gdb/infcmd.c	23 Feb 2009 18:21:54 -0000
> @@ -1948,21 +1948,6 @@ registers_info (char *addr_exp, int fpre
>  	  }
>        }
>  
> -      /* A register number?  (how portable is this one?).  */
> -      {
> -	char *endptr;
> -	int regnum = strtol (start, &endptr, 0);
> -	if (endptr == end
> -	    && regnum >= 0
> -	    && regnum < gdbarch_num_regs (gdbarch)
> -			+ gdbarch_num_pseudo_regs (gdbarch))
> -	  {
> -	    gdbarch_print_registers_info (gdbarch, gdb_stdout,
> -					  frame, regnum, fpregs);
> -	    continue;
> -	  }
> -      }
> -
>        /* A register group?  */
>        {
>  	struct reggroup *group;

This patch is fine, and approved. Please just run it through the testsuite
before checking in. (can you remember to post a ChangeLog for it as well?)

The mips-tdep part can be treated independently.  Although I don't see
any problem with it, and you updated it the same way I would have changed
it, I'm not sure about going against the comment:

>    /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers, 
>       but then don't make the raw register names visible.  */

It looks fine to return "0" ... "31" as the names of the raw registers,
but I'd like someone with more experience with the mips target to confirm
it.

> Index: gdb/mips-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/mips-tdep.c,v
> retrieving revision 1.490
> diff -u -p -r1.490 mips-tdep.c
> --- gdb/mips-tdep.c	22 Feb 2009 01:02:17 -0000	1.490
> +++ gdb/mips-tdep.c	23 Feb 2009 18:21:57 -0000
> @@ -440,13 +440,25 @@ mips_register_name (struct gdbarch *gdba
>      "t8", "t9", "k0", "k1", "gp", "sp", "s8", "ra"
>    };
>  
> +  static char *mips_gpr_numeric_names[] = {

A little short comment explaining what this is about might be useful.

> +    "0", "1", "2", "3", "4", "5", "6", "7",
> +    "8", "9", "10", "11", "12", "13", "14", "15",
> +    "16", "17", "18", "19", "20", "21", "22", "23",
> +    "24", "25", "26", "27", "28", "29", "30", "31"
> +  };
> +
>    enum mips_abi abi = mips_abi (gdbarch);
>  
>    /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers, 
>       but then don't make the raw register names visible.  */

This comment needs to be removed or updated.  If the comment next to
mips_gpr_numeric_names is clear enough, I think it can simply be
removed.

>    int rawnum = regno % gdbarch_num_regs (gdbarch);
>    if (regno < gdbarch_num_regs (gdbarch))
> -    return "";
> +    {
> +      if (regno >= 0)
> +	return mips_gpr_numeric_names [regno];
> +      else
> +	return "";
> +    }
>  
>    /* The MIPS integer registers are always mapped from 0 to 31.  The
>       names of the registers (which reflects the conventions regarding
> @@ -4601,7 +4613,9 @@ mips_print_registers_info (struct gdbarc
>  {
>    if (regnum != -1)		/* do one specified register */
>      {
> -      gdb_assert (regnum >= gdbarch_num_regs (gdbarch));
> +      if (regnum < gdbarch_num_regs (gdbarch)
> +	  && regnum >= 0)
> +	regnum += gdbarch_num_regs (gdbarch); /* Print pseudo register.  */
>        if (*(gdbarch_register_name (gdbarch, regnum)) == '\0')
>  	error (_("Not a valid register for the current processor type"));
>  


-- 
Joel

Re: [patch] mips-tdep: info registers

[Aleksandar Ristovski]

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

Joel Brobecker wrote:
> I think there are two parts to this patch:
> 
...
> 
> This patch is fine, and approved. Please just run it through the testsuite
> before checking in. (can you remember to post a ChangeLog for it as well?)

Ok, I committed this part. I have had weird issues with 
running testsuite, I ended up running all the test 
one-by-one. (tested on linux).


> 
> The mips-tdep part can be treated independently.  Although I don't see
> any problem with it, and you updated it the same way I would have changed
> it, I'm not sure about going against the comment:
> 
>>    /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers, 
>>       but then don't make the raw register names visible.  */
> 
> It looks fine to return "0" ... "31" as the names of the raw registers,
> but I'd like someone with more experience with the mips target to confirm
> it.

...

I made all suggested changes. New patch is attached.

Note: I tested this on our gdb based on gdb 6.8 sources, 
using our remote protocol to connect to a MIPS target. The 
changes are the same - the code affected by the patch does 
not differ.
On the HEAD gdb sources I did a compile but I could not do a 
  test on the target.


Thanks,

Aleksandar



* mips-tdep.c (mips_register_name): Handle numeric GPR 
register numbers.
(mips_print_registers_info): Remove gdb_assert.


[-- Attachment #2: mips-tdep.c-info-registers-20090304.diff --]
[-- Type: text/x-patch, Size: 1775 bytes --]

Index: gdb/mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.490
diff -u -p -r1.490 mips-tdep.c
--- gdb/mips-tdep.c	22 Feb 2009 01:02:17 -0000	1.490
+++ gdb/mips-tdep.c	4 Mar 2009 21:39:25 -0000
@@ -440,13 +440,27 @@ mips_register_name (struct gdbarch *gdba
     "t8", "t9", "k0", "k1", "gp", "sp", "s8", "ra"
   };
 
+  /* MIPS GPR register numbers, as used by assembler.  Order must
+     reflect gdb's regno<->MIPS register number mapping which is 
+     currently 1-1.  */
+  static char *mips_gpr_numeric_names[] = {
+    "0", "1", "2", "3", "4", "5", "6", "7",
+    "8", "9", "10", "11", "12", "13", "14", "15",
+    "16", "17", "18", "19", "20", "21", "22", "23",
+    "24", "25", "26", "27", "28", "29", "30", "31"
+  };
+
   enum mips_abi abi = mips_abi (gdbarch);
 
-  /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers, 
-     but then don't make the raw register names visible.  */
+  /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers.  */
   int rawnum = regno % gdbarch_num_regs (gdbarch);
   if (regno < gdbarch_num_regs (gdbarch))
-    return "";
+    {
+      if (regno >= 0 && regno < 32)
+	return mips_gpr_numeric_names [regno];
+      else
+	return "";
+    }
 
   /* The MIPS integer registers are always mapped from 0 to 31.  The
      names of the registers (which reflects the conventions regarding
@@ -4601,7 +4615,6 @@ mips_print_registers_info (struct gdbarc
 {
   if (regnum != -1)		/* do one specified register */
     {
-      gdb_assert (regnum >= gdbarch_num_regs (gdbarch));
       if (*(gdbarch_register_name (gdbarch, regnum)) == '\0')
 	error (_("Not a valid register for the current processor type"));
 

Re: [patch] mips-tdep: info registers

[Joel Brobecker]

Hi Daniel,

IIRC, you've worked on the mips target a few times in the past.
Do you forsee any problem if mips_register_name return "0" .. "31"
for the raw GP registers?

What Aleksandar is trying to do is allow "info register 1" to work
on mips, instead of having to use "info register at"...

Thanks!

> * mips-tdep.c (mips_register_name): Handle numeric GPR 
> register numbers.
> (mips_print_registers_info): Remove gdb_assert.


> Index: gdb/mips-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/mips-tdep.c,v
> retrieving revision 1.490
> diff -u -p -r1.490 mips-tdep.c
> --- gdb/mips-tdep.c	22 Feb 2009 01:02:17 -0000	1.490
> +++ gdb/mips-tdep.c	4 Mar 2009 21:39:25 -0000
> @@ -440,13 +440,27 @@ mips_register_name (struct gdbarch *gdba
>      "t8", "t9", "k0", "k1", "gp", "sp", "s8", "ra"
>    };
>  
> +  /* MIPS GPR register numbers, as used by assembler.  Order must
> +     reflect gdb's regno<->MIPS register number mapping which is 
> +     currently 1-1.  */
> +  static char *mips_gpr_numeric_names[] = {
> +    "0", "1", "2", "3", "4", "5", "6", "7",
> +    "8", "9", "10", "11", "12", "13", "14", "15",
> +    "16", "17", "18", "19", "20", "21", "22", "23",
> +    "24", "25", "26", "27", "28", "29", "30", "31"
> +  };
> +
>    enum mips_abi abi = mips_abi (gdbarch);
>  
> -  /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers, 
> -     but then don't make the raw register names visible.  */
> +  /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers.  */
>    int rawnum = regno % gdbarch_num_regs (gdbarch);
>    if (regno < gdbarch_num_regs (gdbarch))
> -    return "";
> +    {

I would like a commenthere explaining why we return the numeric
names for raw registers, and why we only do that for GPRs.

> +      if (regno >= 0 && regno < 32)
> +	return mips_gpr_numeric_names [regno];
> +      else
> +	return "";
> +    }
>  
>    /* The MIPS integer registers are always mapped from 0 to 31.  The
>       names of the registers (which reflects the conventions regarding
> @@ -4601,7 +4615,6 @@ mips_print_registers_info (struct gdbarc
>  {
>    if (regnum != -1)		/* do one specified register */
>      {
> -      gdb_assert (regnum >= gdbarch_num_regs (gdbarch));
>        if (*(gdbarch_register_name (gdbarch, regnum)) == '\0')
>  	error (_("Not a valid register for the current processor type"));
>  

-- 
Joel

Re: [patch] mips-tdep: info registers

[Daniel Jacobowitz]

On Thu, Mar 05, 2009 at 11:12:45AM -0800, Joel Brobecker wrote:
> Hi Daniel,
> 
> IIRC, you've worked on the mips target a few times in the past.
> Do you forsee any problem if mips_register_name return "0" .. "31"
> for the raw GP registers?
> 
> What Aleksandar is trying to do is allow "info register 1" to work
> on mips, instead of having to use "info register at"...
> 
> Thanks!

I think it's weird for them to map to the raw registers.  info
registers displays the ABI-sized registers, so why should these be any
different?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] mips-tdep: info registers

[Aleksandar Ristovski]

Daniel Jacobowitz wrote:
> On Thu, Mar 05, 2009 at 11:12:45AM -0800, Joel Brobecker wrote:
>> Hi Daniel,
>>
>> IIRC, you've worked on the mips target a few times in the past.
>> Do you forsee any problem if mips_register_name return "0" .. "31"
>> for the raw GP registers?
>>
>> What Aleksandar is trying to do is allow "info register 1" to work
>> on mips, instead of having to use "info register at"...
>>
>> Thanks!
> 
> I think it's weird for them to map to the raw registers.  info
> registers displays the ABI-sized registers, so why should these be any
> different?
> 

Bottom line is that mips assembly will have something like this:
      move  $4, $9        # copy contents of $9 into $4

and it would be natural to be able to print value of $9.

My patch may have not implemented this correctly, maybe I 
still need to return pseudo registers, but the numeric name 
should be recognized in any case.

Re: [patch] mips-tdep: info registers

[Daniel Jacobowitz]

Sorry, missed reply-to-all.

On Thu, Mar 05, 2009 at 02:26:55PM -0500, Aleksandar Ristovski wrote:
> Bottom line is that mips assembly will have something like this:
>      move  $4, $9        # copy contents of $9 into $4
>
> and it would be natural to be able to print value of $9.
>
> My patch may have not implemented this correctly, maybe I still need to 
> return pseudo registers, but the numeric name should be recognized in any 
> case.

They should be aliases for the normal registers, e.g. $1 and $at
should mean the same thing.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] mips-tdep: info registers

[Joel Brobecker]

Daniel,

Thanks for taking a look - I knew there was a reason I wasn't
comfortable with this approach...

> They should be aliases for the normal registers, e.g. $1 and $at
> should mean the same thing.

I have never implemented something like this before. How would you
do it? I see that mips-tdep.c already has some table of register
name aliases. This doesn't look like it can be connected to
the "info registers" command, because of the way this command
is implemented (it iterates over all register numbers, gets their
associated names, and compares with the name used by the user).
As far as I can tell, the only way that I can see is by adding
a new series of pseudo-registers.

-- 
Joel

Re: [patch] mips-tdep: info registers

[Daniel Jacobowitz]

On Thu, Mar 05, 2009 at 02:50:30PM -0800, Joel Brobecker wrote:
> > They should be aliases for the normal registers, e.g. $1 and $at
> > should mean the same thing.
> 
> I have never implemented something like this before. How would you
> do it? I see that mips-tdep.c already has some table of register
> name aliases. This doesn't look like it can be connected to
> the "info registers" command, because of the way this command
> is implemented (it iterates over all register numbers, gets their
> associated names, and compares with the name used by the user).
> As far as I can tell, the only way that I can see is by adding
> a new series of pseudo-registers.

Or change how info registers works, perhaps.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] mips-tdep: info registers

[Aleksandar Ristovski]

oops... now I missed reply-to-all...

Daniel Jacobowitz wrote:
> On Thu, Mar 05, 2009 at 02:50:30PM -0800, Joel Brobecker wrote:
>>> They should be aliases for the normal registers, e.g. $1 and $at
>>> should mean the same thing.
>> I have never implemented something like this before. How would you
>> do it? I see that mips-tdep.c already has some table of register
>> name aliases. This doesn't look like it can be connected to
>> the "info registers" command, because of the way this command
>> is implemented (it iterates over all register numbers, gets their
>> associated names, and compares with the name used by the user).
>> As far as I can tell, the only way that I can see is by adding
>> a new series of pseudo-registers.
> 
> Or change how info registers works, perhaps.
> 

Maybe adding gdbarch_regname_to_regnum or something like 
that (which would avoid iterating through the registers and 
comparing returned register names).

Re: [patch] mips-tdep: info registers

[Daniel Jacobowitz]

On Fri, Mar 06, 2009 at 12:00:31AM -0500, Aleksandar Ristovski wrote:
> Maybe adding gdbarch_regname_to_regnum or something like that (which would 
> avoid iterating through the registers and comparing returned register 
> names).

Let's keep optimization separate from functionality; there's a
function in user-regs.h to do the lookup and if it's too slow it can
be converted to use a hash table.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] mips-tdep: info registers

[Aleksandar Ristovski]

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

Daniel Jacobowitz wrote:
> On Fri, Mar 06, 2009 at 12:00:31AM -0500, Aleksandar Ristovski wrote:
>> Maybe adding gdbarch_regname_to_regnum or something like that (which would 
>> avoid iterating through the registers and comparing returned register 
>> names).
> 
> Let's keep optimization separate from functionality; there's a
> function in user-regs.h to do the lookup and if it's too slow it can
> be converted to use a hash table.
> 

I didn't really think about optimization, but I did not 
understand what/how user-regs is being used. Now I realize 
that is the mechanism for adding register aliases.

Here is another try.

(again, tested on in-house gdb variant).

ChangeLog:


* mips-tdep.c (mips_numeric_register_alieses): New definition.
(mips_gdbarch_init): Add user registers from
mips_numeric_register_aliases.

[-- Attachment #2: mips-tdep.c-numeric-register-names-20090306.diff --]
[-- Type: text/x-patch, Size: 1242 bytes --]

Index: gdb/mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.490
diff -u -p -r1.490 mips-tdep.c
--- gdb/mips-tdep.c	22 Feb 2009 01:02:17 -0000	1.490
+++ gdb/mips-tdep.c	6 Mar 2009 14:43:42 -0000
@@ -143,6 +143,15 @@ const struct register_alias mips_registe
   { "fsr", MIPS_EMBED_FP0_REGNUM + 32 }
 };
 
+const struct register_alias mips_numeric_register_aliases[] = {
+#define R(n) { #n, n }
+  R(0), R(1), R(2), R(3), R(4), R(5), R(6), R(7),
+  R(8), R(9), R(10), R(11), R(12), R(13), R(14), R(15),
+  R(16), R(17), R(18), R(19), R(20), R(21), R(22), R(23),
+  R(24), R(25), R(26), R(27), R(28), R(29), R(30), R(31),
+#undef R
+};
+
 #ifndef MIPS_DEFAULT_FPU_TYPE
 #define MIPS_DEFAULT_FPU_TYPE MIPS_FPU_DOUBLE
 #endif
@@ -6058,6 +6067,11 @@ mips_gdbarch_init (struct gdbarch_info i
     user_reg_add (gdbarch, mips_register_aliases[i].name,
 		  value_of_mips_user_reg, &mips_register_aliases[i].regnum);
 
+  for (i = 0; i < ARRAY_SIZE (mips_numeric_register_aliases); i++)
+    user_reg_add (gdbarch, mips_numeric_register_aliases[i].name,
+		  value_of_mips_user_reg, 
+		  &mips_numeric_register_aliases[i].regnum);
+
   return gdbarch;
 }
 

Re: [patch] mips-tdep: info registers

[Joel Brobecker]

Aleksandar,

(sorry for the late review)

> * mips-tdep.c (mips_numeric_register_alieses): New definition.
> (mips_gdbarch_init): Add user registers from
> mips_numeric_register_aliases.

This looks fine. Please go ahead and check it in.

-- 
Joel

Re: [patch] mips-tdep: info registers

[Aleksandar Ristovski]

Joel Brobecker wrote:
> Aleksandar,
> 
> (sorry for the late review)
> 
>> * mips-tdep.c (mips_numeric_register_alieses): New definition.
>> (mips_gdbarch_init): Add user registers from
>> mips_numeric_register_aliases.
> 
> This looks fine. Please go ahead and check it in.
> 

Done.

Thank you,

Aleksandar Ristovski
QNX Software Systems

Re: [patch] mips-tdep: info registers

[Maciej W. Rozycki]

On Mon, 23 Feb 2009, Aleksandar Ristovski wrote:

> Joel, the attached removes numeric value syntax, but allows it for mips; mips
> will explicitly map regno to raw regno (which is currently 1-1).

 Is it really 1-1?  I smell it may not be the case for the FP registers 
(which are laid out differently depending on the FPU type and its current 
configuration), but it might just be my memory playing tricks on me...

  Maciej

Re: [patch] mips-tdep: info registers

[Aleksandar Ristovski]

Maciej W. Rozycki wrote:
> On Mon, 23 Feb 2009, Aleksandar Ristovski wrote:
> 
>> Joel, the attached removes numeric value syntax, but allows it for mips; mips
>> will explicitly map regno to raw regno (which is currently 1-1).
> 
>  Is it really 1-1?  I smell it may not be the case for the FP registers 
> (which are laid out differently depending on the FPU type and its current 
> configuration), but it might just be my memory playing tricks on me...
> 
>   Maciej
> 

This particular patch I proposed deals only with gpr-s. 
Therefore, it will take care of

info registers 0
to
info registers 31

Outside of that, works as before.

Re: [patch] mips-tdep: info registers

[Maciej W. Rozycki]

On Tue, 24 Feb 2009, Aleksandar Ristovski wrote:

> This particular patch I proposed deals only with gpr-s. Therefore, it will
> take care of
> 
> info registers 0
> to
> info registers 31
> 
> Outside of that, works as before.

 OK then. :)

  Maciej