Re: [WIP/RFC] MIPS registers overhaul

[Andrew Cagney <ac131313@redhat.com>]

> The patch below represents the results of trying to fix two problems
> related to floating point registers, stumbling at various points along
> the way, and realizing that I needed to fix even more...
> 
> The two problems related to floating point registers that I started out
> trying to fix are the following:
> 
>  1) On mips64, it's not possible to set a floating point register to
>     a single precision value.

Hmm, ``oops''.

>  2) Again, on mips64, when running a program using the o32 ABI, floating
>     point doubles were not being fetched from GDB's regcache correctly.
>     For o32, floating point doubles are represented using register pairs,
>     but, for mips64, each register is stored in a 64-bit container.  When
>     attempting to retrieve the value of a 64-bit double that should
>     be retrieved by taking 32-bits from a pair, the single 64-bit container
>     was being considered to be the value of the double.

For a long long time people were trying to solve this using the 
convertible methods.  It never really worked.

> Problem 1 was solved by introducing a union type for floating point
> registers.  When attempting to display a value using ``print'', you'll
> see (with my patch) something like this:
> 
>     (gdb) p $f20
>     $1 = {i = 4621199872640208077, f = -107374184, d = 8.9000000000000004}
> 
> (If someone can think of more meaningful, but still terse field names for
> the above, please let me know.)

I'd try to be consistent with the other register unions, uint64 for 
instance.  As for the d/f, I don't know.

> Problem 2 was solved by introducing pseudo-registers for the floating point
> registers.  Along the way (last Friday, I think), I ran into an assertion
> failure which caused me to make even more extensive changes to the MIPS code.

Ya!  Finally!

> BTW, the raw floating point registers are still accessible.  Doing
> "info registers raw" will display all of the raw registers.  Or, if
> you know the names of the registers you want to display, you can do,
> e.g, "info registers raw_f20".

Hmm, is this necessary?  Confusing?  ``maint print 
{raw-,cooked-,}registers'' are already available and provide access to 
the underlying values.

``info registers'' should always display the target's underlying 
register set.  In the case of o32 running on a 64 bit ISA, the 64 bit 
registers should be displayed.

> Which reminds me...  I've introduced two sets of register numbers.  I
> called these the "cooked" and "raw" regnums.  In many cases, the
> cooked and raw numbers are the same.  (They'll usually be the same
> when the raw and cooked types are the same.) In the cases where the
> numbers are different, the cooked number is a pseudo register number
> and the corresponding raw register is (usually) used to determine the
> value of cooked value.  Raw registers numbers should be used by the
> lower layers of GDB which communicate with the target and need to
> populate GDB's regcache.  The cooked registers *should* be used almost
> everywhere else.  Except they aren't at the moment.  In particular,
> most of mips-tdep.c wants to still operate on "raw" register data. 
> (Anywhere you see a byte order check, that's a clue that raw register
> numbers should still be used.) So, in order to prevent this patch from
> becoming even larger than it already is, much of mips-tdep.c still
> uses raw register numbers.  These should (probably) be cleaned up some
> day to use cooked numbers instead.  (I need to check to make sure I
> have this as a comment in the code somewhere...)
> 
> For the o32 case that I was originally trying to fix, we end up
> creating 16 64-bit cooked floating point registers of the union type
> mentioned above.  The raw floating point registers (in pairs) are used
> to construct the cooked value.  It's also possible to end up with no
> cooked floating point registers or 32-floating floating point
> registers, either 32- or 64-bit.  (The values of FP_REGISTER_DOUBLE
> and MIPS_FPU_TYPE influence what exactly gets done.)

Ok.

> Index: mdebugread.c

The below isn't right.

RA_REGNUM is defined by both Alpha and MIPS.  There are a number of long 
standing problems with mdebug read vis:
tm-alpha.h:64:#define mips_extra_func_info alpha_extra_func_info
and hard-wiring the reference to RA_REGNUM just wouldn't work.

I'd leave RA_REGNUM as is (or put the definition in tm-mips*.h so that 
it is clear that it still needs to be fixed).

