[patch] Fix attaching to Linux stopped processes

[patch] Fix attaching to Linux stopped processes

[Jan Kratochvil]

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

Hi,

sleep 1h& pid=$!; kill -STOP $pid; gdb sleep $pid
	->
Attaching to program: /bin/sleep, process 20768
../../gdb/linux-nat.c:1057: internal-error: linux_nat_attach: Assertion `pid == GET_PID (inferior_ptid) && WIFSTOPPED (status) && WSTOPSIG (status) == SIGSTOP' failed.


Testsuite not affected.

Not tested if the patch is properly utilized on older (<=2.4) Linux kernels.


2006-09-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb/linux-nat.c (linux_nat_attach): Handle already stopped
	processes.

2006-09-17  Jeff Johnston  <jjohnstn@redhat.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/attachstop.exp, gdb.base/attachstop.c: New files,
	test attaching to already stopped processes.



Regards,
Jan

[-- Attachment #2: gdb-6.5-attach-stop.patch --]
[-- Type: text/plain, Size: 8118 bytes --]

Index: gdb/linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-nat.c,v
retrieving revision 1.50
diff -u -p -r1.50 linux-nat.c
--- gdb/linux-nat.c	16 Sep 2006 09:48:12 -0000	1.50
+++ gdb/linux-nat.c	17 Sep 2006 21:50:32 -0000
@@ -1022,13 +1022,16 @@ linux_nat_attach (char *args, int from_t
       lp->cloned = 1;
     }
 
-  gdb_assert (pid == GET_PID (inferior_ptid)
-	      && WIFSTOPPED (status) && WSTOPSIG (status) == SIGSTOP);
+  /* Do not check `WSTOPSIG (status) == SIGSTOP' as the status may be
+     arbitrary - depending on the signal that stopped the processes.
+     If the process was running we get SIGSTOP, if it was already stopped
+     by SIGSTOP we get 0.  The value gets used for `PTRACE_CONT'.  */
+  gdb_assert (pid == GET_PID (inferior_ptid) && WIFSTOPPED (status));
 
   lp->stopped = 1;
 
-  /* Fake the SIGSTOP that core GDB expects.  */
-  lp->status = W_STOPCODE (SIGSTOP);
+  /* Provide the stop status that core GDB expects.  */
+  lp->status = status;
   lp->resumed = 1;
   if (debug_linux_nat)
     {
Index: gdb/testsuite/gdb.base/attachstop.c
===================================================================
RCS file: gdb/testsuite/gdb.base/attachstop.c
diff -N gdb/testsuite/gdb.base/attachstop.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.base/attachstop.c	17 Sep 2006 21:50:32 -0000
@@ -0,0 +1,29 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2005 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+ 
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. */
+
+/* This program is intended to be started outside of gdb, then
+   manually stopped via a signal.  */
+
+#include <unistd.h>
+
+int main ()
+{
+  sleep (10000);  /* Ridiculous time, but we will eventually kill it.  */
+  sleep (10000);  /* Second sleep.  */
+  return 0;
+}
Index: gdb/testsuite/gdb.base/attachstop.exp
===================================================================
RCS file: gdb/testsuite/gdb.base/attachstop.exp
diff -N gdb/testsuite/gdb.base/attachstop.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.base/attachstop.exp	17 Sep 2006 21:50:32 -0000
@@ -0,0 +1,198 @@
+# Copyright 2005-2006
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+# 
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+
+# This test was created by modifying attach.exp.
+# This file was created by Jeff Johnston <jjohnstn@redhat.com>.
+# This file was updated by Jan Kratochvil <jan.kratochvil@redhat.com>.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+set prms_id 0
+set bug_id 0
+
+# This test only works on Linux
+if { ![istarget "*-*-linux-gnu*"] } {
+    return 0
+}
+
+set testfile "attachstop"
+set srcfile  ${testfile}.c
+set binfile  ${objdir}/${subdir}/${testfile}
+set escapedbinfile  [string_to_regexp ${objdir}/${subdir}/${testfile}]
+
+#execute_anywhere "rm -f ${binfile}"
+remote_exec build "rm -f ${binfile}"
+# For debugging this test
+#
+#log_user 1
+
+# build the test case
+#
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
+}
+
+if [get_compiler_info ${binfile}] {
+    return -1
+}
+
+# Start the program running and then wait for a bit, to be sure
+# that it can be attached to.
+
+set testpid [eval exec $binfile &]
+exec sleep 2
+   
+# Stop the program 
+remote_exec build "kill -s STOP ${testpid}"
+
+# Start with clean gdb
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+# Verify that we can attach to the process by first giving its
+# executable name via the file command, and using attach with the
+# process ID.
+
+set test "set file, before attach1 to stopped process"
+gdb_test_multiple "file $binfile" "$test" {
+   -re "Load new symbol table from.*y or n. $" {
+        gdb_test "y" "Reading symbols from $escapedbinfile\.\.\.*done." \
+		"$test (re-read)"
+    }
+    -re "Reading symbols from $escapedbinfile\.\.\.*done.*$gdb_prompt $" {
+        pass "$test"
+    }
+}
+
+set test "attach1 to stopped, after setting file"
+gdb_test_multiple "attach $testpid" "$test" {
+    -re "Attaching to program.*`?$escapedbinfile'?, process $testpid.*$gdb_prompt $" {
+        pass "$test"
+    }
+}
+
+gdb_test "bt" ".*sleep.*main.*" "attach1 to stopped bt"
+
+# Exit and detach the process.
+   
+gdb_exit
+
+set fileid [open /proc/${testpid}/status r];
+gets $fileid line1;
+gets $fileid line2;
+close $fileid;
+
+set test "attach1, exit leaves process stopped"
+if {[string match "*(stopped)*" $line2]} {
+  pass $test
+} else {
+  fail $test
+}
+
+# At this point, the process should still be stopped
+
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+# Verify that we can attach to the process just by giving the
+# process ID.
+   
+set test "attach2 to stopped, after setting file"
+gdb_test_multiple "attach $testpid" "$test" {
+    -re "Attaching to program.*`?$escapedbinfile'?, process $testpid.*$gdb_prompt $" {
+        pass "$test"
+    }
+}
+
+gdb_test "bt" ".*sleep.*main.*" "attach2 to stopped bt"
+gdb_breakpoint [gdb_get_line_number "Second sleep"]
+set test "attach2 continue"
+send_gdb "continue\n"
+gdb_expect {
+  -re "Continuing"
+    { pass "continue ($test)" }
+  timeout
+    { fail "continue ($test) (timeout)" }
+}
+
+# For this to work we must be sure to consume the "Continuing."
+# message first, or GDB's signal handler may not be in place.
+after 1000 {send_gdb "\003"}
+set test "attach2 stop unbreakable interrupt"
+gdb_expect 4 {
+  -re "Program received signal SIGINT.*$gdb_prompt $"
+    {
+      fail "$test (broke into)"
+    }
+  -re "Breakpoint \[0-9\].*$srcfile.*$gdb_prompt $"
+    {
+      fail "$test (broke into)"
+    }
+  timeout
+    {
+      pass $test
+    }
+}
+
+# Continue the program 
+remote_exec build "kill -s CONT ${testpid}"
+
+# Already sent before: after 1000 {send_gdb "\003"}
+set test "attach2 stop by interrupt"
+gdb_expect {
+  -re "Program received signal SIGINT.*$gdb_prompt $"
+    {
+      pass $test
+    }
+  -re "Breakpoint \[0-9\].*$srcfile.*$gdb_prompt $"
+    {
+      pass $test
+    }
+  timeout
+    {
+      fail "$test (timeout)"
+    }
+}
+
+gdb_exit
+
+# At this point, the process should be sleeping
+
+set fileid2 [open /proc/${testpid}/status r];
+gets $fileid2 line1;
+gets $fileid2 line2;
+close $fileid2;
+
+set test "attach2, exit leaves process sleeping"
+if {[string match "*(sleeping)*" $line2]} {
+  pass $test
+} else {
+  fail $test
+}
+
+# Make sure we don't leave a process around to confuse
+# the next test run (and prevent the compile by keeping
+# the text file busy), in case the "set should_exit" didn't
+# work.
+   
+remote_exec build "kill -9 ${testpid}"
+
+return 0

