[PATCH] gcore registers storing fix

[PATCH] gcore registers storing fix

[Daniel Gutson]

[-- Attachment #1: Type: text/plain, Size: 1019 bytes --]

The attached patch attempts to solve a bug that caused the gcore command 
to produce core files containing incorrect registers information. The 
problem caused incomplete backtraces when the files were read back into GDB.

I tested this patch with the gdb testsuite, and my addition to the 
gcore-thread.exp test case.

If OK, please commit it for me since I don't have write access.

Thanks,
	Daniel.

2009-11-06  Daniel Gutson <dgutson@codesourcery.com>

	gdb/
	* procfs.c (procfs_do_thread_registers): Added a call to fetch
	register values before saving them in the core file
	through the gcore command.
	(procfs_corefile_thread_callback): removed the backup of
	inferior_ptid before calling procfs_do_thread_registers since
	the function already saves and restores it before returning.

	gdb/testsuite/gdb.threads/
	* gcore-thread.exp: Added a test case in order to check
	if the core dump contains the registers values, and symbol
	lookup is working properly.

-- 
Daniel Gutson
CodeSourcery
www.codesourcery.com

[-- Attachment #2: gcore.patch --]
[-- Type: text/x-diff, Size: 3163 bytes --]

Index: gdb/procfs.c
===================================================================
RCS file: /cvs/src/src/gdb/procfs.c,v
retrieving revision 1.119
diff -u -p -r1.119 procfs.c
--- gdb/procfs.c	21 Oct 2009 08:27:25 -0000	1.119
+++ gdb/procfs.c	6 Nov 2009 22:30:54 -0000
@@ -6060,9 +6060,19 @@ procfs_do_thread_registers (bfd *obfd, p
   gdb_gregset_t gregs;
   gdb_fpregset_t fpregs;
   unsigned long merged_pid;
+  struct cleanup *old_chain;
 
   merged_pid = TIDGET (ptid) << 16 | PIDGET (ptid);
 
+  /* This part is the old method for fetching registers.
+     It should be replaced by the newer one using regsets
+     once it is implemented in this platform:
+     gdbarch_regset_from_core_section() and regset->collect_regset(). */
+
+  old_chain = save_inferior_ptid ();
+  inferior_ptid = ptid;
+  target_fetch_registers (regcache, -1);
+
   fill_gregset (regcache, &gregs, -1);
 #if defined (UNIXWARE)
   note_data = (char *) elfcore_write_lwpstatus (obfd,
@@ -6085,6 +6095,9 @@ procfs_do_thread_registers (bfd *obfd, p
 					      note_size,
 					      &fpregs,
 					      sizeof (fpregs));
+
+  do_cleanups (old_chain);
+
   return note_data;
 }
 
@@ -6102,13 +6115,11 @@ procfs_corefile_thread_callback (procinf
 
   if (pi != NULL)
     {
-      ptid_t saved_ptid = inferior_ptid;
-      inferior_ptid = MERGEPID (pi->pid, thread->tid);
-      args->note_data = procfs_do_thread_registers (args->obfd, inferior_ptid,
+      ptid_t ptid = MERGEPID (pi->pid, thread->tid);
+      args->note_data = procfs_do_thread_registers (args->obfd, ptid,
 						    args->note_data,
 						    args->note_size,
 						    args->stop_signal);
-      inferior_ptid = saved_ptid;
     }
   return 0;
 }
Index: gdb/testsuite/gdb.threads/gcore-thread.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.threads/gcore-thread.exp,v
retrieving revision 1.13
diff -u -p -r1.13 gcore-thread.exp
--- gdb/testsuite/gdb.threads/gcore-thread.exp	3 Jan 2009 05:58:07 -0000	1.13
+++ gdb/testsuite/gdb.threads/gcore-thread.exp	6 Nov 2009 22:30:54 -0000
@@ -154,11 +154,7 @@ gdb_expect {
     }
 }
 
-# FIXME: now what can we test about the thread state?
-# We do not know for certain that there should be at least 
-# three threads, because who knows what kind of many-to-one
-# mapping various OS's may do?  Let's assume that there must
-# be at least two threads:
+# There must be at least two threads.
 
 gdb_test "info threads" ".*${nl}  2 ${horiz}${nl}\\* 1 .*" \
 	"corefile contains at least two threads"
@@ -173,3 +169,16 @@ gdb_test "info threads" ".* thread2 .*" 
 gdb_test "info threads" ".*${nl}\\* ${horiz} thread2 .*" \
 	"thread2 is current thread in corefile"
 
+# The threads should be standing at a known function, rather than ??.
+
+gdb_test_multiple "info threads" \
+	"corefile contains good register values and enables symbols lookup" \
+{
+    -re ".* in \\?\\? .*" {
+        fail "corefile contains good register values and enables symbols lookup"
+    }
+    -re ".* in \[_a-zA-Z0-9]+ \\(.*" {
+        pass "corefile contains good register values and enables symbols lookup"
+    }
+}
+

Re: [PATCH] gcore registers storing fix

[Michael Snyder]

Daniel Gutson wrote:
> The attached patch attempts to solve a bug that caused the gcore command 
> to produce core files containing incorrect registers information. The 
> problem caused incomplete backtraces when the files were read back into GDB.
> 
> I tested this patch with the gdb testsuite, and my addition to the 
> gcore-thread.exp test case.
> 
> If OK, please commit it for me since I don't have write access.

What host configuration is this for?

At first glance, the procfs change looks ok (but I'm no longer
up to date on procfs).  I have some doubt about the test change.

You say in your comment, "The threads should be standing at a
known function, rather than ??".  I'm not sure how we can know
that.  The threads may have been stopped anywhere, and it's
always possible to find a library with no symbols.


> 2009-11-06  Daniel Gutson <dgutson@codesourcery.com>
> 
> 	gdb/
> 	* procfs.c (procfs_do_thread_registers): Added a call to fetch
> 	register values before saving them in the core file
> 	through the gcore command.
> 	(procfs_corefile_thread_callback): removed the backup of
> 	inferior_ptid before calling procfs_do_thread_registers since
> 	the function already saves and restores it before returning.
> 
> 	gdb/testsuite/gdb.threads/
> 	* gcore-thread.exp: Added a test case in order to check
> 	if the core dump contains the registers values, and symbol
> 	lookup is working properly.
> 

Re: [PATCH] gcore registers storing fix

[Daniel Gutson]

Hi Michael,

Michael Snyder wrote:
> Daniel Gutson wrote:
>> The attached patch attempts to solve a bug that caused the gcore 
>> command to produce core files containing incorrect registers 
>> information. The problem caused incomplete backtraces when the files 
>> were read back into GDB.
>>
>> I tested this patch with the gdb testsuite, and my addition to the 
>> gcore-thread.exp test case.
>>
>> If OK, please commit it for me since I don't have write access.
> 
> What host configuration is this for?

native gdb: solaris-sparc

> 
> At first glance, the procfs change looks ok (but I'm no longer
> up to date on procfs).  I have some doubt about the test change.
> 
> You say in your comment, "The threads should be standing at a
> known function, rather than ??".  I'm not sure how we can know
> that.  The threads may have been stopped anywhere, and it's
> always possible to find a library with no symbols.

What would you suggest? I could bound the check to the current frame.

Thanks for reviewing,
	Daniel.

-- 
Daniel Gutson
CodeSourcery
www.codesourcery.com

Re: [PATCH] gcore registers storing fix

[Daniel Jacobowitz]

On Mon, Nov 09, 2009 at 11:59:26AM -0300, Daniel Gutson wrote:
> >You say in your comment, "The threads should be standing at a
> >known function, rather than ??".  I'm not sure how we can know
> >that.  The threads may have been stopped anywhere, and it's
> >always possible to find a library with no symbols.
> 
> What would you suggest? I could bound the check to the current frame.

How about we check that at least one thread is in "thread2"?  That's
where gcore was used to create the core file.  Except, there's already
a test for that in the file.  So maybe we do not need a new test.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] gcore registers storing fix

[Daniel Gutson]

Daniel Jacobowitz wrote:
> On Mon, Nov 09, 2009 at 11:59:26AM -0300, Daniel Gutson wrote:
>>> You say in your comment, "The threads should be standing at a
>>> known function, rather than ??".  I'm not sure how we can know
>>> that.  The threads may have been stopped anywhere, and it's
>>> always possible to find a library with no symbols.
>> What would you suggest? I could bound the check to the current frame.
> 
> How about we check that at least one thread is in "thread2"?  That's
> where gcore was used to create the core file.  Except, there's already
> a test for that in the file.  So maybe we do not need a new test.

The problem shows up when "in ??" appears. I think that the current 
thread will never be shown in that way unless the test does fail.
That being said, I think that I could add the regex for the "*" in order 
to looking into the current thread only.

Thoughts?

Thanks,
	Daniel.

-- 
Daniel Gutson
CodeSourcery
www.codesourcery.com

Re: [PATCH] gcore registers storing fix

[Daniel Jacobowitz]

On Mon, Nov 09, 2009 at 03:17:36PM -0300, Daniel Gutson wrote:
> The problem shows up when "in ??" appears. I think that the current
> thread will never be shown in that way unless the test does fail.
> That being said, I think that I could add the regex for the "*" in
> order to looking into the current thread only.

How is that different from the two following tests in
gcore-thread.exp?

# One thread in the corefile should be in the "thread2" function.

gdb_test "info threads" ".* thread2 .*" \
        "a corefile thread is executing thread2"

# The thread2 thread should be marked as the current thread.

gdb_test "info threads" ".*${nl}\\* ${horiz} thread2 .*" \
        "thread2 is current thread in corefile"

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] gcore registers storing fix

[Daniel Gutson]

Daniel Jacobowitz wrote:
> On Mon, Nov 09, 2009 at 03:17:36PM -0300, Daniel Gutson wrote:
>> The problem shows up when "in ??" appears. I think that the current
>> thread will never be shown in that way unless the test does fail.
>> That being said, I think that I could add the regex for the "*" in
>> order to looking into the current thread only.
> 
> How is that different from the two following tests in
> gcore-thread.exp?
> 
> # One thread in the corefile should be in the "thread2" function.
> 
> gdb_test "info threads" ".* thread2 .*" \
>         "a corefile thread is executing thread2"
> 
> # The thread2 thread should be marked as the current thread.
> 
> gdb_test "info threads" ".*${nl}\\* ${horiz} thread2 .*" \
>         "thread2 is current thread in corefile"
> 

Because none of them checks that the text "in ??" does *not* show up

-- 
Daniel Gutson
CodeSourcery
www.codesourcery.com

Re: [PATCH] gcore registers storing fix

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Mon, Nov 09, 2009 at 11:59:26AM -0300, Daniel Gutson wrote:
>>> You say in your comment, "The threads should be standing at a
>>> known function, rather than ??".  I'm not sure how we can know
>>> that.  The threads may have been stopped anywhere, and it's
>>> always possible to find a library with no symbols.
>> What would you suggest? I could bound the check to the current frame.
> 
> How about we check that at least one thread is in "thread2"?  That's
> where gcore was used to create the core file.  Except, there's already
> a test for that in the file.  So maybe we do not need a new test.
> 

I'm guessing that what you want to test is the non-event
thread, ie. the one that is suspended while the other one
hits the breakpoint?

Re: [PATCH] gcore registers storing fix

[Daniel Jacobowitz]

On Mon, Nov 09, 2009 at 03:36:02PM -0300, Daniel Gutson wrote:
> Because none of them checks that the text "in ??" does *not* show up

Michael has pointed out that this text can legitimately appear.
If there is a function in the system libraries without debug
information, we may be there.  For instance, inside the implementation
of printf.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] gcore registers storing fix

