[rfc] linux-nat: Never PTRACE_CONT a stepping thread

[rfc] linux-nat: Never PTRACE_CONT a stepping thread

[Jan Kratochvil]

Hi,

as referenced in:
	[patch 3/4]#3 linux-nat: Do not respawn signals
	http://sourceware.org/ml/gdb-patches/2010-09/msg00360.html

If multiple signals happen besides SIGTRAP GDB still may switch from thread A
away (as not considering it stepping) to thread B for SIGUSR1 and accidentally
PTRACE_CONT thread A while resignalling SIGUSR1.

It probably could have its own testcase.
I will code one depending on the resolution of the #3 series above.

No regressions on {x86_64,x86_64-m32,i686}-fedora13-linux-gnu.


Thanks,
Jan


gdb/
2010-09-20  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdbthread.h (currently_stepping): New declaration.
	* infrun.c (currently_stepping): Remove the forward declaration.
	(currently_stepping): Make it global.
	* linux-nat.c (resume_callback) <lp->stopped && lp->status == 0>: New
	variables tp and step, initialized them.  Pass STEP to to_resume.
	Print also possibly "PTRACE_SINGLESTEP" if STEP.  Initialize LP->STEP.

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index cd24eaf..e89e88d 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -352,4 +352,6 @@ extern struct thread_info* inferior_thread (void);
 
 extern void update_thread_list (void);
 
+extern int currently_stepping (struct thread_info *tp);
+
 #endif /* GDBTHREAD_H */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 0720b31..8afdd77 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -74,8 +74,6 @@ static int follow_fork (void);
 static void set_schedlock_func (char *args, int from_tty,
 				struct cmd_list_element *c);
 
-static int currently_stepping (struct thread_info *tp);
-
 static int currently_stepping_or_nexting_callback (struct thread_info *tp,
 						   void *data);
 
@@ -4842,7 +4840,7 @@ infrun: not switching back to stepped thread, it has vanished\n");
 
 /* Is thread TP in the middle of single-stepping?  */
 
-static int
+int
 currently_stepping (struct thread_info *tp)
 {
   return ((tp->step_range_end && tp->step_resume_breakpoint == NULL)
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index a0eca0e..69b9a5f 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1820,20 +1820,26 @@ resume_callback (struct lwp_info *lp, void *data)
     }
   else if (lp->stopped && lp->status == 0)
     {
+      struct thread_info *tp = find_thread_ptid (lp->ptid);
+      /* lp->step may already contain a stale value.  */
+      int step = tp ? currently_stepping (tp) : 0;
+
       if (debug_linux_nat)
 	fprintf_unfiltered (gdb_stdlog,
-			    "RC:  PTRACE_CONT %s, 0, 0 (resuming sibling)\n",
+			    "RC:  %s %s, 0, 0 (resuming sibling)\n",
+			    step ? "PTRACE_SINGLESTEP" : "PTRACE_CONT",
 			    target_pid_to_str (lp->ptid));
 
       linux_ops->to_resume (linux_ops,
 			    pid_to_ptid (GET_LWP (lp->ptid)),
-			    0, TARGET_SIGNAL_0);
+			    step, TARGET_SIGNAL_0);
       if (debug_linux_nat)
 	fprintf_unfiltered (gdb_stdlog,
-			    "RC:  PTRACE_CONT %s, 0, 0 (resume sibling)\n",
+			    "RC:  %s %s, 0, 0 (resume sibling)\n",
+			    step ? "PTRACE_SINGLESTEP" : "PTRACE_CONT",
 			    target_pid_to_str (lp->ptid));
       lp->stopped = 0;
-      lp->step = 0;
+      lp->step = step;
       memset (&lp->siginfo, 0, sizeof (lp->siginfo));
       lp->stopped_by_watchpoint = 0;
     }

Re: [rfc] linux-nat: Never PTRACE_CONT a stepping thread

[Pedro Alves]

On Wednesday 22 September 2010 00:43:25, Jan Kratochvil wrote:
> Hi,
> 
> as referenced in:
> 	[patch 3/4]#3 linux-nat: Do not respawn signals
> 	http://sourceware.org/ml/gdb-patches/2010-09/msg00360.html
> 
> If multiple signals happen besides SIGTRAP GDB still may switch from thread A
> away (as not considering it stepping) to thread B for SIGUSR1 and accidentally
> PTRACE_CONT thread A while resignalling SIGUSR1.

Yeah.  I think you should put this in.

> It probably could have its own testcase.
> I will code one depending on the resolution of the #3 series above.

This patch alone on top of current mainline fixes the sigstep-threads.exp
test from <http://sourceware.org/ml/gdb-patches/2010-09/msg00360.html>.
Maybe check that test in along with this patch?
The corresponding remote.c patch (pasted below) that translates this
into a vCont with three actions (e.g., "vCont;C1e:795a;s:7959;c") also
fixes that test for gdbserver linux.

> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c

