[rfa/ppc/branch too] Fix PowerPC/Linux cores

[rfa/ppc/branch too] Fix PowerPC/Linux cores

[Daniel Jacobowitz]

I figure it would be nice if core files for Linux/PPC worked on the branch. 
Linux/PPC doesn't have gregset_t or fpregset_t in its headers, so the body
of fetch_core_registers in core-regset.c gets #if'd out by autoconf.  Cores
load but are absolutely useless.  I have the feeling those #if's ought to be
outside the function rather than inside... but in any case, for now, this
patch fixes it the same way most other Linux targets do.  I'll get back to
my rework of core support in the next few weeks now that we've branched.

OK to commit, trunk and branch?

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer



2001-07-30  Daniel Jacobowitz  <drow@mvista.com>

	* config/powerpc/linux.mh (NATDEPFILES): Remove core-regset.o.
	* ppc-linux-nat.c (fetch_core_registers): New function.
	(regset_core_fns): New structure.
	(_initialize_ppc_linux_nat): New function.

Index: config/powerpc/linux.mh
===================================================================
RCS file: /cvs/src/src/gdb/config/powerpc/linux.mh,v
retrieving revision 1.6
diff -u -r1.6 linux.mh
--- gdb/gdb/config/powerpc/linux.mh	2000/10/30 22:33:32	1.6
+++ gdb/gdb/config/powerpc/linux.mh	2001/07/30 19:30:59
@@ -6,7 +6,7 @@
 
 NAT_FILE= nm-linux.h
 NATDEPFILES= infptrace.o inftarg.o fork-child.o corelow.o \
-core-aout.o core-regset.o ppc-linux-nat.o proc-service.o thread-db.o lin-lwp.o
+core-aout.o ppc-linux-nat.o proc-service.o thread-db.o lin-lwp.o
 
 LOADLIBES = -ldl -rdynamic
 
Index: ppc-linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/ppc-linux-nat.c,v
retrieving revision 1.9
diff -u -r1.9 ppc-linux-nat.c
--- gdb/gdb/ppc-linux-nat.c	2001/07/05 23:22:04	1.9
+++ gdb/gdb/ppc-linux-nat.c	2001/07/30 19:44:38
@@ -126,3 +126,57 @@
         }
     }
 }
+
+/*  Use a local version of this function to get the correct types for
+    regsets, until multi-arch core support is ready.  */
+
+static void
+fetch_core_registers (char *core_reg_sect, unsigned core_reg_size,
+		      int which, CORE_ADDR reg_addr)
+{
+  elf_gregset_t gregset;
+  elf_fpregset_t fpregset;
+
+  if (which == 0)
+    {
+      if (core_reg_size != sizeof (gregset))
+	{
+	  warning ("wrong size gregset struct in core file");
+	}
+      else
+	{
+	  memcpy ((char *) &gregset, core_reg_sect, sizeof (gregset));
+	  supply_gregset (&gregset);
+	}
+    }
+  else if (which == 2)
+    {
+      if (core_reg_size != sizeof (fpregset))
+	{
+	  warning ("wrong size fpregset struct in core file");
+	}
+      else
+	{
+	  memcpy ((char *) &fpregset, core_reg_sect, sizeof (fpregset));
+	  supply_fpregset (&fpregset);
+	}
+    }
+}
+
+/* Register that we are able to handle ELF file formats using standard
+   procfs "regset" structures.  */
+
+static struct core_fns regset_core_fns =
+{
+  bfd_target_elf_flavour,		/* core_flavour */
+  default_check_format,			/* check_format */
+  default_core_sniffer,			/* core_sniffer */
+  fetch_core_registers,			/* core_read_registers */
+  NULL					/* next */
+};
+
+void
+_initialize_ppc_linux_nat (void)
+{
+  add_core_fns (&regset_core_fns);
+}

Re: [rfa/ppc/branch too] Fix PowerPC/Linux cores

[Kevin Buettner]

On Jul 30,  2:23pm, Daniel Jacobowitz wrote:

> I figure it would be nice if core files for Linux/PPC worked on the branch. 

It would be nice.  It even used to work.  :-(

> Linux/PPC doesn't have gregset_t or fpregset_t in its headers, so the body
> of fetch_core_registers in core-regset.c gets #if'd out by autoconf.  Cores
> load but are absolutely useless.  I have the feeling those #if's ought to be
> outside the function rather than inside...

You may be right.  The other alternative that occurs to me is to put
a #else branch in with a call to internal_error().  (It's a choice
between not linking vs. not running and it's not clear which way the
tradeoff should go.  Personally, when I'm doing a new baseport, I'd
rather have it link with the understanding that the functionality may
be limited...)

