[PATCH] Correct irix5-nat.c's fetch_core_registers

[PATCH] Correct irix5-nat.c's fetch_core_registers

[Roger Sayle]

On my mips-sgi-irix6.5 box, attempting to read a "core" file on all
versions of gdb since v6.0, result in diagnostic message "wrong
size gregset struct in core file", though gdb v5.3 works fine.

The problem appears to be related to the transition to using regcache.
Where back in v5.3, core_reg_size was initially tested against
REGISTER_BYTES, which on MIPS IRIX was defined in tm.h as:

(MIPS_NUMREGS * 8 + (NUM_REGS - MIPS_NUMREGS) * MIPS_REGSIZE)

which used to evaluate to 568, the code in mainline now instead
tests against deprecated_register_bytes () which currently evaluates
to 1128!?  Investigating deprecated_register_bytes() currently
returns current_regcache->descr->sizeof_raw_registers whose
calculation has a FIXME next to it, regcache.c:140, stating that
in 2002 it switched to using sizeof_cooked_registers.  Hence
instead of 71*8 that it originally returned, it now returns 141*8,
which breaks the logic in irix5-nat.c.

The clean-up below tidies this up.  Alas other breakage (bit rot)
on MIPS IRIX still prevents reading "core" files from working,
however a back-port of this change to gdb-6.0 allows 6.0 to read
core files without problems.

The following patch has been tested against mainline CVS on
mips-sgi-irix6.5 (together with yesterday's patch to resolve the
build issues).  Attempting to read a core file now procedes past
the previous "wrong size gregset" error messages.  Instead I'm
now only getting "You can't do that without a process to debug."
failures due to push_target vs. irix weirdness.  [Any help on
how things are supposed to work would be appreciated].

Ok for mainline?  As with yesterday's post, I'd appreciate it if
someone could commit this change for me.  Many thanks in advance.



2006-07-21  Roger Sayle  <roger@eyesopen.com>

	* irix5-nat.c (fetch_core_registers): Fix logic error caused by
	the transition to or changes in deprecated_register_bytes.


Index: irix5-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/irix5-nat.c,v
retrieving revision 1.40
diff -c -3 -p -r1.40 irix5-nat.c
*** irix5-nat.c	17 Dec 2005 22:34:01 -0000	1.40
--- irix5-nat.c	21 Jul 2006 16:10:33 -0000
***************
*** 1,7 ****
  /* Native support for the SGI Iris running IRIX version 5, for GDB.

     Copyright (C) 1988, 1989, 1990, 1991, 1992, 1993, 1994, 1995, 1996,
!    1998, 1999, 2000, 2001, 2002, 2004 Free Software Foundation, Inc.

     Contributed by Alessandro Forin(af@cs.cmu.edu) at CMU
     and by Per Bothner(bothner@cs.wisc.edu) at U.Wisconsin.
--- 1,7 ----
  /* Native support for the SGI Iris running IRIX version 5, for GDB.

     Copyright (C) 1988, 1989, 1990, 1991, 1992, 1993, 1994, 1995, 1996,
!    1998, 1999, 2000, 2001, 2002, 2004, 2006 Free Software Foundation, Inc.

     Contributed by Alessandro Forin(af@cs.cmu.edu) at CMU
     and by Per Bothner(bothner@cs.wisc.edu) at U.Wisconsin.
*************** fetch_core_registers (char *core_reg_sec
*** 244,259 ****
    char *srcp = core_reg_sect;
    int regno;

!   if (core_reg_size == deprecated_register_bytes ())
      {
        for (regno = 0; regno < NUM_REGS; regno++)
          {
            regcache_raw_write (current_regcache, regno, srcp);
!           srcp += register_size (current_gdbarch, regno);
          }
      }
!   else if (mips_isa_regsize (current_gdbarch) == 4 &&
! 	   core_reg_size == (2 * mips_isa_regsize (current_gdbarch)) * NUM_REGS)
      {
        /* This is a core file from a N32 executable, 64 bits are saved
           for all registers.  */
--- 244,264 ----
    char *srcp = core_reg_sect;
    int regno;