> -static int
> +int
>  currently_stepping (struct thread_info *tp)
>  {

It'd be much cleaner to make the target_resume interface similar
to gdbserver's target_ops->resume interface (pass in a list of
resume actions, similar to vCont), but that shouldn't be a
prerequisite.  Maybe someday...

-- 
Pedro Alves

---
 gdb/remote.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2010-10-16 17:20:41.000000000 +0100
+++ src/gdb/remote.c	2010-10-16 18:02:02.000000000 +0100
@@ -4416,6 +4416,12 @@ append_resumption (char *p, char *endp,
   return p;
 }
 
+static int
+currently_stepping_callback (struct thread_info *tp, void *data)
+{
+  return currently_stepping (tp);
+}
+
 /* Resume the remote inferior by using a "vCont" packet.  The thread
    to be resumed is PTID; STEP and SIGGNAL indicate whether the
    resumed thread should be single-stepped and/or signalled.  If PTID
@@ -4458,6 +4464,8 @@ remote_vcont_resume (ptid_t ptid, int st
     }
   else if (ptid_equal (ptid, minus_one_ptid) || ptid_is_pid (ptid))
     {
+      struct thread_info *tp;
+
       /* Resume all threads (of all processes, or of a single
 	 process), with preference for INFERIOR_PTID.  This assumes
 	 inferior_ptid belongs to the set of all threads we are about
@@ -4468,6 +4476,12 @@ remote_vcont_resume (ptid_t ptid, int st
 	  p = append_resumption (p, endp, inferior_ptid, step, siggnal);
 	}
 
+      tp = iterate_over_threads (currently_stepping_callback, NULL);
+      if (tp && !ptid_equal (tp->ptid, inferior_ptid))
+	{
+	  p = append_resumption (p, endp, tp->ptid, 1, TARGET_SIGNAL_0);
+	}
+
       /* And continue others without a signal.  */
       p = append_resumption (p, endp, ptid, /*step=*/ 0, TARGET_SIGNAL_0);
     }

Re: [rfc] linux-nat: Never PTRACE_CONT a stepping thread

[Jan Kratochvil]

On Sat, 16 Oct 2010 19:09:52 +0200, Pedro Alves wrote:
> This patch alone on top of current mainline fixes the sigstep-threads.exp
> test from <http://sourceware.org/ml/gdb-patches/2010-09/msg00360.html>.
> Maybe check that test in along with this patch?

OK, yes, I will recheck that patch on top of this one.


> The corresponding remote.c patch (pasted below) that translates this
> into a vCont with three actions (e.g., "vCont;C1e:795a;s:7959;c") also
> fixes that test for gdbserver linux.

Included it.


> It'd be much cleaner to make the target_resume interface similar
> to gdbserver's target_ops->resume interface (pass in a list of
> resume actions, similar to vCont), but that shouldn't be a
> prerequisite.  Maybe someday...

I agree that interface is better.  Not a scope of this patch, though.


Checked-in.

Thanks for the review.


Jan


http://sourceware.org/ml/gdb-cvs/2010-10/msg00102.html

--- src/gdb/ChangeLog	2010/10/17 17:45:16	1.12266
+++ src/gdb/ChangeLog	2010/10/17 18:24:46	1.12267
@@ -1,4 +1,19 @@
 2010-10-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
+	    Pedro Alves  <pedro@codesourcery.com>
+
+	* gdbthread.h (currently_stepping): New declaration.
+	* infrun.c (currently_stepping): Remove the forward declaration.
+	(currently_stepping): Make it global.
+	* linux-nat.c (resume_callback) <lp->stopped && lp->status == 0>: New
+	variables tp and step, initialized them.  Pass STEP to to_resume.
+	Print also possibly "PTRACE_SINGLESTEP" if STEP.  Initialize LP->STEP.
+	* remote.c (currently_stepping_callback): New.
+	(remote_vcont_resume)
+	<ptid_equal (ptid, minus_one_ptid) || ptid_is_pid (ptid)>:
+	New variable tp.  Call currently_stepping_callback and step such
+	thread.
+
+2010-10-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* infrun.c (follow_exec): Replace symbol_file_add_main by
 	symbol_file_add with SYMFILE_DEFER_BP_RESET, set_initial_language and
--- src/gdb/gdbthread.h	2010/01/12 21:40:24	1.54
+++ src/gdb/gdbthread.h	2010/10/17 18:24:47	1.55
@@ -352,4 +352,6 @@
 
 extern void update_thread_list (void);
 
+extern int currently_stepping (struct thread_info *tp);
+
 #endif /* GDBTHREAD_H */
--- src/gdb/infrun.c	2010/10/17 17:45:16	1.452
+++ src/gdb/infrun.c	2010/10/17 18:24:47	1.453
@@ -74,8 +74,6 @@
 static void set_schedlock_func (char *args, int from_tty,
 				struct cmd_list_element *c);
 
-static int currently_stepping (struct thread_info *tp);
-
 static int currently_stepping_or_nexting_callback (struct thread_info *tp,
 						   void *data);
 