> but in any case, for now, this
> patch fixes it the same way most other Linux targets do.  I'll get back to
> my rework of core support in the next few weeks now that we've branched.
> 
> OK to commit, trunk and branch?

I think I'd like to explore some alternatives first.  The only
real difference that I see between the version of fetch_core_registers()
that you want to add to ppc-linux-nat.c and the one in core-regset.c is
that the former uses elf_{g,fp}regset_t whereas the latter uses
{g,fp}regset_t, right?

If that's the case, couldn't we simply make the core-regset.c version
use gdb_gregset_t and gdb_fpgregset_t?  That seems cleaner to me than
propagating nearly identical copies of fetch_core_registers() to the
various *-nat.c files.  (I see that mips-linux-tdep.c and
m68klinux-nat.c have taken the approach that you're suggesting;
i386-linux-nat.c has a good excuse for having its own version since
the format that it needs to support can't be handled by the generic
code.)

Re: [rfa/ppc/branch too] Fix PowerPC/Linux cores

[Daniel Jacobowitz]

On Mon, Jul 30, 2001 at 03:44:04PM -0700, Kevin Buettner wrote:
> On Jul 30,  2:23pm, Daniel Jacobowitz wrote:
> 
> > I figure it would be nice if core files for Linux/PPC worked on the branch. 
> 
> It would be nice.  It even used to work.  :-(

Once upon a time...

> > Linux/PPC doesn't have gregset_t or fpregset_t in its headers, so the body
> > of fetch_core_registers in core-regset.c gets #if'd out by autoconf.  Cores
> > load but are absolutely useless.  I have the feeling those #if's ought to be
> > outside the function rather than inside...
> 
> You may be right.  The other alternative that occurs to me is to put
> a #else branch in with a call to internal_error().  (It's a choice
> between not linking vs. not running and it's not clear which way the
> tradeoff should go.  Personally, when I'm doing a new baseport, I'd
> rather have it link with the understanding that the functionality may
> be limited...)

I would go with a warning (), probably - although I suppose
internal_error () might be appropriate.

> > but in any case, for now, this
> > patch fixes it the same way most other Linux targets do.  I'll get back to
> > my rework of core support in the next few weeks now that we've branched.
> > 
> > OK to commit, trunk and branch?
> 
> I think I'd like to explore some alternatives first.  The only
> real difference that I see between the version of fetch_core_registers()
> that you want to add to ppc-linux-nat.c and the one in core-regset.c is
> that the former uses elf_{g,fp}regset_t whereas the latter uses
> {g,fp}regset_t, right?
> 
> If that's the case, couldn't we simply make the core-regset.c version
> use gdb_gregset_t and gdb_fpgregset_t?  That seems cleaner to me than
> propagating nearly identical copies of fetch_core_registers() to the
> various *-nat.c files.  (I see that mips-linux-tdep.c and
> m68klinux-nat.c have taken the approach that you're suggesting;
> i386-linux-nat.c has a good excuse for having its own version since
> the format that it needs to support can't be handled by the generic
> code.)

The danger in that is that the gdb_*regset_t types mean a lot of
different things to a lot of different ports.  For all Linux targets,
certainly, such a change would be appropriate.  I have no idea what
other targets it would or wouldn't work for.

As I've said, half of the binutils changes for cross core debugging
have been checked in, and I'm just cleaning up the other
platform-specific bits now.  Once I've done that I'm going to tweak
(replace?) core-regset.c to not use the sizeof() operator; that's the
only thing it does at present which causes it to be native-dependent. 
After that, the copies in mips-linux-tdep, m68klinux-nat, and maybe now
ppc-linux-nat (which are all my fault to some degree) can disappear.

My preference would be to use this patch for the moment, given the
branch timeframe; while not ideal, it is certainly correct.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [rfa/ppc/branch too] Fix PowerPC/Linux cores

[Kevin Buettner]

On Jul 30,  3:54pm, Daniel Jacobowitz wrote:

