[PATCH]: Use core regset when possible in linux-nat.c

[PATCH]: Use core regset when possible in linux-nat.c

[David S. Miller]

As discussed in:

  http://sources.redhat.com/ml/gdb/2006-04/msg00030.html

Some Linux platforms have different ptrace vs. core file
register layouts.

The suggested way to handle this is to use the core regset
interfaces.  Not all Linux target support that yet, so we
have to check and use the old code if the target does not
have core regset support.

Tested on sparc*-*-linux* Both gdb.base/corefile.exp and
gdb.base/gcore.exp pass (*1), which is what I was originally trying to
achieve :-)

Ok to apply?

*1: The test in corefile.exp to look at the mmap() area pointed
    to by buf2 fails due to a Linux kernel bug, it doesn't dump
    mmap() areas which have not been written to into the core
    file so gdb can't look at it.  I've posted a patch to linux-kernel
    already suggesting to take away that check in the ELF core dumper.

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

	* linux-nat.c (linux_nat_do_thread_registers): Use the
	regset_from_core_section infrastructure if the target
	supports it.
	* Makefile.in: Update dependencies.
	
--- ./linux-nat.c.~1~	2006-04-05 18:08:11.000000000 -0700
+++ ./linux-nat.c	2006-04-06 14:56:06.000000000 -0700
@@ -36,6 +36,7 @@
 #include "gdbthread.h"
 #include "gdbcmd.h"
 #include "regcache.h"
+#include "regset.h"
 #include "inf-ptrace.h"
 #include "auxv.h"
 #include <sys/param.h>		/* for MAXPATHLEN */