@@ -4851,7 +4849,7 @@
 
 /* Is thread TP in the middle of single-stepping?  */
 
-static int
+int
 currently_stepping (struct thread_info *tp)
 {
   return ((tp->step_range_end && tp->step_resume_breakpoint == NULL)
--- src/gdb/linux-nat.c	2010/09/06 13:59:02	1.184
+++ src/gdb/linux-nat.c	2010/10/17 18:24:47	1.185
@@ -1820,20 +1820,26 @@
     }
   else if (lp->stopped && lp->status == 0)
     {
+      struct thread_info *tp = find_thread_ptid (lp->ptid);
+      /* lp->step may already contain a stale value.  */
+      int step = tp ? currently_stepping (tp) : 0;
+
       if (debug_linux_nat)
 	fprintf_unfiltered (gdb_stdlog,
-			    "RC:  PTRACE_CONT %s, 0, 0 (resuming sibling)\n",
+			    "RC:  %s %s, 0, 0 (resuming sibling)\n",
+			    step ? "PTRACE_SINGLESTEP" : "PTRACE_CONT",
 			    target_pid_to_str (lp->ptid));
 
       linux_ops->to_resume (linux_ops,
 			    pid_to_ptid (GET_LWP (lp->ptid)),
-			    0, TARGET_SIGNAL_0);
+			    step, TARGET_SIGNAL_0);
       if (debug_linux_nat)
 	fprintf_unfiltered (gdb_stdlog,
-			    "RC:  PTRACE_CONT %s, 0, 0 (resume sibling)\n",
+			    "RC:  %s %s, 0, 0 (resume sibling)\n",
+			    step ? "PTRACE_SINGLESTEP" : "PTRACE_CONT",
 			    target_pid_to_str (lp->ptid));
       lp->stopped = 0;
-      lp->step = 0;
+      lp->step = step;
       memset (&lp->siginfo, 0, sizeof (lp->siginfo));
       lp->stopped_by_watchpoint = 0;
     }
--- src/gdb/remote.c	2010/07/28 20:20:26	1.421
+++ src/gdb/remote.c	2010/10/17 18:24:47	1.422
@@ -4416,6 +4416,12 @@
   return p;
 }
 
+static int
+currently_stepping_callback (struct thread_info *tp, void *data)
+{
+  return currently_stepping (tp);
+}
+
 /* Resume the remote inferior by using a "vCont" packet.  The thread
    to be resumed is PTID; STEP and SIGGNAL indicate whether the
    resumed thread should be single-stepped and/or signalled.  If PTID
@@ -4458,6 +4464,8 @@
     }
   else if (ptid_equal (ptid, minus_one_ptid) || ptid_is_pid (ptid))
     {
+      struct thread_info *tp;
+
       /* Resume all threads (of all processes, or of a single
 	 process), with preference for INFERIOR_PTID.  This assumes
 	 inferior_ptid belongs to the set of all threads we are about
@@ -4468,6 +4476,12 @@
 	  p = append_resumption (p, endp, inferior_ptid, step, siggnal);
 	}
 
+      tp = iterate_over_threads (currently_stepping_callback, NULL);
+      if (tp && !ptid_equal (tp->ptid, inferior_ptid))
+	{
+	  p = append_resumption (p, endp, tp->ptid, 1, TARGET_SIGNAL_0);
+	}
+
       /* And continue others without a signal.  */
       p = append_resumption (p, endp, ptid, /*step=*/ 0, TARGET_SIGNAL_0);
     }
--- src/gdb/testsuite/ChangeLog	2010/10/17 17:45:17	1.2482
+++ src/gdb/testsuite/ChangeLog	2010/10/17 18:24:47	1.2483
@@ -1,5 +1,10 @@
 2010-10-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
+	* gdb.threads/sigstep-threads.exp: New file.
+	* gdb.threads/sigstep-threads.c: New file.
+
+2010-10-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
 	* gdb.base/pie-execl.exp: New file.
 	* gdb.base/pie-execl.c: New file.
 