> > > OK to commit, trunk and branch?
> > 
> > I think I'd like to explore some alternatives first.  The only
> > real difference that I see between the version of fetch_core_registers()
> > that you want to add to ppc-linux-nat.c and the one in core-regset.c is
> > that the former uses elf_{g,fp}regset_t whereas the latter uses
> > {g,fp}regset_t, right?
> > 
> > If that's the case, couldn't we simply make the core-regset.c version
> > use gdb_gregset_t and gdb_fpgregset_t?  That seems cleaner to me than
> > propagating nearly identical copies of fetch_core_registers() to the
> > various *-nat.c files.  (I see that mips-linux-tdep.c and
> > m68klinux-nat.c have taken the approach that you're suggesting;
> > i386-linux-nat.c has a good excuse for having its own version since
> > the format that it needs to support can't be handled by the generic
> > code.)
> 
> The danger in that is that the gdb_*regset_t types mean a lot of
> different things to a lot of different ports.  For all Linux targets,
> certainly, such a change would be appropriate.  I have no idea what
> other targets it would or wouldn't work for.

If you look in gregset.h, you'll see that gdb_gregset_t is typedef'd
to be gregset_t and gdb_fpregset_t is typedef'd to be fpregset_t
unless GDB_GREGSET_T / GDB_FPREGSET_T are defined in which case
these'll override the default values.

So all targets which *don't* define GDB_GREGSET_T / GDB_FPREGSET_T
won't be affected.  I.e, that means that we only need to worry about
those ports which *do* define GDB_*REGSET_T.  That means that
we have to consider Linux (all architectures), NetBSD/sparc,
Solaris/sparc and AIX/IA-64.

For Linux, using gdb_*regset_t in fetch_core_registers() will do the
right thing because we really want to use elf_*regset_t.  (Note,
however, that Linux/i386 isn't affected because it has its own
fetch_core_registers function.)

Neither NetBSD/sparc nor Solaris/sparc use core-regset.c, so we don't
have to worry about them.