Re: [patch] Fix attaching to Linux stopped processes

[Daniel Jacobowitz]

On Mon, Sep 18, 2006 at 12:12:28AM +0200, Jan Kratochvil wrote:
> Hi,
> 
> sleep 1h& pid=$!; kill -STOP $pid; gdb sleep $pid
> 	->
> Attaching to program: /bin/sleep, process 20768
> ../../gdb/linux-nat.c:1057: internal-error: linux_nat_attach: Assertion `pid == GET_PID (inferior_ptid) && WIFSTOPPED (status) && WSTOPSIG (status) == SIGSTOP' failed.

> +  /* Do not check `WSTOPSIG (status) == SIGSTOP' as the status may be
> +     arbitrary - depending on the signal that stopped the processes.
> +     If the process was running we get SIGSTOP, if it was already stopped
> +     by SIGSTOP we get 0.  The value gets used for `PTRACE_CONT'.  */

That's a very strange behavior.  It doesn't make much sense to me, and
it isn't familiar - which is strange because I've been all over the
kernel code for this.  So I checked.  Sure enough, on my 2.6.18-rc4
installation, this is _not_ what happens.  Instead, the traditional
thing happens: wait hangs and no new event is returned.

Is this new in kernel.org?  Or is it a Red Hat kernel patch?  Where
did it come from?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Fix attaching to Linux stopped processes

[Jan Kratochvil]

On Mon, 18 Sep 2006 00:26:22 +0200, Daniel Jacobowitz wrote:
> On Mon, Sep 18, 2006 at 12:12:28AM +0200, Jan Kratochvil wrote:
> > Hi,
> > 
> > sleep 1h& pid=$!; kill -STOP $pid; gdb sleep $pid
> > 	->
> > Attaching to program: /bin/sleep, process 20768
> > ../../gdb/linux-nat.c:1057: internal-error: linux_nat_attach: Assertion `pid == GET_PID (inferior_ptid) && WIFSTOPPED (status) && WSTOPSIG (status) == SIGSTOP' failed.
> 
> > +  /* Do not check `WSTOPSIG (status) == SIGSTOP' as the status may be
> > +     arbitrary - depending on the signal that stopped the processes.
> > +     If the process was running we get SIGSTOP, if it was already stopped
> > +     by SIGSTOP we get 0.  The value gets used for `PTRACE_CONT'.  */
> 
> So I checked.  Sure enough, on my 2.6.18-rc4
> installation, this is _not_ what happens.  Instead, the traditional
> thing happens: wait hangs and no new event is returned.
> 
> Is this new in kernel.org?  Or is it a Red Hat kernel patch?  Where
> did it come from?