--- src/gdb/testsuite/gdb.threads/sigstep-threads.c
+++ src/gdb/testsuite/gdb.threads/sigstep-threads.c	2010-10-17 18:24:59.596239000 +0000
@@ -0,0 +1,54 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 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 3 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, see <http://www.gnu.org/licenses/>.  */
+
+#include <pthread.h>
+#include <assert.h>
+#include <signal.h>
+
+#include <asm/unistd.h>
+#include <unistd.h>
+#define tgkill(tgid, tid, sig) syscall (__NR_tgkill, (tgid), (tid), (sig))
+#define gettid() syscall (__NR_gettid)
+
+static volatile int var;
+
+static void
+handler (int signo)	/* step-0 */
+{			/* step-0 */
+  var++;		/* step-1 */
+  tgkill (getpid (), gettid (), SIGUSR1);	/* step-2 */
+}
+
+static void *
+start (void *arg)
+{
+  signal (SIGUSR1, handler);
+  tgkill (getpid (), gettid (), SIGUSR1);
+  assert (0);
+
+  return NULL;
+}
+
+int
+main (void)
+{
+  pthread_t thread;
+
+  pthread_create (&thread, NULL, start, NULL);
+  start (NULL);	/* main-start */
+  return 0;
+}
--- src/gdb/testsuite/gdb.threads/sigstep-threads.exp
+++ src/gdb/testsuite/gdb.threads/sigstep-threads.exp	2010-10-17 18:24:59.896865000 +0000
@@ -0,0 +1,74 @@
+# Copyright 2010 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 3 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, see <http://www.gnu.org/licenses/>.
+
+set testfile sigstep-threads
+set srcfile ${testfile}.c
+set executable ${testfile}
+set binfile ${objdir}/${subdir}/${executable}
+
+if { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    untested ${testfile}.exp
+    return -1
+}
+
+clean_restart $executable
+
+if ![runto_main] {
+    return -1;
+}
+
+# `noprint' would not test the full logic of GDB.
+gdb_test "handle SIGUSR1 nostop print pass" "\r\nSIGUSR1\[ \t\]+No\[ \t\]+Yes\[ \t\]+Yes\[ \t\].*"
+
+gdb_test_no_output "set scheduler-locking off"
+
+gdb_breakpoint [gdb_get_line_number "step-1"]
+gdb_test_no_output {set $step1=$bpnum}
+gdb_continue_to_breakpoint "step-1" ".* step-1 .*"
+gdb_test_no_output {disable $step1}
+
+# 1 as we are now stopped at the `step-1' label.
+set step_at 1
+for {set i 0} {$i < 100} {incr i} {
+    set test "step $i"
+    # Presume this step failed - as in the case of a timeout.
+    set failed 1
+    gdb_test_multiple "step" $test {
+	-re "\r\nProgram received signal SIGUSR1, User defined signal 1.\r\n" {
+	    exp_continue -continue_timer
+	}
+	-re "step-(\[012\]).*\r\n$gdb_prompt $" {
+	    set now $expect_out(1,string)
+	    if {$step_at == 2 && $now == 1} {
+		set failed 0
+	    } elseif {$step_at == 1 && $now == 2} {
+		set failed 0
+		# Continue over the re-signalling back to the handle entry.
+		gdb_test_no_output {enable $step1} ""
+		gdb_test "continue" " step-1 .*" ""
+		set now 1
+		gdb_test_no_output {disable $step1} ""
+	    } else  {
+		fail $test
+	    }
+	    set step_at $now
+	}
+    }
+    if $failed {
+	return
+    }
+}
+# We can never reliably say the racy problematic case has been tested.
+pass "step"

Re: [rfc] linux-nat: Never PTRACE_CONT a stepping thread

[Pedro Alves]

On Sunday 17 October 2010 19:27:45, Jan Kratochvil wrote:
> On Sat, 16 Oct 2010 19:09:52 +0200, Pedro Alves wrote:
> > This patch alone on top of current mainline fixes the sigstep-threads.exp
> > test from <http://sourceware.org/ml/gdb-patches/2010-09/msg00360.html>.
> > Maybe check that test in along with this patch?
> 
> OK, yes, I will recheck that patch on top of this one.
> 
> 
> > The corresponding remote.c patch (pasted below) that translates this
> > into a vCont with three actions (e.g., "vCont;C1e:795a;s:7959;c") also
> > fixes that test for gdbserver linux.
> 
> Included it.
> 
> 
> > It'd be much cleaner to make the target_resume interface similar
> > to gdbserver's target_ops->resume interface (pass in a list of
> > resume actions, similar to vCont), but that shouldn't be a
> > prerequisite.  Maybe someday...
> 
> I agree that interface is better.  Not a scope of this patch, though.


Thanks.  I just belatedly realized that this probably
breaks software single-step archs though.  :-/