@@ -2529,21 +2530,49 @@ linux_nat_do_thread_registers (bfd *obfd
   gdb_fpxregset_t fpxregs;
 #endif
   unsigned long lwp = ptid_get_lwp (ptid);
-
-  fill_gregset (&gregs, -1);
+  struct gdbarch *gdbarch = current_gdbarch;
+  const struct regset *regset;
+  int core_regset_p;
+
+  core_regset_p = gdbarch_regset_from_core_section_p (gdbarch);
+  if (core_regset_p)
+    {
+      regset = gdbarch_regset_from_core_section (gdbarch, ".reg",
+						 sizeof (gregs));
+      regset->collect_regset (regset, current_regcache, -1,
+			      &gregs, sizeof (gregs));
+    }
+  else
+    fill_gregset (&gregs, -1);
   note_data = (char *) elfcore_write_prstatus (obfd,
 					       note_data,
 					       note_size,
 					       lwp,
 					       stop_signal, &gregs);
 
-  fill_fpregset (&fpregs, -1);
+  if (core_regset_p)
+    {
+      regset = gdbarch_regset_from_core_section (gdbarch, ".reg2",
+						 sizeof (fpregs));
+      regset->collect_regset (regset, current_regcache, -1,
+			      &fpregs, sizeof (fpregs));
+    }
+  else
+    fill_fpregset (&fpregs, -1);
   note_data = (char *) elfcore_write_prfpreg (obfd,
 					      note_data,
 					      note_size,
 					      &fpregs, sizeof (fpregs));
 #ifdef FILL_FPXREGSET
-  fill_fpxregset (&fpxregs, -1);
+  if (core_regset_p)
+    {
+      regset = gdbarch_regset_from_core_section (gdbarch, ".reg-xfp",
+						 sizeof (fpxregs));
+      regset->collect_regset (regset, current_regcache, -1,
+			      &fpxregs, sizeof (fpxregs));
+    }
+  else
+    fill_fpxregset (&fpxregs, -1);
   note_data = (char *) elfcore_write_prxfpreg (obfd,
 					       note_data,
 					       note_size,
--- ./Makefile.in.~1~	2006-04-05 15:55:07.000000000 -0700
+++ ./Makefile.in	2006-04-06 14:56:33.000000000 -0700
@@ -2195,8 +2195,8 @@ linux-fork.o: linux-fork.c $(defs_h) $(i
 	$(linux_nat_h)
 linux-nat.o: linux-nat.c $(defs_h) $(inferior_h) $(target_h) $(gdb_string_h) \
 	$(gdb_wait_h) $(gdb_assert_h) $(linux_nat_h) $(gdbthread_h) \
-	$(gdbcmd_h) $(regcache_h) $(inf_ptrace_h) $(auxv_h) $(elf_bfd_h) \
-	$(gregset_h) $(gdbcore_h) $(gdbthread_h) $(gdb_stat_h) \
+	$(gdbcmd_h) $(regcache_h) $(regset_h) $(inf_ptrace_h) $(auxv_h) \
+	$(elf_bfd_h) $(gregset_h) $(gdbcore_h) $(gdbthread_h) $(gdb_stat_h) \
 	$(linux_fork_h)
 linux-thread-db.o: linux-thread-db.c $(defs_h) $(gdb_assert_h) \
 	$(gdb_proc_service_h) $(gdb_thread_db_h) $(bfd_h) $(exceptions_h) \

Re: [PATCH]: Use core regset when possible in linux-nat.c

[Daniel Jacobowitz]

On Thu, Apr 06, 2006 at 03:03:30PM -0700, David S. Miller wrote:
> *1: The test in corefile.exp to look at the mmap() area pointed
>     to by buf2 fails due to a Linux kernel bug, it doesn't dump
>     mmap() areas which have not been written to into the core
>     file so gdb can't look at it.  I've posted a patch to linux-kernel
>     already suggesting to take away that check in the ELF core dumper.

Or, we could just change the test.  I don't know...

> 2006-04-06  David S. Miller  <davem@sunset.davemloft.net>
> 
> 	* linux-nat.c (linux_nat_do_thread_registers): Use the
> 	regset_from_core_section infrastructure if the target
> 	supports it.
> 	* Makefile.in: Update dependencies.

Almost.  I know I suggested changing the fallback case from your
original suggestion, but this isn't quite what I meant:

> -  fill_fpregset (&fpregs, -1);
> +  if (core_regset_p)
> +    {
> +      regset = gdbarch_regset_from_core_section (gdbarch, ".reg2",
> +						 sizeof (fpregs));
> +      regset->collect_regset (regset, current_regcache, -1,
> +			      &fpregs, sizeof (fpregs));
> +    }
> +  else
> +    fill_fpregset (&fpregs, -1);
>    note_data = (char *) elfcore_write_prfpreg (obfd,
>  					      note_data,
>  					      note_size,
>  					      &fpregs, sizeof (fpregs));

A target which only defines .reg and not .reg2 will crash here.
We'd still have to check for non-NULL; I just think that if the
target supports regset_from_core_section, and still returns NULL,
that that's a pretty good hint we have nothing to write.

The other thing that bugs me about this interface may be harder to fix
- we're still assuming the two are the same, more or less, because the
regset type is required here and we use its sizeof.  A problem for
another day?


-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH]: Use core regset when possible in linux-nat.c

[David S. Miller]

From: Daniel Jacobowitz <drow@false.org>
Date: Sat, 8 Apr 2006 16:34:37 -0400

> On Thu, Apr 06, 2006 at 03:03:30PM -0700, David S. Miller wrote:
> > *1: The test in corefile.exp to look at the mmap() area pointed
> >     to by buf2 fails due to a Linux kernel bug, it doesn't dump
> >     mmap() areas which have not been written to into the core
> >     file so gdb can't look at it.  I've posted a patch to linux-kernel
> >     already suggesting to take away that check in the ELF core dumper.
> 
> Or, we could just change the test.  I don't know...

I think something like that should work properly.  For example,
say you're debugging a compiler or config file parser that mmap()'s
the file being parsed.  If you get a crash and that file being looked
at is critical for debugging the problem, it should be obtainable
during the debugging session.

Perhaps the alignment of the object being parsed in that mmap()'d area
is critical for reproducing the bug.

I've been burned enough times in situations like this, where some
omitted piece of debugging information held the clue to the problem,
that I strongly hesitate to ever suggest we report less rather than
more information in a core file :-)

> > -  fill_fpregset (&fpregs, -1);
> > +  if (core_regset_p)
> > +    {
> > +      regset = gdbarch_regset_from_core_section (gdbarch, ".reg2",
> > +						 sizeof (fpregs));
> > +      regset->collect_regset (regset, current_regcache, -1,
> > +			      &fpregs, sizeof (fpregs));
> > +    }
> > +  else
> > +    fill_fpregset (&fpregs, -1);
> >    note_data = (char *) elfcore_write_prfpreg (obfd,
> >  					      note_data,
> >  					      note_size,
> >  					      &fpregs, sizeof (fpregs));
> 
> A target which only defines .reg and not .reg2 will crash here.
> We'd still have to check for non-NULL; I just think that if the
> target supports regset_from_core_section, and still returns NULL,
> that that's a pretty good hint we have nothing to write.

Ok, I've attached an updated version of the patch below.

> The other thing that bugs me about this interface may be harder to fix
> - we're still assuming the two are the same, more or less, because the
> regset type is required here and we use its sizeof.  A problem for
> another day?

Perhaps.  There are some other dragons lurking here.

The targets that support regset_from_core_section fall roughly into two
categories:

1) They check that the size == the regset size.
2) They check that the size >= the regset size.