Thanks for checking it.
OK, the patch was originally for GDB on Red Hat "kernel-2.6.17-1.2647.fc6".

Still it is reproducible even on both linux-2.6.17.13 and linux-2.6.18-rc4, you
just need to "kill -CONT $pid" the inferior as gdb(1) will hang during
attaching to it.  Screenshot (sorry for graphics):
	http://www.jankratochvil.net/priv/gdb.png
gdb is built from the current clean CVS snapshot.

Also the patch looks right to me - after `PT_ATTACH' it is appropriate that
`WSTOPSIG (status)' will report the original signal that stopped process,
(WSTOPSIG (status) == SIGSTOP) <=> (process was in running/sleeping mode).


Regards,
Jan

Re: [patch] Fix attaching to Linux stopped processes

[Daniel Jacobowitz]

On Mon, Sep 18, 2006 at 11:53:31AM +0200, Jan Kratochvil wrote:
> > Is this new in kernel.org?  Or is it a Red Hat kernel patch?  Where
> > did it come from?
> 
> Thanks for checking it.
> OK, the patch was originally for GDB on Red Hat "kernel-2.6.17-1.2647.fc6".
> 
> Still it is reproducible even on both linux-2.6.17.13 and linux-2.6.18-rc4, you
> just need to "kill -CONT $pid" the inferior as gdb(1) will hang during
> attaching to it.  Screenshot (sorry for graphics):
> 	http://www.jankratochvil.net/priv/gdb.png

Oh right - I've seen this crash before.

> Also the patch looks right to me - after `PT_ATTACH' it is appropriate that
> `WSTOPSIG (status)' will report the original signal that stopped process,
> (WSTOPSIG (status) == SIGSTOP) <=> (process was in running/sleeping mode).

The part I was dubious about was the zero if already stopped by
sigstop.  In any case, I agree.  The patch to linux-nat.c is OK
to apply.

I'm not sure about the testcase though :-(  I don't think it'll work
without the RH patched kernel, will it?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Fix attaching to Linux stopped processes

[Jan Kratochvil]

On Mon, 18 Sep 2006 15:16:17 +0200, Daniel Jacobowitz wrote:
...
> > Also the patch looks right to me - after `PT_ATTACH' it is appropriate that
> > `WSTOPSIG (status)' will report the original signal that stopped process,
> > (WSTOPSIG (status) == SIGSTOP) <=> (process was in running/sleeping mode).
> 
> The part I was dubious about was the zero if already stopped by
> sigstop.
[ see below ]
> In any case, I agree.  The patch to linux-nat.c is OK to apply.
> 
> I'm not sure about the testcase though :-(  I don't think it'll work
> without the RH patched kernel, will it?

The testcase does not work for kernel.org kernels.
And in fast on kernel.org Linux kernels the patch also does not work well as
you need to manually "kill -CONT" the process during the attach, moreover gdb
will get 'SIGCONT' id afterwards (and leave the process running after detach).

(I did not test but) I expect the Red Hat kernels behavior change is due to the
new utrace patch from Intel present in Red Hat kernels. It is going to be
imported to kernel.org one day changing the current (IMO broken - I may be
wrong) kernels behavior.