> 
> 
> Checked-in.
> 
> Thanks for the review.
> 
> 
> Jan
> 
> 
> http://sourceware.org/ml/gdb-cvs/2010-10/msg00102.html
> 
> --- src/gdb/ChangeLog	2010/10/17 17:45:16	1.12266
> +++ src/gdb/ChangeLog	2010/10/17 18:24:46	1.12267
> @@ -1,4 +1,19 @@
>  2010-10-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
> +	    Pedro Alves  <pedro@codesourcery.com>
> +
> +	* gdbthread.h (currently_stepping): New declaration.
> +	* infrun.c (currently_stepping): Remove the forward declaration.
> +	(currently_stepping): Make it global.
> +	* linux-nat.c (resume_callback) <lp->stopped && lp->status == 0>: New
> +	variables tp and step, initialized them.  Pass STEP to to_resume.
> +	Print also possibly "PTRACE_SINGLESTEP" if STEP.  Initialize LP->STEP.
> +	* remote.c (currently_stepping_callback): New.
> +	(remote_vcont_resume)
> +	<ptid_equal (ptid, minus_one_ptid) || ptid_is_pid (ptid)>:
> +	New variable tp.  Call currently_stepping_callback and step such
> +	thread.
> +
> +2010-10-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
>  
>  	* infrun.c (follow_exec): Replace symbol_file_add_main by
>  	symbol_file_add with SYMFILE_DEFER_BP_RESET, set_initial_language and
> --- src/gdb/gdbthread.h	2010/01/12 21:40:24	1.54
> +++ src/gdb/gdbthread.h	2010/10/17 18:24:47	1.55
> @@ -352,4 +352,6 @@
>  
>  extern void update_thread_list (void);
>  
> +extern int currently_stepping (struct thread_info *tp);
> +
>  #endif /* GDBTHREAD_H */
> --- src/gdb/infrun.c	2010/10/17 17:45:16	1.452
> +++ src/gdb/infrun.c	2010/10/17 18:24:47	1.453
> @@ -74,8 +74,6 @@
>  static void set_schedlock_func (char *args, int from_tty,
>  				struct cmd_list_element *c);
>  
> -static int currently_stepping (struct thread_info *tp);
> -
>  static int currently_stepping_or_nexting_callback (struct thread_info *tp,
>  						   void *data);
>  
> @@ -4851,7 +4849,7 @@
>  
>  /* Is thread TP in the middle of single-stepping?  */
>  
> -static int
> +int
>  currently_stepping (struct thread_info *tp)
>  {
>    return ((tp->step_range_end && tp->step_resume_breakpoint == NULL)
> --- src/gdb/linux-nat.c	2010/09/06 13:59:02	1.184
> +++ src/gdb/linux-nat.c	2010/10/17 18:24:47	1.185
> @@ -1820,20 +1820,26 @@
>      }
>    else if (lp->stopped && lp->status == 0)
>      {
> +      struct thread_info *tp = find_thread_ptid (lp->ptid);
> +      /* lp->step may already contain a stale value.  */
> +      int step = tp ? currently_stepping (tp) : 0;
> +
>        if (debug_linux_nat)
>  	fprintf_unfiltered (gdb_stdlog,
> -			    "RC:  PTRACE_CONT %s, 0, 0 (resuming sibling)\n",
> +			    "RC:  %s %s, 0, 0 (resuming sibling)\n",
> +			    step ? "PTRACE_SINGLESTEP" : "PTRACE_CONT",
>  			    target_pid_to_str (lp->ptid));
>  
>        linux_ops->to_resume (linux_ops,
>  			    pid_to_ptid (GET_LWP (lp->ptid)),
> -			    0, TARGET_SIGNAL_0);
> +			    step, TARGET_SIGNAL_0);
>        if (debug_linux_nat)
>  	fprintf_unfiltered (gdb_stdlog,
> -			    "RC:  PTRACE_CONT %s, 0, 0 (resume sibling)\n",
> +			    "RC:  %s %s, 0, 0 (resume sibling)\n",
> +			    step ? "PTRACE_SINGLESTEP" : "PTRACE_CONT",
>  			    target_pid_to_str (lp->ptid));
>        lp->stopped = 0;
> -      lp->step = 0;
> +      lp->step = step;
>        memset (&lp->siginfo, 0, sizeof (lp->siginfo));
>        lp->stopped_by_watchpoint = 0;
>      }
> --- src/gdb/remote.c	2010/07/28 20:20:26	1.421
> +++ src/gdb/remote.c	2010/10/17 18:24:47	1.422
> @@ -4416,6 +4416,12 @@
>    return p;
>  }
>  
> +static int
> +currently_stepping_callback (struct thread_info *tp, void *data)
> +{
> +  return currently_stepping (tp);
> +}
> +
>  /* Resume the remote inferior by using a "vCont" packet.  The thread
>     to be resumed is PTID; STEP and SIGGNAL indicate whether the
>     resumed thread should be single-stepped and/or signalled.  If PTID
> @@ -4458,6 +4464,8 @@
>      }
>    else if (ptid_equal (ptid, minus_one_ptid) || ptid_is_pid (ptid))
>      {
> +      struct thread_info *tp;
> +
>        /* Resume all threads (of all processes, or of a single
>  	 process), with preference for INFERIOR_PTID.  This assumes
>  	 inferior_ptid belongs to the set of all threads we are about
> @@ -4468,6 +4476,12 @@
>  	  p = append_resumption (p, endp, inferior_ptid, step, siggnal);
>  	}
>  
> +      tp = iterate_over_threads (currently_stepping_callback, NULL);
> +      if (tp && !ptid_equal (tp->ptid, inferior_ptid))
> +	{
> +	  p = append_resumption (p, endp, tp->ptid, 1, TARGET_SIGNAL_0);
> +	}
> +
>        /* And continue others without a signal.  */
>        p = append_resumption (p, endp, ptid, /*step=*/ 0, TARGET_SIGNAL_0);
>      }
> --- src/gdb/testsuite/ChangeLog	2010/10/17 17:45:17	1.2482
> +++ src/gdb/testsuite/ChangeLog	2010/10/17 18:24:47	1.2483
> @@ -1,5 +1,10 @@
>  2010-10-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
>  
> +	* gdb.threads/sigstep-threads.exp: New file.
> +	* gdb.threads/sigstep-threads.c: New file.
> +
> +2010-10-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
> +
>  	* gdb.base/pie-execl.exp: New file.
>  	* gdb.base/pie-execl.c: New file.
>  
> --- src/gdb/testsuite/gdb.threads/sigstep-threads.c
> +++ src/gdb/testsuite/gdb.threads/sigstep-threads.c	2010-10-17 18:24:59.596239000 +0000
> @@ -0,0 +1,54 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2010 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 3 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, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <pthread.h>
> +#include <assert.h>
> +#include <signal.h>
> +
> +#include <asm/unistd.h>
> +#include <unistd.h>
> +#define tgkill(tgid, tid, sig) syscall (__NR_tgkill, (tgid), (tid), (sig))
> +#define gettid() syscall (__NR_gettid)
> +
> +static volatile int var;
> +
> +static void
> +handler (int signo)	/* step-0 */
> +{			/* step-0 */
> +  var++;		/* step-1 */
> +  tgkill (getpid (), gettid (), SIGUSR1);	/* step-2 */
> +}
> +
> +static void *
> +start (void *arg)
> +{
> +  signal (SIGUSR1, handler);
> +  tgkill (getpid (), gettid (), SIGUSR1);
> +  assert (0);
> +
> +  return NULL;
> +}
> +
> +int
> +main (void)
> +{
> +  pthread_t thread;
> +
> +  pthread_create (&thread, NULL, start, NULL);
> +  start (NULL);	/* main-start */
> +  return 0;
> +}
> --- src/gdb/testsuite/gdb.threads/sigstep-threads.exp
> +++ src/gdb/testsuite/gdb.threads/sigstep-threads.exp	2010-10-17 18:24:59.896865000 +0000
> @@ -0,0 +1,74 @@
> +# Copyright 2010 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 3 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, see <http://www.gnu.org/licenses/>.
> +
> +set testfile sigstep-threads
> +set srcfile ${testfile}.c
> +set executable ${testfile}
> +set binfile ${objdir}/${subdir}/${executable}
> +
> +if { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
> +    untested ${testfile}.exp
> +    return -1
> +}
> +
> +clean_restart $executable
> +
> +if ![runto_main] {
> +    return -1;
> +}
> +
> +# `noprint' would not test the full logic of GDB.
> +gdb_test "handle SIGUSR1 nostop print pass" "\r\nSIGUSR1\[ \t\]+No\[ \t\]+Yes\[ \t\]+Yes\[ \t\].*"
> +
> +gdb_test_no_output "set scheduler-locking off"
> +
> +gdb_breakpoint [gdb_get_line_number "step-1"]
> +gdb_test_no_output {set $step1=$bpnum}
> +gdb_continue_to_breakpoint "step-1" ".* step-1 .*"
> +gdb_test_no_output {disable $step1}
> +
> +# 1 as we are now stopped at the `step-1' label.
> +set step_at 1
> +for {set i 0} {$i < 100} {incr i} {
> +    set test "step $i"
> +    # Presume this step failed - as in the case of a timeout.
> +    set failed 1
> +    gdb_test_multiple "step" $test {
> +	-re "\r\nProgram received signal SIGUSR1, User defined signal 1.\r\n" {
> +	    exp_continue -continue_timer
> +	}
> +	-re "step-(\[012\]).*\r\n$gdb_prompt $" {
> +	    set now $expect_out(1,string)
> +	    if {$step_at == 2 && $now == 1} {
> +		set failed 0
> +	    } elseif {$step_at == 1 && $now == 2} {
> +		set failed 0
> +		# Continue over the re-signalling back to the handle entry.
> +		gdb_test_no_output {enable $step1} ""
> +		gdb_test "continue" " step-1 .*" ""
> +		set now 1
> +		gdb_test_no_output {disable $step1} ""
> +	    } else  {
> +		fail $test
> +	    }
> +	    set step_at $now
> +	}
> +    }
> +    if $failed {
> +	return
> +    }
> +}
> +# We can never reliably say the racy problematic case has been tested.
> +pass "step"
> 


