Re: RFA: Fix lin-lwp SIGINT handling for 2.6

[Daniel Jacobowitz <drow at mvista dot com>]

On Wed, Aug 27, 2003 at 10:42:25AM -0700, Michael Snyder wrote:
> Daniel Jacobowitz wrote:
> >On Tue, Aug 26, 2003 at 07:22:13PM -0700, Michael Snyder wrote:
> >
> >>Daniel Jacobowitz wrote:
> >>
> >>>This patch fixes a number of inconsistent regressions in schedlock.exp 
> >>>and
> >>>pthreads.exp on 2.6-series kernels, using LinuxThreads.  Red Hat 2.4 
> >>>kernels
> >>>have the same problems; the fix will work there too iff there is an 
> >>>update
> >>>which exports ShdPnd in /proc.
> >>>
> >>>The problem is that the SIGINT is delivered to every thread, and not
> >>>properly flushed.  So we go later to step or continue and get an "echo" 
> >>>of
> >>>the original SIGINT.  This is a timing problem caused by the 
> >>>introduction of
> >>>two signal queues in the kernel; the SIGSTOP is now guaranteed to be
> >>>delivered before the SIGINT, since it's on the thread-specific queue.  It
> >>>used to be that SIGINT would be delivered first; they were on the same
> >>>queue, and SIGINT was lower numbered.
> >>>
> >>>So we have to check whether a SIGINT is pending (and not blocked/ignored)
> >>>for the current thread after we stop it, and resume the thread to catch 
> >>>the
> >>>SIGINT if so.
> >>>
> >>>It's not a perfect fix, but it's enough more reliable than the current
> >>>scheme that I haven't been able to reproduce the problems.  OK?  HEAD 
> >>>only;
> >>>this is a bit of an annoyance, but too risky for the branch, IMO.
> >>>
> >>
> >>I've been worried about that.
> >>
> >>May I suggest that the function that opens a proc file belongs
> >>in linux-proc.c?
> >
> >
> >It is in linux-proc.c :)
> >
> >        * linux-proc.c (linux_proc_add_line_to_sigset): New function.
> >        (linux_proc_pending_signals): New function.
> >        * linux-nat.h (linux_proc_pending_signals): Add prototype
> 
> Oh.  Well then -- never mind.  ;-)
> Speaking as the originator of linux-proc.c, it's fine with me
> to put it there.  I don't feel competent to judge the correctness
> of the code, though.

With some luke-warm approval from Mark, I've checked this in.  I added
only an additional check in flush_callback; if there is only one LWP in
the LWP list, it may have been the last thread exiting, so check if
it's alive before trying to flush its signals.  Another timing problem
which didn't show up before but somehow does now.

Now all threads tests PASS, KFAIL, or XFAIL on my system except for a
couple backtracing through nanosleep.  Another prologue analyzer issue
I assume - I'll be back to that momentarily.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2003-09-07  Daniel Jacobowitz  <drow@mvista.com>

	* lin-lwp.c (detach_callback): Don't call stop_wait_callback.
	(stop_wait_callback): Handle !lp->signalled also.
	(lin_lwp_has_pending, flush_callback): New functions.
	(lin_lwp_wait): Call flush_callback.
	* linux-proc.c (linux_proc_add_line_to_sigset): New function.
	(linux_proc_pending_signals): New function.
	* linux-nat.h (linux_proc_pending_signals): Add prototype.

Index: lin-lwp.c
===================================================================
RCS file: /cvs/src/src/gdb/lin-lwp.c,v
retrieving revision 1.50
diff -u -p -r1.50 lin-lwp.c
--- lin-lwp.c	28 Aug 2003 14:20:03 -0000	1.50
+++ lin-lwp.c	7 Sep 2003 18:46:07 -0000
@@ -417,7 +417,12 @@ detach_callback (struct lwp_info *lp, vo
       lp->stopped = 0;
       lp->signalled = 0;
       lp->status = 0;
-      stop_wait_callback (lp, NULL);
+      /* FIXME drow/2003-08-26: There was a call to stop_wait_callback
+	 here.  But since lp->signalled was cleared above,
+	 stop_wait_callback didn't do anything; the process was left
+	 running.  Shouldn't we be waiting for it to stop?
+	 I've removed the call, since stop_wait_callback now does do
+	 something when called with lp->signalled == 0.  */
 
       gdb_assert (lp->status == 0 || WIFSTOPPED (lp->status));
     }