!   if (core_reg_size != NUM_REGS * 8)
!     {
!       warning (_("wrong size gregset struct in core file"));
!       return;
!     }
!
!   if (mips_isa_regsize (current_gdbarch) == 8)
      {
        for (regno = 0; regno < NUM_REGS; regno++)
          {
            regcache_raw_write (current_regcache, regno, srcp);
!           srcp += 8;
          }
      }
!   else /* mips_isa_regsize (current_gdbarch) == 4 */
      {
        /* This is a core file from a N32 executable, 64 bits are saved
           for all registers.  */
*************** fetch_core_registers (char *core_reg_sec
*** 270,280 ****
            srcp += 8;
  	}
      }
-   else
-     {
-       warning (_("wrong size gregset struct in core file"));
-       return;
-     }
  }

  /* Register that we are able to handle irix5 core file formats.
--- 275,280 ----


Roger
--

Re: [PATCH] Correct irix5-nat.c's fetch_core_registers

[Daniel Jacobowitz]

On Fri, Jul 21, 2006 at 10:12:10AM -0600, Roger Sayle wrote:
> !   else if (mips_isa_regsize (current_gdbarch) == 4 &&
> ! 	   core_reg_size == (2 * mips_isa_regsize (current_gdbarch)) * NUM_REGS)

> !   else /* mips_isa_regsize (current_gdbarch) == 4 */
>       {
>         /* This is a core file from a N32 executable, 64 bits are saved
>            for all registers.  */

This change doesn't make sense to me.  If I'm missing something, could
you try to explain it again?

As far as I remember, Irix supports O32 executables.  So at a minimum
the comment is wrong.  I don't know if it dumps 32-bit or 64-bit
registers for O32 core files; I wouldn't be too surprised if it dumped
64-bit core files and just the comment needed fixing.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Correct irix5-nat.c's fetch_core_registers

[Roger Sayle]

Hi Daniel,

Many thanks for the review.

On Mon, 24 Jul 2006, Daniel Jacobowitz wrote:
> On Fri, Jul 21, 2006 at 10:12:10AM -0600, Roger Sayle wrote:
> > !   else if (mips_isa_regsize (current_gdbarch) == 4 &&
> > ! 	   core_reg_size == (2 * mips_isa_regsize (current_gdbarch)) * NUM_REGS)
>
> > !   else /* mips_isa_regsize (current_gdbarch) == 4 */
> >       {
> >         /* This is a core file from a N32 executable, 64 bits are saved
> >            for all registers.  */
>
> This change doesn't make sense to me.  If I'm missing something, could
> you try to explain it again?

The current implementation in fetch_core_registers has two main
clauses.  The first which currently reads "core_reg_size ==
deprecated_register_bytes ()" really expects deprecated_register_bytes
to return 568, or 8*NUM_REGS are explained in the original post.
The second clause checks whether "mips_isa_regsize (current_gdbarch) == 4"
and then whether
  core_reg_size == (2 * mips_isa_regsize (current_gdbarch)) * NUM_REGS

A quick bit of CSE, constant-folding and mental arithmetic, reveals
that this is equivalent to 8*NUM_REGS, or 568 bytes.  Hence, it looks
like GDB has only ever supported core files from ABIs that use 568
bytes to store all 71 MIPS registers.

Once this is grasped, the code can be cleaned up.  We check at the
top of the function whether the core_reg_size has the correct value,
and then use one of two implementations to extract the actual
register values based upon whether mips_isa_regsize is 4 or 8.
When mips_isa_regsize is 8 everything is simple, otherwise the
existing code selects whether the register is stored in the high-end
or low-end of the 8 bytes stored on disk (depending upon whether
the register is integer or floating point).


> As far as I remember, Irix supports O32 executables.  So at a minimum
> the comment is wrong.  I don't know if it dumps 32-bit or 64-bit
> registers for O32 core files; I wouldn't be too surprised if it dumped
> 64-bit core files and just the comment needed fixing.

Ok, I've just done some experimentation, and it looks like GDB has
never supported O32 core files.  Not only do I get the error message
"warning: wrong size gregset struct in core file" with all releases
of GDB since v6.0 (as I do with N32 core files), but I also get the
same failure with gdb v5.3.  So N32 core files have been broken
since 2002, but O32 core files have been broken for much longer or
never supported.

Turns out the code in fetch_core_registers only supports N32/N64,
and this comment is in a section of code that can only ever be
reached for N32.  Infact, its an N32 core file that I'm trying to
debug, but because I'm running gdb on a 64-bit mips4 host, GDB's
mips_isa_regsize returns 8.  Hence, its the first clause that needs
to be fixed for me.

The good news is I think I may be able to get O32 to work.  Turns
out in O32 core files, core_reg_size=284 (which is conveniently
4*NUM_REGS).  It also looks like when working on an O32 executable,
mips_isa_regsize is 4 (even on my MIPS4 box).  So this aspect of
the logic seems simple enough, even though I'm not sure how much
other support would need to be added to GDB to finish an O32 port.


Of course, it's academic at the moment as GDB on IRIX is still very
broken; I still get "You can't do that without a process to debug"
error messages long before the code ever gets to fetch_core_registers
when attempting to read an O32 executable and core file, and I need
to apply my other "cast" patch to just get the tree to build!


Many thanks again for looking at this patch.  Much appreciated.

Roger
--

Re: [PATCH] Correct irix5-nat.c's fetch_core_registers

[Daniel Jacobowitz]

On Mon, Jul 24, 2006 at 03:03:23PM -0600, Roger Sayle wrote:
> The current implementation in fetch_core_registers has two main
> clauses.  The first which currently reads "core_reg_size ==
> deprecated_register_bytes ()" really expects deprecated_register_bytes
> to return 568, or 8*NUM_REGS are explained in the original post.

This is the part at which your logic breaks down.  Remember this one
for a moment...

> The good news is I think I may be able to get O32 to work.  Turns
> out in O32 core files, core_reg_size=284 (which is conveniently
> 4*NUM_REGS).  It also looks like when working on an O32 executable,
> mips_isa_regsize is 4 (even on my MIPS4 box).  So this aspect of
> the logic seems simple enough, even though I'm not sure how much
> other support would need to be added to GDB to finish an O32 port.

...and then look at this.  At one point, at least, the first branch was
taken for both o32 and n64.  If you look at the code in that branch,
this seems fairly sensible.

What is probably confusing you is the mips_isa_regsize bits.  They're
much more recent than the rest of the code, so don't read too much into
them; and they were added mechanically.

Registers 0 - NUM_REGS are represented in the register cache as having
mips_isa_regsize bytes, which is not necessarily the same as the ABI
regsize.  You can just accomodate that here.  If the core register set
size matches the ISA register size, just read them in.  It should
match.  You can check and warn if they don't.

IOW I think the entire else branch is dead; it reads in four bytes of
N32 64-bit registers.  Not very useful!


-- 
Daniel Jacobowitz
CodeSourcery

[PATCH] Correct irix5-nat.c's fetch_core_registers (take 2)

[Roger Sayle]

On Mon, 24 Jul 2006, Daniel Jacobowitz wrote:
> If the core register set size matches the ISA register size, just read
> them in.  It should match.  You can check and warn if they don't.
> IOW I think the entire else branch is dead; it reads in four bytes of
> N32 64-bit registers.  Not very useful!

Try as I might, even resurecting a long dead SGI O2 and trying to use
the command "set mips saved-gpreg-size 32" to spoof the mips_isa_regsize
I couldn't get the else branch to trigger, so I think you're right that
this code is long dead.  Well, that greatly simplifies things!  The patch
below follows your suggestions and should additionally fix O32.

The following revised patch has been tested by building mainline gdb
on mips-sgi-irix6.5.  Unfortunately, as explained in my original post,
loading shared libraries on IRIX is broken, but an earlier version of
this patch cured to "wrong size gregset" errors when applied to older
versions of GDB.

Thanks again for your help.


2006-07-25  Roger Sayle  <roger@eyesopen.com>
	    Daniel Jacobowitz  <dan@codesourcery.com>

	* irix5-nat.c (fetch_core_registers): Simplify and correct logic.


Index: irix5-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/irix5-nat.c,v
retrieving revision 1.40
diff -c -3 -p -r1.40 irix5-nat.c
*** irix5-nat.c	17 Dec 2005 22:34:01 -0000	1.40
--- irix5-nat.c	25 Jul 2006 05:52:11 -0000
***************
*** 1,7 ****
  /* Native support for the SGI Iris running IRIX version 5, for GDB.

     Copyright (C) 1988, 1989, 1990, 1991, 1992, 1993, 1994, 1995, 1996,
!    1998, 1999, 2000, 2001, 2002, 2004 Free Software Foundation, Inc.

     Contributed by Alessandro Forin(af@cs.cmu.edu) at CMU
     and by Per Bothner(bothner@cs.wisc.edu) at U.Wisconsin.
--- 1,7 ----
  /* Native support for the SGI Iris running IRIX version 5, for GDB.

     Copyright (C) 1988, 1989, 1990, 1991, 1992, 1993, 1994, 1995, 1996,
!    1998, 1999, 2000, 2001, 2002, 2004, 2006 Free Software Foundation, Inc.

     Contributed by Alessandro Forin(af@cs.cmu.edu) at CMU
     and by Per Bothner(bothner@cs.wisc.edu) at U.Wisconsin.
*************** fetch_core_registers (char *core_reg_sec
*** 242,280 ****
  		      int which, CORE_ADDR reg_addr)
  {
    char *srcp = core_reg_sect;
    int regno;

!   if (core_reg_size == deprecated_register_bytes ())
!     {
!       for (regno = 0; regno < NUM_REGS; regno++)
!         {
!           regcache_raw_write (current_regcache, regno, srcp);
!           srcp += register_size (current_gdbarch, regno);
!         }
!     }
!   else if (mips_isa_regsize (current_gdbarch) == 4 &&
! 	   core_reg_size == (2 * mips_isa_regsize (current_gdbarch)) * NUM_REGS)
!     {
!       /* This is a core file from a N32 executable, 64 bits are saved
!          for all registers.  */
!       for (regno = 0; regno < NUM_REGS; regno++)
! 	{
! 	  if (regno >= FP0_REGNUM && regno < (FP0_REGNUM + 32))
! 	    {
!               regcache_raw_write (current_regcache, regno, srcp);
! 	    }
! 	  else
! 	    {
!               regcache_raw_write (current_regcache, regno, srcp + 4);
! 	    }
!           srcp += 8;
! 	}
!     }
!   else
      {
        warning (_("wrong size gregset struct in core file"));
        return;
      }
  }

  /* Register that we are able to handle irix5 core file formats.
--- 242,263 ----
  		      int which, CORE_ADDR reg_addr)
  {
    char *srcp = core_reg_sect;
+   int regsize = mips_isa_regsize (current_gdbarch);
    int regno;

!   /* If regsize is 8, this is a N32 or N64 core file.
!      If regsize is 4, this is an O32 core file.  */
!   if (core_reg_size != regsize * NUM_REGS)
      {
        warning (_("wrong size gregset struct in core file"));
        return;
      }
+
+   for (regno = 0; regno < NUM_REGS; regno++)
+     {
+       regcache_raw_write (current_regcache, regno, srcp);
+       srcp += regsize;
+     }
  }

  /* Register that we are able to handle irix5 core file formats.

Roger
--

Re: [PATCH] Correct irix5-nat.c's fetch_core_registers (take 2)

[Daniel Jacobowitz]

On Tue, Jul 25, 2006 at 09:59:38AM -0600, Roger Sayle wrote:
> 2006-07-25  Roger Sayle  <roger@eyesopen.com>
> 	    Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* irix5-nat.c (fetch_core_registers): Simplify and correct logic.

Thank you; checked in.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Correct irix5-nat.c's fetch_core_registers

[David Anderson]

Daniel Jacobowitz writes:
>As far as I remember, Irix supports O32 executables.  So at a minimum
>the comment is wrong.  I don't know if it dumps 32-bit or 64-bit
>registers for O32 core files;

All IRIX o32 core files record 32 bit register values.   
(n32 and 64 core files record 64 bit register values.) 


David Anderson (davea at sgi dot com)