Still I consider the patch to be better than the current behavior but still it
is not fully functional, sorry for its submission; going to check the patches
on kernel.org kernels next time.  Patch update from me possible later.


Regards,
Jan

Re: [patch] Fix attaching to Linux stopped processes

[Daniel Jacobowitz]

On Mon, Sep 18, 2006 at 10:49:12PM +0200, Jan Kratochvil wrote:
> (I did not test but) I expect the Red Hat kernels behavior change is due to the
> new utrace patch from Intel present in Red Hat kernels. It is going to be
> imported to kernel.org one day changing the current (IMO broken - I may be
> wrong) kernels behavior.

Actually, I think it's probably a separate issue: Roland McGrath wrote
utrace, and he was pretty meticulous about matching existing ptrace
behavior.  It may be a change since the last time I looked at his
patches.

I definitely agree that the current kernel behavior is not very useful.
It may be that the "signal 0" behavior you describe is better, but I'll
let linux-kernel figure that out...

> Still I consider the patch to be better than the current behavior but still it
> is not fully functional, sorry for its submission; going to check the patches
> on kernel.org kernels next time.  Patch update from me possible later.

Thanks; hopefully this will be fixed in kernel.org sometime soon.  And
hopefully utrace will be merged, too.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Fix attaching to Linux stopped processes

[Mark Kettenis]

> Date: Mon, 18 Sep 2006 11:53:31 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> > So I checked.  Sure enough, on my 2.6.18-rc4
> > installation, this is _not_ what happens.  Instead, the traditional
> > thing happens: wait hangs and no new event is returned.
> > 
> > Is this new in kernel.org?  Or is it a Red Hat kernel patch?  Where
> > did it come from?
> 
> Thanks for checking it.
> OK, the patch was originally for GDB on Red Hat "kernel-2.6.17-1.2647.fc6".
> 
> Still it is reproducible even on both linux-2.6.17.13 and linux-2.6.18-rc4, you
> just need to "kill -CONT $pid" the inferior as gdb(1) will hang during
> attaching to it.  Screenshot (sorry for graphics):
> 	http://www.jankratochvil.net/priv/gdb.png
> gdb is built from the current clean CVS snapshot.
> 
> Also the patch looks right to me - after `PT_ATTACH' it is appropriate that
> `WSTOPSIG (status)' will report the original signal that stopped process,
> (WSTOPSIG (status) == SIGSTOP) <=> (process was in running/sleeping mode).

Whoa stop, the status returned by wait(2) doesn't tell you whether a
process is sleeping or running.  Traditionally, after invoking
ptrace(PT_ATTACH, ...) the kernel will always respond with a SIGSTOP.
Any information about any previous signals is lost.

This all smells like a race condition in the kernel to me.  The
linux-nat.c code is quite hairy already.  I'd suggest fixing the
kernel rather than adding more workarounds.

Mark

Re: [patch] Fix attaching to Linux stopped processes

[Jan Kratochvil]

On Sat, 23 Sep 2006 19:36:37 +0200, Mark Kettenis wrote:
...
> > Also the patch looks right to me - after `PT_ATTACH' it is appropriate that
> > `WSTOPSIG (status)' will report the original signal that stopped process,
> > (WSTOPSIG (status) == SIGSTOP) <=> (process was in running/sleeping mode).
> 
> Whoa stop, the status returned by wait(2) doesn't tell you whether a
> process is sleeping or running.  Traditionally, after invoking
> ptrace(PT_ATTACH, ...) the kernel will always respond with a SIGSTOP.
> Any information about any previous signals is lost.

(Red Hat) kernel 2.6.17-1.2647.fc6 reports SIGSTOP for running processes and
0 for processes already stopped before ptrace(PTRACE_ATTACH).  It does not
differentiate SIGTTIN vs. SIGSTOP etc., though, as I expected, thanks.

I found in POSIX that the waitpid(2) vs. ptrace(2) behavior is underfined:
	http://www.opengroup.org/onlinepubs/009695399/functions/wait.html
	Since ptrace() is not part of this volume of IEEE Std 1003.1-2001,
	[ I did not find any other ptrace(2) on http://www.opengroup.org/ . ]


> This all smells like a race condition in the kernel to me.  The
> linux-nat.c code is quite hairy already.  I'd suggest fixing the
> kernel rather than adding more workarounds.

Do you have some standard/specification to follow?
I found more that the WSTOPSIG() _should_ report the specific signal:
	http://www.scit.wlv.ac.uk/cgi-bin/mansec?3B+wait4
	the 8 bits higher up from the low-order 8 bits of w_status contain the
	number of the signal that caused the process to stop.
	See ptrace(2) and sigvec(3UCB).


Regards,
Jan