@@ -696,7 +701,7 @@ stop_wait_callback (struct lwp_info *lp,
 {
   sigset_t *flush_mask = data;
 
-  if (!lp->stopped && lp->signalled)
+  if (!lp->stopped)
     {
       int status;
 
@@ -707,6 +712,12 @@ stop_wait_callback (struct lwp_info *lp,
       /* Ignore any signals in FLUSH_MASK.  */
       if (flush_mask && sigismember (flush_mask, WSTOPSIG (status)))
 	{
+	  if (!lp->signalled)
+	    {
+	      lp->stopped = 1;
+	      return 0;
+	    }
+
 	  errno = 0;
 	  ptrace (PTRACE_CONT, GET_LWP (lp->ptid), 0, 0);
 	  if (debug_lin_lwp)
@@ -822,6 +833,88 @@ stop_wait_callback (struct lwp_info *lp,
   return 0;
 }
 
+/* Check whether PID has any pending signals in FLUSH_MASK.  If so set
+   the appropriate bits in PENDING, and return 1 - otherwise return 0.  */
+
+static int
+lin_lwp_has_pending (int pid, sigset_t *pending, sigset_t *flush_mask)
+{
+  sigset_t blocked, ignored;
+  int i;
+
+  linux_proc_pending_signals (pid, pending, &blocked, &ignored);
+
+  if (!flush_mask)
+    return 0;
+
+  for (i = 1; i < NSIG; i++)
+    if (sigismember (pending, i))
+      if (!sigismember (flush_mask, i)
+	  || sigismember (&blocked, i)
+	  || sigismember (&ignored, i))
+	sigdelset (pending, i);
+
+  if (sigisemptyset (pending))
+    return 0;
+
+  return 1;
+}
+
+/* DATA is interpreted as a mask of signals to flush.  If LP has
+   signals pending, and they are all in the flush mask, then arrange
+   to flush them.  LP should be stopped, as should all other threads
+   it might share a signal queue with.  */
+
+static int
+flush_callback (struct lwp_info *lp, void *data)
+{
+  sigset_t *flush_mask = data;
+  sigset_t pending, intersection, blocked, ignored;
+  int pid, status;
+
+  /* Normally, when an LWP exits, it is removed from the LWP list.  The
+     last LWP isn't removed till later, however.  So if there is only
+     one LWP on the list, make sure it's alive.  */
+  if (lwp_list == lp && lp->next == NULL)
+    if (!lin_lwp_thread_alive (lp->ptid))
+      return 0;
+
+  /* Just because the LWP is stopped doesn't mean that new signals
+     can't arrive from outside, so this function must be careful of
+     race conditions.  However, because all threads are stopped, we
+     can assume that the pending mask will not shrink unless we resume
+     the LWP, and that it will then get another signal.  We can't
+     control which one, however.  */
+
+  if (lp->status)
+    {
+      if (debug_lin_lwp)
+	printf_unfiltered ("FC: LP has pending status %06x\n", lp->status);
+      if (WIFSTOPPED (lp->status) && sigismember (flush_mask, WSTOPSIG (lp->status)))
+	lp->status = 0;
+    }
+
+  while (lin_lwp_has_pending (GET_LWP (lp->ptid), &pending, flush_mask))
+    {
+      int ret;
+      
+      errno = 0;
+      ret = ptrace (PTRACE_CONT, GET_LWP (lp->ptid), 0, 0);
+      if (debug_lin_lwp)
+	fprintf_unfiltered (gdb_stderr,
+			    "FC: Sent PTRACE_CONT, ret %d %d\n", ret, errno);
+
+      lp->stopped = 0;
+      stop_wait_callback (lp, flush_mask);
+      if (debug_lin_lwp)
+	fprintf_unfiltered (gdb_stderr,
+			    "FC: Wait finished; saved status is %d\n",
+			    lp->status);
+    }
+
+  return 0;
+}
+
 /* Return non-zero if LP has a wait status pending.  */
 
 static int
@@ -1465,6 +1558,7 @@ retry:
   /* ... and wait until all of them have reported back that they're no
      longer running.  */
   iterate_over_lwps (stop_wait_callback, &flush_mask);
+  iterate_over_lwps (flush_callback, &flush_mask);
 
   /* If we're not waiting for a specific LWP, choose an event LWP from
      among those that have had events.  Giving equal priority to all
Index: linux-nat.h
===================================================================
RCS file: /cvs/src/src/gdb/linux-nat.h,v
retrieving revision 1.3
diff -u -p -r1.3 linux-nat.h
--- linux-nat.h	17 Aug 2003 18:22:25 -0000	1.3
+++ linux-nat.h	7 Sep 2003 18:46:07 -0000
@@ -65,6 +65,9 @@ extern int linux_proc_xfer_memory (CORE_
 				   int write, struct mem_attrib *attrib,
 				   struct target_ops *target);
 
+/* Find process PID's pending signal set from /proc/pid/status.  */
+void linux_proc_pending_signals (int pid, sigset_t *pending, sigset_t *blocked, sigset_t *ignored);
+
 /* linux-nat functions for handling fork events.  */
 extern void linux_record_stopped_pid (int pid);
 extern void linux_enable_event_reporting (ptid_t ptid);
Index: linux-proc.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-proc.c,v
retrieving revision 1.18
diff -u -p -r1.18 linux-proc.c
--- linux-proc.c	27 Aug 2003 15:41:41 -0000	1.18
+++ linux-proc.c	7 Sep 2003 18:46:07 -0000
@@ -35,6 +35,8 @@
 #include "cli/cli-decode.h"	/* for add_info */
 #include "gdb_string.h"
 
+#include <signal.h>
+
 #include "linux-nat.h"
 
 #ifndef O_LARGEFILE
@@ -621,4 +623,85 @@ linux_proc_xfer_memory (CORE_ADDR addr, 
 
   close (fd);
   return ret;
+}
+
+/* Parse LINE as a signal set and add its set bits to SIGS.  */
+
+static void
+linux_proc_add_line_to_sigset (const char *line, sigset_t *sigs)
+{
+  int len = strlen (line) - 1;
+  const char *p;
+  int signum;
+
+  if (line[len] != '\n')
+    error ("Could not parse signal set: %s", line);
+
+  p = line;
+  signum = len * 4;
+  while (len-- > 0)
+    {
+      int digit;
+
+      if (*p >= '0' && *p <= '9')
+	digit = *p - '0';
+      else if (*p >= 'a' && *p <= 'f')
+	digit = *p - 'a' + 10;
+      else
+	error ("Could not parse signal set: %s", line);
+
+      signum -= 4;
+
+      if (digit & 1)
+	sigaddset (sigs, signum + 1);
+      if (digit & 2)
+	sigaddset (sigs, signum + 2);
+      if (digit & 4)
+	sigaddset (sigs, signum + 3);
+      if (digit & 8)
+	sigaddset (sigs, signum + 4);
+
+      p++;
+    }
+}
+
+/* Find process PID's pending signals from /proc/pid/status and set SIGS
+   to match.  */
+
+void
+linux_proc_pending_signals (int pid, sigset_t *pending, sigset_t *blocked, sigset_t *ignored)
+{
+  FILE *procfile;
+  char buffer[MAXPATHLEN], fname[MAXPATHLEN];
+  int signum;
+
+  sigemptyset (pending);
+  sigemptyset (blocked);
+  sigemptyset (ignored);
+  sprintf (fname, "/proc/%d/status", pid);
+  procfile = fopen (fname, "r");
+  if (procfile == NULL)
+    error ("Could not open %s", fname);
+
+  while (fgets (buffer, MAXPATHLEN, procfile) != NULL)
+    {
+      /* Normal queued signals are on the SigPnd line in the status
+	 file.  However, 2.6 kernels also have a "shared" pending queue
+	 for delivering signals to a thread group, so check for a ShdPnd
+	 line also.
+
+	 Unfortunately some Red Hat kernels include the shared pending queue
+	 but not the ShdPnd status field.  */
+
+      if (strncmp (buffer, "SigPnd:\t", 8) == 0)
+	linux_proc_add_line_to_sigset (buffer + 8, pending);
+      else if (strncmp (buffer, "ShdPnd:\t", 8) == 0)
+	linux_proc_add_line_to_sigset (buffer + 8, pending);
+      else if (strncmp (buffer, "SigBlk:\t", 8) == 0)
+	linux_proc_add_line_to_sigset (buffer + 8, blocked);
+      else if (strncmp (buffer, "SigIgn:\t", 8) == 0)
+	linux_proc_add_line_to_sigset (buffer + 8, ignored);
+    }
+
+  fclose (procfile);
 }