-- 
Pedro Alves

Re: [rfc] linux-nat: Never PTRACE_CONT a stepping thread

[Jan Kratochvil]

On Sun, 17 Oct 2010 21:04:18 +0200, Pedro Alves wrote:
> I just belatedly realized that this probably
> breaks software single-step archs though.  :-/

I have run the testsuite on arm-fedora12-linux-gnu and the only
regression-candidate there is gdb.threads/local-watch-wrong-thread.exp .

But that one is reproducible even on x86_64-fedora14snapshot-linux-gnu just by
`set can-use-hw-watchpoints 0'.  That testcase in fact should not have
executed on arm due to:
	if [target_info exists gdb,no_hardware_watchpoints] { return 0; }

Checking more why that change happened.

I do not see why it should fail in general.  insert_single_step_breakpoint
already supports two existing single step breakpoints, one gets used for
inferior_ptid and one for stepping ptid.  Maybe the support of two single step
breakpoints is there for some inferior code constructs, in such case
I understand two would be no longer enough.


Thanks,
Jan

Re: [rfc] linux-nat: Never PTRACE_CONT a stepping thread

[Pedro Alves]

On Monday 18 October 2010 09:51:38, Jan Kratochvil wrote:
> On Sun, 17 Oct 2010 21:04:18 +0200, Pedro Alves wrote:
> > I just belatedly realized that this probably
> > breaks software single-step archs though.  :-/
> 
> I have run the testsuite on arm-fedora12-linux-gnu and the only (...)

Thanks for testing.  Does the new test pass there?

What I actually remembered was that using PTRACE_SINGLESTEP,
and leaving lwp->step true on software single-step archs
probably isn't a good idea.  And it crossed my mind that we'd
probably need to make infrun.c know it needs to insert
software single-step breakpoint in this case too.

> I do not see why it should fail in general.  insert_single_step_breakpoint
> already supports two existing single step breakpoints, one gets used for
> inferior_ptid and one for stepping ptid.  

But hmm, you left me confused.  In the case at hand, from reading the code,
by the time you get to linux_nat_resume, infrun.c has context-switched
inferior_ptid to the thread that we're "delivering" the signal to.  Right
after context-switching, infrun pulled the previous single-step breakpoints
out of target:

  if (singlestep_breakpoints_inserted_p)
    {
      /* Pull the single step breakpoints out of the target. */
      remove_single_step_breakpoints ();
      singlestep_breakpoints_inserted_p = 0;
    }

when handle_inferior_event handles the random signal, and decides
to forward it to the target, it calls keep_going, which then
calls resume, passing it currently_stepping(ecs->event_thread).
ecs->event_thread was _not_ stepping, so _no_ single step
breakpoints are inserted in the target.  I'm following this
only by reading the code, but since I see:
 
 "vCont;C1e:795a;s:7959;c"

coming out, and not

 "vCont;S1e:795a;s:7959;c"

I'm led to believe the analysis is correct.

> Maybe the support of two single step
> breakpoints is there for some inferior code constructs, in such case
> I understand two would be no longer enough.

Yes, there are constructs where we need two breakpoints, usually in
cases related to conditional branching where we can't tell whether
the branch will be taken or not, so we need one breakpoint at the
branch destination and a breakpoint after the branch instruction.
Grepping for insert_single_step_breakpoint will find all cases.


I'm also wondering if tweaking the test to use a signal less than
SIGTRAP (5) rather than SIGUSR1 (10) so that waitpid always
picks it up over SIGTRAP if both are pending simulateneously
reveals that we need to rethink this?

-- 
Pedro Alves

Re: [rfc] linux-nat: Never PTRACE_CONT a stepping thread

[Jan Kratochvil]

On Tue, 19 Oct 2010 21:24:04 +0200, Pedro Alves wrote:
> What I actually remembered was that using PTRACE_SINGLESTEP,
> and leaving lwp->step true on software single-step archs
> probably isn't a good idea.  And it crossed my mind that we'd
> probably need to make infrun.c know it needs to insert
> software single-step breakpoint in this case too.

OK, I see now the software singlestep is _above_ this linux-nat.c patched
code, not _below_.

Therefore I have reverted it now as it was at a wrong place as the new patch
will need to revert this one anyway.


> I'm also wondering if tweaking the test to use a signal less than
> SIGTRAP (5) rather than SIGUSR1 (10) so that waitpid always
> picks it up over SIGTRAP if both are pending simulateneously
> reveals that we need to rethink this?

OK.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2010-11/msg00005.html

--- src/gdb/ChangeLog	2010/11/01 07:00:09	1.12283
+++ src/gdb/ChangeLog	2010/11/02 01:37:30	1.12284
@@ -1,3 +1,20 @@
+2010-11-02  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	Revert:
+	2010-10-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
+		    Pedro Alves  <pedro@codesourcery.com>
+	* gdbthread.h (currently_stepping): New declaration.
+	* infrun.c (currently_stepping): Remove the forward declaration.
+	(currently_stepping): Make it global.
+	* linux-nat.c (resume_callback) <lp->stopped && lp->status == 0>: New
+	variables tp and step, initialized them.  Pass STEP to to_resume.
+	Print also possibly "PTRACE_SINGLESTEP" if STEP.  Initialize LP->STEP.
+	* remote.c (currently_stepping_callback): New.
+	(remote_vcont_resume)
+	<ptid_equal (ptid, minus_one_ptid) || ptid_is_pid (ptid)>:
+	New variable tp.  Call currently_stepping_callback and step such
+	thread.
+
 2010-11-01  Hui Zhu  <teawater@gmail.com>
 
 	* tracepoint.c (tfile_xfer_partial): Change lma to vma.
--- src/gdb/gdbthread.h	2010/10/17 18:24:47	1.55
+++ src/gdb/gdbthread.h	2010/11/02 01:37:31	1.56
@@ -352,6 +352,4 @@
 
 extern void update_thread_list (void);
 
-extern int currently_stepping (struct thread_info *tp);
-
 #endif /* GDBTHREAD_H */
--- src/gdb/infrun.c	2010/10/17 18:24:47	1.453
+++ src/gdb/infrun.c	2010/11/02 01:37:31	1.454
@@ -74,6 +74,8 @@
 static void set_schedlock_func (char *args, int from_tty,
 				struct cmd_list_element *c);
 
+static int currently_stepping (struct thread_info *tp);
+
 static int currently_stepping_or_nexting_callback (struct thread_info *tp,
 						   void *data);
 
@@ -4849,7 +4851,7 @@
 
 /* Is thread TP in the middle of single-stepping?  */
 
-int
+static int
 currently_stepping (struct thread_info *tp)
 {
   return ((tp->step_range_end && tp->step_resume_breakpoint == NULL)
--- src/gdb/linux-nat.c	2010/10/17 18:24:47	1.185
+++ src/gdb/linux-nat.c	2010/11/02 01:37:32	1.186
@@ -1820,26 +1820,20 @@
     }
   else if (lp->stopped && lp->status == 0)
     {
-      struct thread_info *tp = find_thread_ptid (lp->ptid);
-      /* lp->step may already contain a stale value.  */
-      int step = tp ? currently_stepping (tp) : 0;
-
       if (debug_linux_nat)
 	fprintf_unfiltered (gdb_stdlog,
-			    "RC:  %s %s, 0, 0 (resuming sibling)\n",
-			    step ? "PTRACE_SINGLESTEP" : "PTRACE_CONT",
+			    "RC:  PTRACE_CONT %s, 0, 0 (resuming sibling)\n",
 			    target_pid_to_str (lp->ptid));
 
       linux_ops->to_resume (linux_ops,
 			    pid_to_ptid (GET_LWP (lp->ptid)),
-			    step, TARGET_SIGNAL_0);
+			    0, TARGET_SIGNAL_0);
       if (debug_linux_nat)
 	fprintf_unfiltered (gdb_stdlog,
-			    "RC:  %s %s, 0, 0 (resume sibling)\n",
-			    step ? "PTRACE_SINGLESTEP" : "PTRACE_CONT",
+			    "RC:  PTRACE_CONT %s, 0, 0 (resume sibling)\n",
 			    target_pid_to_str (lp->ptid));
       lp->stopped = 0;
-      lp->step = step;
+      lp->step = 0;
       memset (&lp->siginfo, 0, sizeof (lp->siginfo));
       lp->stopped_by_watchpoint = 0;
     }
--- src/gdb/remote.c	2010/10/20 09:10:48	1.423
+++ src/gdb/remote.c	2010/11/02 01:37:32	1.424
@@ -4416,12 +4416,6 @@
   return p;
 }
 
-static int
-currently_stepping_callback (struct thread_info *tp, void *data)
-{
-  return currently_stepping (tp);
-}
-
 /* Resume the remote inferior by using a "vCont" packet.  The thread
    to be resumed is PTID; STEP and SIGGNAL indicate whether the
    resumed thread should be single-stepped and/or signalled.  If PTID
@@ -4464,8 +4458,6 @@
     }
   else if (ptid_equal (ptid, minus_one_ptid) || ptid_is_pid (ptid))
     {
-      struct thread_info *tp;
-
       /* Resume all threads (of all processes, or of a single
 	 process), with preference for INFERIOR_PTID.  This assumes
 	 inferior_ptid belongs to the set of all threads we are about
@@ -4476,12 +4468,6 @@
 	  p = append_resumption (p, endp, inferior_ptid, step, siggnal);
 	}
 
-      tp = iterate_over_threads (currently_stepping_callback, NULL);
-      if (tp && !ptid_equal (tp->ptid, inferior_ptid))
-	{
-	  p = append_resumption (p, endp, tp->ptid, 1, TARGET_SIGNAL_0);
-	}
-
       /* And continue others without a signal.  */
       p = append_resumption (p, endp, ptid, /*step=*/ 0, TARGET_SIGNAL_0);
     }
--- src/gdb/testsuite/ChangeLog	2010/10/20 23:58:06	1.2490
+++ src/gdb/testsuite/ChangeLog	2010/11/02 01:37:32	1.2491
@@ -1,3 +1,10 @@
+2010-11-02  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	Revert:
+	2010-10-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
+	* gdb.threads/sigstep-threads.exp: New file.
+	* gdb.threads/sigstep-threads.c: New file.
+
 2010-10-20  Michael Snyder  <msnyder@vmware.com>
 
 	* gdb.threads/fork-child-threads.exp: Don't run on remote target.
[ The two files got deleted.  ]