> ===================================================================
> RCS file: /cvs/src/src/gdb/mdebugread.c,v
> retrieving revision 1.44
> diff -u -p -r1.44 mdebugread.c
> --- mdebugread.c	19 Mar 2003 19:45:49 -0000	1.44
> +++ mdebugread.c	9 May 2003 23:36:32 -0000
> @@ -55,6 +55,9 @@
>  #include "gdb_assert.h"
>  #include "block.h"
>  
> +#include "mips-tdep.h"
> +#define RA_REGNUM (mips_cooked_regnums (current_gdbarch)->ra_regnum)
> +
>  /* These are needed if the tm.h file does not contain the necessary
>     mips specific definitions.  */
>  
> @@ -69,9 +72,6 @@ typedef struct mips_extra_func_info
>      PDR pdr;
>    }
>   *mips_extra_func_info_t;
> -#ifndef RA_REGNUM
> -#define RA_REGNUM 0
> -#endif
>  #endif
>  
>  #ifdef USG


> Index: mips-tdep.h

Should there be separate raw and cooked num structures?

> +struct mips_regnums
> +  {
> +    int fp0_regnum;		/* First floating point register.  */
> +    int fplast_regnum;		/* Last floating point register.  */
> +    int last_arg_regnum;	/* Last general purpose register used for
> +    				   passing arguments.  (a0_regnum is the
> +				   first.)  */
> +    int first_fp_arg_regnum;	/* First floating point register used for
> +    				   passing floating point arguments.  */
> +    int last_fp_arg_regnum;	/* Last floating point register used for
> +    				   passing floating point arguments.  */
> +    int zero_regnum;		/* The zero register; read-only, always 0.  */
> +    int v0_regnum;		/* Function return value.  */
> +    int a0_regnum;		/* First GPR used for passing arguments.  */
> +    int t9_regnum;		/* Contains address of callee in PIC code.  */
> +    int sp_regnum;		/* Stack pointer.  */
> +    int ra_regnum;		/* Return address.  */
> +    int ps_regnum;		/* Processor status.  */
> +    int hi_regnum;		/* High portion of internal multiply/divide
> +				   register.  */
> +    int lo_regnum;		/* Low portion of internal multiply/divide
> +    				   register.  */
> +    int badvaddr_regnum;	/* Address associated with
> +    				   addressing exception.  */
> +    int cause_regnum;		/* Describes last exception.  */
> +    int pc_regnum;		/* Program counter.  */
> +    int fcrcs_regnum;		/* FP control/status.  */
> +    int fcrir_regnum;		/* FP implementation/revision.  */
> +    int first_embed_regnum;	/* First CP0 register for embedded use.  */
> +    int last_embed_regnum;	/* Last CP0 register for embedded use.  */
> +    int prid_regnum;		/* Processor ID.  */
> +  };
> +
> +const struct mips_regnums *mips_raw_regnums (struct gdbarch *gdbarch);
> +const struct mips_regnums *mips_cooked_regnums (struct gdbarch *gdbarch);

or at least keep things like "last_arg_regnum" out this space (they only 
belong in one of the two spaces).  Having them appear in both makes it 
too easy to do the wrong thing.

Can I suggest for this code:

>  mips_linux_cannot_fetch_register (int regno)
>  {
> +  const struct mips_regnums *regnums = mips_raw_regnums (current_gdbarch);

use a consistent nameing schema that makes the register's space clear vis:

	rawnums = mips_raw_regnums (...);
	...
	if (regno == rawnums->ps_regnum)

it should be possible to apply separate patches that:

	- add mips_raw_regnums() and their initialization
	- roll out the mechanical change
	s/BADVADDR_REGNUM/rawnums->badvaddr_regnum/

How do you know that the raw register numbers were computed correctly?

--

-#define REGISTER_PTRACE_ADDR(regno) \
+register_ptrace_addr (int regno)

is obvious (and separate also).

--

Make certain you use "mips-tdep.h" and not <mips-tdep.h>.

--

Delete the lsi33k support as Stan noted, it likely never worked 
(separate also).

--

This should greatly reduce the diff to just the change adding the 
cooked<->raw map, and code using it.

Andrew