Finally, AIX/IA-64 is a work in progress (and I'm working on it).
I'll make whatever adjustments are necessary...

> As I've said, half of the binutils changes for cross core debugging
> have been checked in, and I'm just cleaning up the other
> platform-specific bits now.  Once I've done that I'm going to tweak
> (replace?) core-regset.c to not use the sizeof() operator; that's the
> only thing it does at present which causes it to be native-dependent. 
> After that, the copies in mips-linux-tdep, m68klinux-nat, and maybe now
> ppc-linux-nat (which are all my fault to some degree) can disappear.

Great!

> My preference would be to use this patch for the moment, given the
> branch timeframe; while not ideal, it is certainly correct.

I have no objection to it going on the branch; but I'd prefer to not
see it go in on the trunk.  OTOH, I'd have no objection to a patch
which updates core-regset.c to use gdb_gregset_t / gdb_fpregset_t
being applied to either the 5.1 release branch or the trunk.  I'm
not quibbling about correctness, but I'd prefer to see it done the
"right" way on the trunk.  That could mean using gdb_*regset_t in
core-regset.c or it could mean using your upcoming cross platform
corefile support or perhaps some combination.

Before you commit anything along these lines (or along the lines of
the patch that you submitted), do check with Andrew first.  I'm not
the core-regset.c maintainer and Andrew may want to see all patches
that go in on the release branch go in on the trunk as well...

Kevin

Re: [rfa/ppc/branch too] Fix PowerPC/Linux cores

[Daniel Jacobowitz]

On Mon, Jul 30, 2001 at 05:49:34PM -0700, Kevin Buettner wrote:
> If you look in gregset.h, you'll see that gdb_gregset_t is typedef'd
> to be gregset_t and gdb_fpregset_t is typedef'd to be fpregset_t
> unless GDB_GREGSET_T / GDB_FPREGSET_T are defined in which case
> these'll override the default values.
> 
> So all targets which *don't* define GDB_GREGSET_T / GDB_FPREGSET_T
> won't be affected.  I.e, that means that we only need to worry about
> those ports which *do* define GDB_*REGSET_T.  That means that
> we have to consider Linux (all architectures), NetBSD/sparc,
> Solaris/sparc and AIX/IA-64.
> 
> For Linux, using gdb_*regset_t in fetch_core_registers() will do the
> right thing because we really want to use elf_*regset_t.  (Note,
> however, that Linux/i386 isn't affected because it has its own
> fetch_core_registers function.)
> 
> Neither NetBSD/sparc nor Solaris/sparc use core-regset.c, so we don't
> have to worry about them.
> 
> Finally, AIX/IA-64 is a work in progress (and I'm working on it).
> I'll make whatever adjustments are necessary...

> I have no objection to it going on the branch; but I'd prefer to not
> see it go in on the trunk.  OTOH, I'd have no objection to a patch
> which updates core-regset.c to use gdb_gregset_t / gdb_fpregset_t
> being applied to either the 5.1 release branch or the trunk.  I'm
> not quibbling about correctness, but I'd prefer to see it done the
> "right" way on the trunk.  That could mean using gdb_*regset_t in
> core-regset.c or it could mean using your upcoming cross platform
> corefile support or perhaps some combination.
> 
> Before you commit anything along these lines (or along the lines of
> the patch that you submitted), do check with Andrew first.  I'm not
> the core-regset.c maintainer and Andrew may want to see all patches
> that go in on the release branch go in on the trunk as well...

OK.  In that case, I think the right thing to do would be:
  - remove the #ifdefs
  - change to the gdb_*regset_t functions
On both branch and trunk.  I thought more platforms had custom
GDB_GREGSET_T definitions, but I guess not.

It's closer to overall right, and it will still go away in a reasonable
amount of time.  I'd rather not diverge the branch and truck if I don't
have to.

Andrew, how's that sound?

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [rfa/ppc/branch too] Fix PowerPC/Linux cores

[Andrew Cagney]

>> Before you commit anything along these lines (or along the lines of
>> the patch that you submitted), do check with Andrew first.  I'm not
>> the core-regset.c maintainer and Andrew may want to see all patches
>> that go in on the release branch go in on the trunk as well...
> 
> 
> OK.  In that case, I think the right thing to do would be:
>   - remove the #ifdefs
>   - change to the gdb_*regset_t functions
> On both branch and trunk.  I thought more platforms had custom
> GDB_GREGSET_T definitions, but I guess not.
> 
> It's closer to overall right, and it will still go away in a reasonable
> amount of time.  I'd rather not diverge the branch and truck if I don't
> have to.
> 
> Andrew, how's that sound?


Well, the one thing I don't want is hacks pulled on the branch that 
aren't matched in the trunk - people will assume bugs are fixed when 
they are not.  Pulling the same tweek on both is ok.

Beyond that it is between you and Kevin.

	Andrew

Re: [rfa/ppc/branch too] Fix PowerPC/Linux cores

[Daniel Jacobowitz]

On Mon, Jul 30, 2001 at 05:49:34PM -0700, Kevin Buettner wrote:
> I have no objection to it going on the branch; but I'd prefer to not
> see it go in on the trunk.  OTOH, I'd have no objection to a patch
> which updates core-regset.c to use gdb_gregset_t / gdb_fpregset_t
> being applied to either the 5.1 release branch or the trunk.  I'm
> not quibbling about correctness, but I'd prefer to see it done the
> "right" way on the trunk.  That could mean using gdb_*regset_t in
> core-regset.c or it could mean using your upcoming cross platform
> corefile support or perhaps some combination.

Fixing it "right" will be cross corefile support, but changing
core-regset is correct for now.  If no one objects I'm going to check
in the below patch tomorrow, branch and trunk (when is the branch
release point supposed to be?  Isn't it coming up on us now?).

I tested the patch on powerpc-linux, and it works exactly as expected.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

2001-08-02  Daniel Jacobowitz  <drow@mvista.com>

	* core-regset.c (fetch_core_registers):  Remove HAVE_GREGSET_T
	and HAVE_FPREGSET_T checks.  Use gdb_gregset_t and
	gdb_fpregset_t.

--- gdb-5.0.cvs20010729/gdb/core-regset.c.orig	Thu Aug  2 11:26:38 2001
+++ gdb-5.0.cvs20010729/gdb/core-regset.c	Thu Aug  2 11:27:05 2001
@@ -84,9 +84,8 @@
 fetch_core_registers (char *core_reg_sect, unsigned core_reg_size, int which,
 		      CORE_ADDR reg_addr)
 {
-#if defined (HAVE_GREGSET_T) && defined (HAVE_FPREGSET_T)
-  gregset_t gregset;
-  fpregset_t fpregset;
+  gdb_gregset_t gregset;
+  gdb_fpregset_t fpregset;
 
   if (which == 0)
     {
@@ -113,7 +112,6 @@
 	    supply_fpregset (&fpregset);
 	}
     }
-#endif /* defined(HAVE_GREGSET_T) && defined (HAVE_FPREGSET_T) */
 }
 \f
 

Re: [rfa/ppc/branch too] Fix PowerPC/Linux cores

[Kevin Buettner]

On Aug 2, 12:08pm, Daniel Jacobowitz wrote:

> Fixing it "right" will be cross corefile support, but changing
> core-regset is correct for now.  If no one objects I'm going to check
> in the below patch tomorrow, branch and trunk (when is the branch
> release point supposed to be?  Isn't it coming up on us now?).
> 
> I tested the patch on powerpc-linux, and it works exactly as expected.

Your patch looks good to me...  (Please note that I'm not the maintainer
of core-regset.c though.)

> 2001-08-02  Daniel Jacobowitz  <drow@mvista.com>
> 
> 	* core-regset.c (fetch_core_registers):  Remove HAVE_GREGSET_T
> 	and HAVE_FPREGSET_T checks.  Use gdb_gregset_t and
> 	gdb_fpregset_t.
> 
> --- gdb-5.0.cvs20010729/gdb/core-regset.c.orig	Thu Aug  2 11:26:38 2001
> +++ gdb-5.0.cvs20010729/gdb/core-regset.c	Thu Aug  2 11:27:05 2001
> @@ -84,9 +84,8 @@
>  fetch_core_registers (char *core_reg_sect, unsigned core_reg_size, int which,
>  		      CORE_ADDR reg_addr)
>  {
> -#if defined (HAVE_GREGSET_T) && defined (HAVE_FPREGSET_T)
> -  gregset_t gregset;
> -  fpregset_t fpregset;
> +  gdb_gregset_t gregset;
> +  gdb_fpregset_t fpregset;
>  
>    if (which == 0)
>      {
> @@ -113,7 +112,6 @@
>  	    supply_fpregset (&fpregset);
>  	}
>      }
> -#endif /* defined(HAVE_GREGSET_T) && defined (HAVE_FPREGSET_T) */
>  }
>  \f

Re: [rfa/ppc/branch too] Fix PowerPC/Linux cores

[Andrew Cagney]

> On Aug 2, 12:08pm, Daniel Jacobowitz wrote:
> 
> 
>> Fixing it "right" will be cross corefile support, but changing
>> core-regset is correct for now.  If no one objects I'm going to check
>> in the below patch tomorrow, branch and trunk (when is the branch
>> release point supposed to be?  Isn't it coming up on us now?).
>> 
>> I tested the patch on powerpc-linux, and it works exactly as expected.
> 
> 
> Your patch looks good to me...  (Please note that I'm not the maintainer
> of core-regset.c though.)


I think you two have figured out it's as ok as it can be made -> 
``obvious''.

	Andrew


>> 2001-08-02  Daniel Jacobowitz  <drow@mvista.com>
>> 
>> * core-regset.c (fetch_core_registers):  Remove HAVE_GREGSET_T
>> and HAVE_FPREGSET_T checks.  Use gdb_gregset_t and
>> gdb_fpregset_t.
>> 
>> --- gdb-5.0.cvs20010729/gdb/core-regset.c.orig	Thu Aug  2 11:26:38 2001
>> +++ gdb-5.0.cvs20010729/gdb/core-regset.c	Thu Aug  2 11:27:05 2001
>> @@ -84,9 +84,8 @@
>> fetch_core_registers (char *core_reg_sect, unsigned core_reg_size, int which,
>> CORE_ADDR reg_addr)
>> {
>> -#if defined (HAVE_GREGSET_T) && defined (HAVE_FPREGSET_T)
>> -  gregset_t gregset;
>> -  fpregset_t fpregset;
>> +  gdb_gregset_t gregset;
>> +  gdb_fpregset_t fpregset;
>> > if (which == 0)
>> {
>> @@ -113,7 +112,6 @@
>> supply_fpregset (&fpregset);
>> }
>> }
>> -#endif /* defined(HAVE_GREGSET_T) && defined (HAVE_FPREGSET_T) */
>> }
>> \f
> 

Re: [rfa/ppc/branch too] Fix PowerPC/Linux cores

[Daniel Jacobowitz]

On Fri, Aug 03, 2001 at 11:39:14AM -0400, Andrew Cagney wrote:
> > On Aug 2, 12:08pm, Daniel Jacobowitz wrote:
> > 
> > 
> >> Fixing it "right" will be cross corefile support, but changing
> >> core-regset is correct for now.  If no one objects I'm going to check
> >> in the below patch tomorrow, branch and trunk (when is the branch
> >> release point supposed to be?  Isn't it coming up on us now?).
> >> 
> >> I tested the patch on powerpc-linux, and it works exactly as expected.
> > 
> > 
> > Your patch looks good to me...  (Please note that I'm not the maintainer
> > of core-regset.c though.)
> 
> 
> I think you two have figured out it's as ok as it can be made -> 
> ``obvious''.

So let it be written, so let it be done (trunk and branch).

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer