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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

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"