SIGIO and the NS port

SIGIO and the NS port

[Alan Third]

I think I’ve just found a simple solution to a whole class of bugs in
the NS port.

We’ve had a number of bug reports where Emacs crashes while doing UI
related things, for example minimizing the frame, and generally we’ve
found the best way to deal with these is by wrapping the relevant bit
in block_input/unblock_input.

Today I discovered that config.h on my machine has two defines related
to SIGIO:

#define INTERRUPT_INPUT 1
#define USABLE_SIGIO 1

The NS port doesn’t use SIGIO to signal for input (I think it polls
using ns_select), and if I comment these two defines out I can remove
block_input/unblock_input from various places with no ill effects.

Does anyone know of any reason not to remove these two defines?

How would I go about doing that? I assume config.h is generated by
./configure, but I don’t know anything about that side of things.

Thanks!
-- 
Alan Third

Re: SIGIO and the NS port

[Eli Zaretskii]

> Date: Fri, 10 Mar 2017 12:41:17 +0000
> From: Alan Third <alan@idiocy.org>
> 
> Does anyone know of any reason not to remove these two defines?
> 
> How would I go about doing that? I assume config.h is generated by
> ./configure, but I don’t know anything about that side of things.

Yes, it's generated by configure, but you can override what it does in
conf_post.h, with suitable preprocessor conditionals.

Re: SIGIO and the NS port

[Alan Third]

On Fri, Mar 10, 2017 at 12:41:17PM +0000, Alan Third wrote:
> Today I discovered that config.h on my machine has two defines related
> to SIGIO:
> 
> #define INTERRUPT_INPUT 1
> #define USABLE_SIGIO 1
> 
> The NS port doesn’t use SIGIO to signal for input (I think it polls
> using ns_select), and if I comment these two defines out I can remove
> block_input/unblock_input from various places with no ill effects.

It looks like this isn’t right. The NS port works without SIGIO most
of the time, but if wait_reading_process_output is called with
read_kbd=0 then it goes into an infinite loop. (At least, I think
that’s what’s happening.)

I was hoping this would work as it would mean I could remove
block_input/unblock_input from ns_select, which appears to be the main
problem with concurrency.
-- 
Alan Third

Re: SIGIO and the NS port

[Alan Third]

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

On Mon, Mar 13, 2017 at 04:38:17PM +0000, Alan Third wrote:
> On Fri, Mar 10, 2017 at 12:41:17PM +0000, Alan Third wrote:
> > The NS port doesn’t use SIGIO to signal for input (I think it polls
> > using ns_select), and if I comment these two defines out I can remove
> > block_input/unblock_input from various places with no ill effects.
> 
> It looks like this isn’t right. The NS port works without SIGIO most
> of the time, but if wait_reading_process_output is called with
> read_kbd=0 then it goes into an infinite loop. (At least, I think
> that’s what’s happening.)

So I’ve been using the attached patch for a few weeks now with no
issues.

I’ve stopped the signal handler from responding to SIGIO, and the two
places where the NS GUI used to raise SIGIO have been replaced with
direct calls to the SIGIO handler code.

I’m not sure if this is a sensible approach. It fixes at least one
crash, and we could remove some HAVE_NS‐only code, so I think it’s
desirable. I’m just not sure about my implementation. Any thoughts,
anyone?
-- 
Alan Third

[-- Attachment #2: 0001-Don-t-use-SIGIO-in-NS.patch --]
[-- Type: text/plain, Size: 2831 bytes --]

From bff3dbfe43a24ee924edd777a7a876b627f94f11 Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Sat, 29 Apr 2017 23:35:16 +0100
Subject: [PATCH] Don't use SIGIO in NS

* src/keyboard.c (handle_user_signal) [HAVE_NS]: Don't handle SIGIO.
* src/keyboard.h (handle_input_available_signal) [HAVE_NS]: Make
available to external code.
* src/nsterm.m (hold_event, ns_Select): Call handle_input_available_signal
directly.
* src/sysdep.c (emacs_sigaction_init) [HAVE_NS]: Ignore SIGIO.
---
 src/keyboard.c |  3 ++-
 src/keyboard.h | 11 +++++++++++
 src/nsterm.m   |  4 ++--
 src/sysdep.c   |  2 +-
 4 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index c9fa2a9f5e..d22f7b08c6 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -7264,7 +7264,8 @@ handle_user_signal (int sig)
           }
 
 	p->npending++;