[Daniel Gutson]

Daniel Jacobowitz wrote:
> On Mon, Nov 09, 2009 at 03:36:02PM -0300, Daniel Gutson wrote:
>> Because none of them checks that the text "in ??" does *not* show up
> 
> Michael has pointed out that this text can legitimately appear.
> If there is a function in the system libraries without debug
> information, we may be there.  For instance, inside the implementation
> of printf.

OK, should I just remove the test case then? That, or only check that 
the ?? is not in the current thread.


-- 
Daniel Gutson
CodeSourcery
www.codesourcery.com

Re: [PATCH] gcore registers storing fix

[Daniel Jacobowitz]

On Mon, Nov 09, 2009 at 05:32:12PM -0300, Daniel Gutson wrote:
> 
> 
> Daniel Jacobowitz wrote:
> >On Mon, Nov 09, 2009 at 03:36:02PM -0300, Daniel Gutson wrote:
> >>Because none of them checks that the text "in ??" does *not* show up
> >
> >Michael has pointed out that this text can legitimately appear.
> >If there is a function in the system libraries without debug
> >information, we may be there.  For instance, inside the implementation
> >of printf.
> 
> OK, should I just remove the test case then? That, or only check that
> the ?? is not in the current thread.

Let's drop the new test.  The code part of your patch is OK to commit.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] gcore registers storing fix

[Daniel Gutson]

Daniel Jacobowitz wrote:
> On Mon, Nov 09, 2009 at 05:32:12PM -0300, Daniel Gutson wrote:
>>
>> Daniel Jacobowitz wrote:
>>> On Mon, Nov 09, 2009 at 03:36:02PM -0300, Daniel Gutson wrote:
>>>> Because none of them checks that the text "in ??" does *not* show up
>>> Michael has pointed out that this text can legitimately appear.
>>> If there is a function in the system libraries without debug
>>> information, we may be there.  For instance, inside the implementation
>>> of printf.
>> OK, should I just remove the test case then? That, or only check that
>> the ?? is not in the current thread.
> 
> Let's drop the new test.  The code part of your patch is OK to commit.

OK, thanks. Please note that I don't have write access, please commit it 
for me if ever possible.

-- 
Daniel Gutson
CodeSourcery
www.codesourcery.com

Re: [PATCH] gcore registers storing fix

[Daniel Jacobowitz]

On Mon, Nov 09, 2009 at 05:37:36PM -0300, Daniel Gutson wrote:
> OK, thanks. Please note that I don't have write access, please commit
> it for me if ever possible.

Checked in.

-- 
Daniel Jacobowitz
CodeSourcery