Since we define GDB_GREGSET_T to the elf regset in config/nm-linux.h it
should work out properly when host==target, but for the case of a 64-bit
host debugging it's 32-bit bretheren we're going to do some queer things.

The BFD elf layer provides quite nice support for 32-bit and 64-bit
ELF core section handling via functions like elfcore_grok_prstatus().
If it sees that the size is the 32-bit variant it will interpret the
size of the registers correctly.  Similarly for elfcore_grok_psinfo()
and elfcore_grok_pstatus().

But going the other way, we have only elfcore_write_prpsinfo() which only
can write out the native prpsinfo_t layout.  So if you have a 64-bit
Linux/Sparc gdb debugging a 32-bit process, it will write out 64-bit
core file sections for the 32-bit process :-)  Now, this works if you
try to read back in the core using the 64-bit GDB but a 32-bit GDB would
not be able to grok it.

It is something to handle properly in the future and platforms like
MIPS and PowerPC, where mixing 32-bit/64-bit environments is common,
would certainly appreciate this working fully as well.

But overall I think this updated version of the patch below is safe and
makes things no worse than they are now, and as a bonus GDB can natively
generate core files and intepret them identically to the kernel produced
core files on Linux/Sparc.

Ok to apply now?

Thanks again for all of your reviewing efforts Daniel.

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

	* linux-nat.c (linux_nat_do_thread_registers): Use the
	regset_from_core_section infrastructure if the target
	supports it.
	* Makefile.in: Update dependencies.
	
--- linux-nat.c.~1~	2006-04-08 14:18:18.000000000 -0700
+++ linux-nat.c	2006-04-08 14:21:56.000000000 -0700
@@ -36,6 +36,7 @@
 #include "gdbthread.h"
 #include "gdbcmd.h"
 #include "regcache.h"
+#include "regset.h"
 #include "inf-ptrace.h"
 #include "auxv.h"
 #include <sys/param.h>		/* for MAXPATHLEN */
@@ -2535,25 +2536,72 @@ linux_nat_do_thread_registers (bfd *obfd
   gdb_fpxregset_t fpxregs;
 #endif
   unsigned long lwp = ptid_get_lwp (ptid);
+  struct gdbarch *gdbarch = current_gdbarch;
+  const struct regset *regset;
+  int core_regset_p, record_reg_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;
+    }
+  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;
+    }
+  else
+    fill_fpregset (&fpregs, -1);
+
+  if (record_reg_p)
+    note_data = (char *) elfcore_write_prfpreg (obfd,
+						note_data,
+						note_size,
+						&fpregs, sizeof (fpregs));
 