-#ifdef USABLE_SIGIO
+#if defined (USABLE_SIGIO) && !defined (HAVE_NS)
+        /* Dont use the interrupt handler in NS. */
 	if (interrupt_input)
 	  handle_input_available_signal (sig);
 	else
diff --git a/src/keyboard.h b/src/keyboard.h
index 2219c01135..29279bef86 100644
--- a/src/keyboard.h
+++ b/src/keyboard.h
@@ -495,6 +495,17 @@ extern void mark_kboards (void);
 extern const char *const lispy_function_keys[];
 #endif
 
+#ifdef HAVE_NS
+/* The NS port only uses SIGIO internally, and even then only in two
+   places in the code, but it causes crashes if certain code is not
+   properly wrapped in block_input/unblock_input.
+
+   Since it's only used internally, we should be able to disable it,
+   and call the SIGIO handler directly.
+ */
+extern void handle_input_available_signal (int sig);
+#endif
+
 extern char const DEV_TTY[];
 
 INLINE_HEADER_END
diff --git a/src/nsterm.m b/src/nsterm.m
index c22c5a70ba..00b7a89472 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -472,7 +472,7 @@ - (NSColor *)colorUsingDefaultColorSpace
 
   hold_event_q.q[hold_event_q.nr++] = *event;
   /* Make sure ns_read_socket is called, i.e. we have input.  */
-  raise (SIGIO);
+  handle_input_available_signal (SIGIO);
   send_appdefined = YES;
 }
 
@@ -4289,7 +4289,7 @@ in certain situations (rapid incoming events).
   if (hold_event_q.nr > 0)
     {
       /* We already have events pending. */
-      raise (SIGIO);
+      handle_input_available_signal (SIGIO);
       errno = EINTR;
       return -1;
     }
diff --git a/src/sysdep.c b/src/sysdep.c
index 91b2a5cb94..70fdf92964 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -1606,7 +1606,7 @@ emacs_sigaction_init (struct sigaction *action, signal_handler_t handler)
     {
       sigaddset (&action->sa_mask, SIGINT);
       sigaddset (&action->sa_mask, SIGQUIT);
-#ifdef USABLE_SIGIO
+#if defined (USABLE_SIGIO) && !defined (HAVE_NS)
       sigaddset (&action->sa_mask, SIGIO);
 #endif
     }
-- 
2.12.0

Re: SIGIO and the NS port

[Paul Eggert]

Alan Third wrote:
> I’ve stopped the signal handler from responding to SIGIO,

I'm not following, as sigaction (SIGIO, ...) is still being called, so the 
signal handler deliver_input_available_signal is still responding to SIGIO, and 
is setting pending_signals to true.

The changes you made affect SIGUSR1 and SIGUSR2 handling, since they patch 
handle_user_signal: they cause the SIGUSR1 and SIGUSR2 handler to not process 
SIGIO events. Also, they prevent SIGIO from being masked out during non-SIGIO 
signal handlers. Is that what you intended? I'm not getting the connection 
between these non-SIGIO handlers and interrupt_input.

> and the two
> places where the NS GUI used to raise SIGIO have been replaced with
> direct calls to the SIGIO handler code.

That sounds like a reasonable thing to do in any event. I don't see why the code 
needs to do a raise (SIGIO) when it can do a handle_input_available_signal (SIGIO);

Re: SIGIO and the NS port

[Alan Third]

On Sun, May 21, 2017 at 12:46:53AM -0700, Paul Eggert wrote:
> Alan Third wrote:
> > I’ve stopped the signal handler from responding to SIGIO,
> 
> I'm not following, as sigaction (SIGIO, ...) is still being called, so the
> signal handler deliver_input_available_signal is still responding to SIGIO,
> and is setting pending_signals to true.
> 
> The changes you made affect SIGUSR1 and SIGUSR2 handling, since they patch
> handle_user_signal: they cause the SIGUSR1 and SIGUSR2 handler to not
> process SIGIO events. Also, they prevent SIGIO from being masked out during
> non-SIGIO signal handlers. Is that what you intended? I'm not getting the
> connection between these non-SIGIO handlers and interrupt_input.

I’ve probably misunderstood what the code was doing. Would it be best
to just leave the signal handlers as‐is, but replace the calls to
raise(SIGIO) with direct calls as below?

> > and the two
> > places where the NS GUI used to raise SIGIO have been replaced with
> > direct calls to the SIGIO handler code.
> 
> That sounds like a reasonable thing to do in any event. I don't see why the
> code needs to do a raise (SIGIO) when it can do a
> handle_input_available_signal (SIGIO);

Thanks!
-- 
Alan Third