[committed] Fix gcore crashes on s390

[committed] Fix gcore crashes on s390

[Ulrich Weigand]

Hello,

since the switch of gcore to use regset_from_core_section, it
crashes on s390, because the regsets we're providing have a
NULL collect_regset function.  Fixed by the patch below.

Tested on s390-ibm-linux and s390x-ibm-linux.
Committed to mainline.

Bye,
Ulrich

ChangeLog:

	* s390-tdep.c (s390_collect_regset): New function.
	(s390_gregset, s390x_gregset, s390_fpregset): Add it.

Index: gdb/s390-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/s390-tdep.c,v
retrieving revision 1.153
diff -c -p -r1.153 s390-tdep.c
*** gdb/s390-tdep.c	9 Apr 2006 01:21:15 -0000	1.153
--- gdb/s390-tdep.c	6 May 2006 00:55:38 -0000
*************** s390_supply_regset (const struct regset 
*** 444,462 ****
      }
  }
  
  static const struct regset s390_gregset = {
    s390_regmap_gregset, 
!   s390_supply_regset
  };
  
  static const struct regset s390x_gregset = {
    s390x_regmap_gregset, 
!   s390_supply_regset
  };
  
  static const struct regset s390_fpregset = {
    s390_regmap_fpregset, 
!   s390_supply_regset
  };
  
  /* Return the appropriate register set for the core section identified
--- 444,484 ----
      }
  }
  
+ /* Collect register REGNUM from the register cache REGCACHE and store
+    it in the buffer specified by REGS and LEN as described by the
+    general-purpose register set REGSET.  If REGNUM is -1, do this for
+    all registers in REGSET.  */
+ static void
+ s390_collect_regset (const struct regset *regset,
+ 		     const struct regcache *regcache,
+ 		     int regnum, void *regs, size_t len)
+ {
+   const int *offset = regset->descr;
+   int i;
+ 
+   for (i = 0; i < S390_NUM_REGS; i++)
+     {
+       if ((regnum == i || regnum == -1) && offset[i] != -1)
+ 	regcache_raw_collect (regcache, i, (char *)regs + offset[i]);
+     }
+ }
+ 
  static const struct regset s390_gregset = {
    s390_regmap_gregset, 
!   s390_supply_regset,
!   s390_collect_regset
  };
  
  static const struct regset s390x_gregset = {
    s390x_regmap_gregset, 
!   s390_supply_regset,
!   s390_collect_regset
  };
  
  static const struct regset s390_fpregset = {
    s390_regmap_fpregset, 
!   s390_supply_regset,
!   s390_collect_regset
  };
  
  /* Return the appropriate register set for the core section identified
-- 
  Dr. Ulrich Weigand
  Linux on zSeries Development
  Ulrich.Weigand@de.ibm.com

Re: [committed] Fix gcore crashes on s390

[David S. Miller]

From: "Ulrich Weigand" <uweigand@de.ibm.com>
Date: Sat, 6 May 2006 03:18:51 +0200 (CEST)

> since the switch of gcore to use regset_from_core_section, it
> crashes on s390, because the regsets we're providing have a
> NULL collect_regset function.  Fixed by the patch below.
> 
> Tested on s390-ibm-linux and s390x-ibm-linux.
> Committed to mainline.

Thanks for catching and fixing this.

Hmmm... is this a common omission?

Re: [committed] Fix gcore crashes on s390

[Daniel Jacobowitz]

On Fri, May 05, 2006 at 06:32:05PM -0700, David S. Miller wrote:
> From: "Ulrich Weigand" <uweigand@de.ibm.com>
> Date: Sat, 6 May 2006 03:18:51 +0200 (CEST)
> 
> > since the switch of gcore to use regset_from_core_section, it
> > crashes on s390, because the regsets we're providing have a
> > NULL collect_regset function.  Fixed by the patch below.
> > 
> > Tested on s390-ibm-linux and s390x-ibm-linux.
> > Committed to mainline.
> 
> Thanks for catching and fixing this.
> 
> Hmmm... is this a common omission?

Apparently :-(

I see FRV has the same problem.  So does HP-UX, hppa-linux, hppa-bsd,
m32r, m68k-bsd, m88k, mips64-openbsd, mips-netbsd, powerpc-linux, and
then I stopped counting.  Oh, and I realize I was looking at the wrong
fields, so I probably missed some.  I had no idea...

I guess we're going to have to check the collect_regset function
pointer for NULL then.  David, would you mind making that change?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [committed] Fix gcore crashes on s390

[David S. Miller]

From: Daniel Jacobowitz <drow@false.org>
Date: Fri, 5 May 2006 21:56:42 -0400

> I guess we're going to have to check the collect_regset function
> pointer for NULL then.  David, would you mind making that change?

I'll note in passing that we're back to the original patch
I wrote :-)

Ok to commit?

2006-05-05  David S. Miller  <davem@sunset.davemloft.net>

	* linux-nat.c (linux_nad_do_thread_registers): Check for
	NULL collect_regset method.

Index: linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-nat.c,v
retrieving revision 1.46
diff -u -p -r1.46 linux-nat.c
--- linux-nat.c	5 May 2006 23:48:28 -0000	1.46
+++ linux-nat.c	6 May 2006 06:27:37 -0000
@@ -2549,70 +2549,52 @@ linux_nat_do_thread_registers (bfd *obfd
   unsigned long lwp = ptid_get_lwp (ptid);
   struct gdbarch *gdbarch = current_gdbarch;
   const struct regset *regset;
-  int core_regset_p, record_reg_p;
+  int core_regset_p;
 
   core_regset_p = gdbarch_regset_from_core_section_p (gdbarch);
-  record_reg_p = 1;
-  if (core_regset_p)
-    {
-      regset = gdbarch_regset_from_core_section (gdbarch, ".reg",
-						 sizeof (gregs));
-      if (regset)
-	regset->collect_regset (regset, current_regcache, -1,
-				&gregs, sizeof (gregs));
-      else
-	record_reg_p = 0;
-    }
+  if (core_regset_p
+      && (regset = gdbarch_regset_from_core_section (gdbarch, ".reg",
+						     sizeof (gregs))) != NULL
+      && regset->collect_regset != NULL)
+    regset->collect_regset (regset, current_regcache, -1,
+			    &gregs, sizeof (gregs));
   else
     fill_gregset (&gregs, -1);
 
-  if (record_reg_p)
-    note_data = (char *) elfcore_write_prstatus (obfd,
-						 note_data,
-						 note_size,
-						 lwp,
-						 stop_signal, &gregs);
-
-  record_reg_p = 1;
-  if (core_regset_p)
-    {
-      regset = gdbarch_regset_from_core_section (gdbarch, ".reg2",
-						 sizeof (fpregs));
-      if (regset)
-	regset->collect_regset (regset, current_regcache, -1,
-				&fpregs, sizeof (fpregs));
-      else
-	record_reg_p = 0;
-    }
+  note_data = (char *) elfcore_write_prstatus (obfd,
+					       note_data,
+					       note_size,
+					       lwp,
+					       stop_signal, &gregs);
+
+  if (core_regset_p
+      && (regset = gdbarch_regset_from_core_section (gdbarch, ".reg2",
+						     sizeof (fpregs))) != NULL
+      && regset->collect_regset != NULL)
+    regset->collect_regset (regset, current_regcache, -1,
+			    &fpregs, sizeof (fpregs));
   else
     fill_fpregset (&fpregs, -1);
 
-  if (record_reg_p)
-    note_data = (char *) elfcore_write_prfpreg (obfd,
-						note_data,
-						note_size,
-						&fpregs, sizeof (fpregs));
+  note_data = (char *) elfcore_write_prfpreg (obfd,
+					      note_data,
+					      note_size,
+					      &fpregs, sizeof (fpregs));
 
 #ifdef FILL_FPXREGSET
-  record_reg_p = 1;
-  if (core_regset_p)
-    {
-      regset = gdbarch_regset_from_core_section (gdbarch, ".reg-xfp",
-						 sizeof (fpxregs));
-      if (regset)
-	regset->collect_regset (regset, current_regcache, -1,
-				&fpxregs, sizeof (fpxregs));
-      else
-	record_reg_p = 0;
-    }
+  if (core_regset_p
+      && (regset = gdbarch_regset_from_core_section (gdbarch, ".reg-xfp",
+						     sizeof (fpxregs))) != NULL
+      && regset->collect_regset != NULL)
+    regset->collect_regset (regset, current_regcache, -1,
+			    &fpxregs, sizeof (fpxregs));
   else
     fill_fpxregset (&fpxregs, -1);
 
-  if (record_reg_p)
-    note_data = (char *) elfcore_write_prxfpreg (obfd,
-						 note_data,
-						 note_size,
-						 &fpxregs, sizeof (fpxregs));
+  note_data = (char *) elfcore_write_prxfpreg (obfd,
+					       note_data,
+					       note_size,
+					       &fpxregs, sizeof (fpxregs));
 #endif
   return note_data;
 }

Re: [committed] Fix gcore crashes on s390

[Mark Kettenis]

> Date: Fri, 05 May 2006 18:32:05 -0700 (PDT)
> From: "David S. Miller" <davem@davemloft.net>
> 
> From: "Ulrich Weigand" <uweigand@de.ibm.com>
> Date: Sat, 6 May 2006 03:18:51 +0200 (CEST)
> 
> > since the switch of gcore to use regset_from_core_section, it
> > crashes on s390, because the regsets we're providing have a
> > NULL collect_regset function.  Fixed by the patch below.
> > 
> > Tested on s390-ibm-linux and s390x-ibm-linux.
> > Committed to mainline.
> 
> Thanks for catching and fixing this.
> 
> Hmmm... is this a common omission?

Yes, targets are allowed to not implement the collect_regset functions
if they only implement reading core dumps and don't need it for
something else (like writing core dumps with gcore or fiddling with
threads).  All Linux targets now fall in the second category, but some
probably think they fall in the first.  So either we should:

1. Deal gracefully with the collect_regset function pointer being
   NULL.

2. Put in a gdb_assert() to check it's not null before it's used.

I'm thinking that we should try option #2 for a while to get people to
implement the functions for the other Linux targets too, just like
Ulrich did for s390.

Mark

Re: [committed] Fix gcore crashes on s390

[Mark Kettenis]

> Date: Fri, 5 May 2006 21:56:42 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Fri, May 05, 2006 at 06:32:05PM -0700, David S. Miller wrote:
> > From: "Ulrich Weigand" <uweigand@de.ibm.com>
> > Date: Sat, 6 May 2006 03:18:51 +0200 (CEST)
> > 
> > > since the switch of gcore to use regset_from_core_section, it
> > > crashes on s390, because the regsets we're providing have a
> > > NULL collect_regset function.  Fixed by the patch below.
> > > 
> > > Tested on s390-ibm-linux and s390x-ibm-linux.
> > > Committed to mainline.
> > 
> > Thanks for catching and fixing this.
> > 
> > Hmmm... is this a common omission?
> 
> Apparently :-(
> 
> I see FRV has the same problem.  So does HP-UX, hppa-linux, hppa-bsd,
> m32r, m68k-bsd, m88k, mips64-openbsd, mips-netbsd, powerpc-linux, and
> then I stopped counting.  Oh, and I realize I was looking at the wrong
> fields, so I probably missed some.  I had no idea...

FYI, thhe HP-UX and OpenBSD/NetBSD targets from the list above don't
need the collect_regset functions since they don't implement gcore.
They will probably never need them, since HP-UX and NetBSD can dump
cores using ptrace(2), and I'm working on implementing something
similar on OpenBSD.

Mark

Re: [committed] Fix gcore crashes on s390

[Daniel Jacobowitz]

On Fri, May 05, 2006 at 11:30:00PM -0700, David S. Miller wrote:
> From: Daniel Jacobowitz <drow@false.org>
> Date: Fri, 5 May 2006 21:56:42 -0400
> 
> > I guess we're going to have to check the collect_regset function
> > pointer for NULL then.  David, would you mind making that change?
> 
> I'll note in passing that we're back to the original patch
> I wrote :-)
> 
> Ok to commit?
> 
> 2006-05-05  David S. Miller  <davem@sunset.davemloft.net>
> 
> 	* linux-nat.c (linux_nad_do_thread_registers): Check for
> 	NULL collect_regset method.

OK.  Thanks.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [committed] Fix gcore crashes on s390

[Daniel Jacobowitz]

On Sat, May 06, 2006 at 10:25:03AM +0200, Mark Kettenis wrote:
> Yes, targets are allowed to not implement the collect_regset functions
> if they only implement reading core dumps and don't need it for
> something else (like writing core dumps with gcore or fiddling with
> threads).  All Linux targets now fall in the second category, but some
> probably think they fall in the first.  So either we should:
> 
> 1. Deal gracefully with the collect_regset function pointer being
>    NULL.
> 
> 2. Put in a gdb_assert() to check it's not null before it's used.
> 
> I'm thinking that we should try option #2 for a while to get people to
> implement the functions for the other Linux targets too, just like
> Ulrich did for s390.

Oh - sorry, I didn't see this before I approved David's change.
I'd rather not do it this way, especially with GDB 6.5 upcoming and a
lot of the Linux targets getting somewhat sporadic test coverage.
Is that all right with you?

If we want to make it mandatory later, someone would have to go through
the targets and fix them.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [committed] Fix gcore crashes on s390

[Mark Kettenis]

> Date: Sat, 6 May 2006 11:19:04 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Sat, May 06, 2006 at 10:25:03AM +0200, Mark Kettenis wrote:
> > Yes, targets are allowed to not implement the collect_regset functions
> > if they only implement reading core dumps and don't need it for
> > something else (like writing core dumps with gcore or fiddling with
> > threads).  All Linux targets now fall in the second category, but some
> > probably think they fall in the first.  So either we should:
> > 
> > 1. Deal gracefully with the collect_regset function pointer being
> >    NULL.
> > 
> > 2. Put in a gdb_assert() to check it's not null before it's used.
> > 
> > I'm thinking that we should try option #2 for a while to get people to
> > implement the functions for the other Linux targets too, just like
> > Ulrich did for s390.
> 
> Oh - sorry, I didn't see this before I approved David's change.
> I'd rather not do it this way, especially with GDB 6.5 upcoming and a
> lot of the Linux targets getting somewhat sporadic test coverage.
> Is that all right with you?

No problem.

Mark

Re: [committed] Fix gcore crashes on s390

[David S. Miller]

From: Mark Kettenis <mark.kettenis@xs4all.nl>
Date: Sat, 6 May 2006 20:28:16 +0200 (CEST)

> > Date: Sat, 6 May 2006 11:19:04 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > 
> > On Sat, May 06, 2006 at 10:25:03AM +0200, Mark Kettenis wrote:
> > > Yes, targets are allowed to not implement the collect_regset functions
> > > if they only implement reading core dumps and don't need it for
> > > something else (like writing core dumps with gcore or fiddling with
> > > threads).  All Linux targets now fall in the second category, but some
> > > probably think they fall in the first.  So either we should:
> > > 
> > > 1. Deal gracefully with the collect_regset function pointer being
> > >    NULL.
> > > 
> > > 2. Put in a gdb_assert() to check it's not null before it's used.
> > > 
> > > I'm thinking that we should try option #2 for a while to get people to
> > > implement the functions for the other Linux targets too, just like
> > > Ulrich did for s390.
> > 
> > Oh - sorry, I didn't see this before I approved David's change.
> > I'd rather not do it this way, especially with GDB 6.5 upcoming and a
> > lot of the Linux targets getting somewhat sporadic test coverage.
> > Is that all right with you?
> 
> No problem.

I've committed my patch to check for the NULL method.

Thanks.