Re: [patch] mips-tdep: info registers

[Joel Brobecker <brobecker@adacore.com>]

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