-  fill_gregset (&gregs, -1);
-  note_data = (char *) elfcore_write_prstatus (obfd,
-					       note_data,
-					       note_size,
-					       lwp,
-					       stop_signal, &gregs);
-
-  fill_fpregset (&fpregs, -1);
-  note_data = (char *) elfcore_write_prfpreg (obfd,
-					      note_data,
-					      note_size,
-					      &fpregs, sizeof (fpregs));
 #ifdef FILL_FPXREGSET
-  fill_fpxregset (&fpxregs, -1);
-  note_data = (char *) elfcore_write_prxfpreg (obfd,
-					       note_data,
-					       note_size,
-					       &fpxregs, sizeof (fpxregs));
+  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;
+    }
+  else
+    fill_fpxregset (&fpxregs, -1);
+
+  if (record_reg_p)
+    note_data = (char *) elfcore_write_prxfpreg (obfd,
+						 note_data,
+						 note_size,
+						 &fpxregs, sizeof (fpxregs));
 #endif
   return note_data;
 }
--- Makefile.in.~1~	2006-04-08 14:18:18.000000000 -0700
+++ Makefile.in	2006-04-08 14:18:22.000000000 -0700
@@ -2195,8 +2195,8 @@ linux-fork.o: linux-fork.c $(defs_h) $(i
 	$(linux_nat_h)
 linux-nat.o: linux-nat.c $(defs_h) $(inferior_h) $(target_h) $(gdb_string_h) \
 	$(gdb_wait_h) $(gdb_assert_h) $(linux_nat_h) $(gdbthread_h) \
-	$(gdbcmd_h) $(regcache_h) $(inf_ptrace_h) $(auxv_h) $(elf_bfd_h) \
-	$(gregset_h) $(gdbcore_h) $(gdbthread_h) $(gdb_stat_h) \
+	$(gdbcmd_h) $(regcache_h) $(regset_h) $(inf_ptrace_h) $(auxv_h) \
+	$(elf_bfd_h) $(gregset_h) $(gdbcore_h) $(gdbthread_h) $(gdb_stat_h) \
 	$(linux_fork_h)
 linux-thread-db.o: linux-thread-db.c $(defs_h) $(gdb_assert_h) \
 	$(gdb_proc_service_h) $(gdb_thread_db_h) $(bfd_h) $(exceptions_h) \

Re: [PATCH]: Use core regset when possible in linux-nat.c

[Daniel Jacobowitz]

On Sat, Apr 08, 2006 at 02:54:33PM -0700, David S. Miller wrote:
> 2006-04-08  David S. Miller  <davem@sunset.davemloft.net>
> 
> 	* linux-nat.c (linux_nat_do_thread_registers): Use the
> 	regset_from_core_section infrastructure if the target
> 	supports it.
> 	* Makefile.in: Update dependencies.

This looks perfect.  Thanks for revising it; this patch is OK.


-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH]: Use core regset when possible in linux-nat.c

[David S. Miller]

From: Daniel Jacobowitz <drow@false.org>
Date: Fri, 5 May 2006 16:14:26 -0400

> On Sat, Apr 08, 2006 at 02:54:33PM -0700, David S. Miller wrote:
> > 2006-04-08  David S. Miller  <davem@sunset.davemloft.net>
> > 
> > 	* linux-nat.c (linux_nat_do_thread_registers): Use the
> > 	regset_from_core_section infrastructure if the target
> > 	supports it.
> > 	* Makefile.in: Update dependencies.
> 
> This looks perfect.  Thanks for revising it; this patch is OK.

Committed, thanks for reviewing.